Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Duplicate and unnecessary system call in pkg/kernel/GetVersionInfo() #2459

Closed
eryx opened this issue Nov 9, 2018 · 10 comments · Fixed by #2463
Closed

Duplicate and unnecessary system call in pkg/kernel/GetVersionInfo() #2459

eryx opened this issue Nov 9, 2018 · 10 comments · Fixed by #2463

Comments

@eryx
Copy link

eryx commented Nov 9, 2018

Ⅰ. Issue Description

as the image (perf record to flame graph) below, i found a CPU hot point in the pkg/kernel/GetVersionInfo().

this method may only need to be called once, because the kernel version is fixed after the system is up. ?

screenshot at 2018-11-09 23-20-20

Ⅱ. Describe what happened

Ⅲ. Describe what you expected to happen

Ⅳ. How to reproduce it (as minimally and precisely as possible)

Ⅴ. Anything else we need to know?

Ⅵ. Environment:

  • pouch version (use pouch version): 1.0.0
  • OS (e.g. from /etc/os-release): CentOS 7.x
  • Kernel (e.g. uname -a):
  • Install tools: rpm -ivh
  • Others:
@houstar
Copy link
Contributor

houstar commented Nov 9, 2018

Good idea, you may assume that kernel version is fixed for most cases, but for kexec? Since kexec can be dynamically switch the kernel version :-)

@eryx
Copy link
Author

eryx commented Nov 9, 2018

@houstar
yes, the kexec will cause problems

@houstar
Copy link
Contributor

houstar commented Nov 9, 2018

This is trade-off problem 👍 Let's the architects make the decision. If plan to merge, add this kexec problem as Limitation 💯

@HusterWan
Copy link
Contributor

Great point, I even not think about kexec at all. 🤣

But, IMO, the kexec is not a problem, right now. We may first think about fix the hot point of pouchd, then care about the kexec problem. WDYT? @eryx @houstar

@houstar
Copy link
Contributor

houstar commented Nov 9, 2018

I'm totally agree with you @HusterWan since this is trade-off problem and add kexec as limitation.

@zhuangqh
Copy link
Contributor

zhuangqh commented Nov 9, 2018

have you compare it with syscall.Uname?

package main

import (
	"fmt"
	"syscall"
)

func main() {
	err := syscall.Uname(&buf)
	if err != nil {
		fmt.Println(err)
		return
	}

	fmt.Println(charsToString(buf.Release[:]))
}

func charsToString(ca []int8) string {
	s := make([]byte, len(ca))
	var lens int
	for ; lens < len(ca); lens++ {
		if ca[lens] == 0 {
			break
		}
		s[lens] = uint8(ca[lens])
	}
	return string(s[0:lens])
}
$ root@pouch:~/go/src/github.com/t# go build main.go
$ root@pouch:~/go/src/github.com/t# ./main
4.4.0-137-generic

What's the performance of this one?

@SimonCqk
Copy link
Contributor

@zhuangqh I've just tried a simple test, use unix.Uname instead of syscall.Uname, comparing with exec.Run version, and the result shows that the performance difference can't be omitted.
Call 1000 times in one loop:
exec.Run takes 3.3s while unix.Uname takes 8.5ms (in mean)

@SimonCqk
Copy link
Contributor

@zhuangqh here is my code:

func GetKernelVersionByUnix() (*VersionInfo, error) {
	var (
		kernel, major, minor int
		flavor               string
	)
	buf := unix.Utsname{}
	err := unix.Uname(&buf)
	if err != nil {
		return nil, err
	}
	release := string(buf.Release[:])
	parsed, _ := fmt.Sscanf(release, "%d.%d.%d-%s", &kernel, &major, &minor, &flavor)
	if parsed < 3 {
		return nil, fmt.Errorf("Can't parse kernel version, release: %s" + release)
	}

	return &VersionInfo{
		Kernel: kernel,
		Major:  major,
		Minor:  minor,
		Flavor: flavor,
	}, nil
}

@zhuangqh
Copy link
Contributor

@zhuangqh here is my code:

func GetKernelVersionByUnix() (*VersionInfo, error) {
	var (
		kernel, major, minor int
		flavor               string
	)
	buf := unix.Utsname{}
	err := unix.Uname(&buf)
	if err != nil {
		return nil, err
	}
	release := string(buf.Release[:])
	parsed, _ := fmt.Sscanf(release, "%d.%d.%d-%s", &kernel, &major, &minor, &flavor)
	if parsed < 3 {
		return nil, fmt.Errorf("Can't parse kernel version, release: %s" + release)
	}

	return &VersionInfo{
		Kernel: kernel,
		Major:  major,
		Minor:  minor,
		Flavor: flavor,
	}, nil
}

good job! Could you submit a pull request?

@eryx
Copy link
Author

eryx commented Nov 10, 2018

bench in multi-implementation

=== RUN TestGetKernelVersion
3.10.0-862.14.4.el7.x86_64
--- PASS: TestGetKernelVersion (0.00s)
goos: linux
goarch: amd64
pkg: github.com/alibaba/pouch/pkg/kernel
Benchmark_GetKernelVersion-8 3000 550459 ns/op
Benchmark_GetKernelVersionByVarCache-8 2000000000 1.78 ns/op
Benchmark_GetKernelVersionByVarCacheWithTTL-8 50000000 39.8 ns/op
Benchmark_GetKernelVersionByUnix-8 500000 3258 ns/op
Benchmark_GetKernelVersionBySyscall-8 1000000 2279 ns/op
PASS
ok github.com/alibaba/pouch/pkg/kernel 11.459s

Codes:
https://github.com/eryx/pouch/blob/kernel-version-bench/pkg/kernel/kernel.go
https://github.com/eryx/pouch/blob/kernel-version-bench/pkg/kernel/kernel_test.go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants