[wip] show number of CPU cores in node list #27662
Conversation
|
This change is |
pkg/server/status/recorder.go
Outdated
| } | ||
| for _, cpuInfo := range cpuInfos { | ||
| // TODO(vilterp): figure out a more robust way of accounting for hyperthreading than `*2` | ||
| numCores += cpuInfo.Cores * 2 |
There was a problem hiding this comment.
logical_cores = physical_cores*2 seems to be the rule for Intel (according to https://en.wikipedia.org/wiki/Hyper-threading, and it works on my laptop), but may not be the case for e.g. AMD. Not sure how best to handle this.
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained
pkg/server/status/recorder.go, line 498 at r2 (raw file):
Previously, vilterp (Pete Vilter) wrote…
logical_cores = physical_cores*2seems to be the rule for Intel (according to https://en.wikipedia.org/wiki/Hyper-threading, and it works on my laptop), but may not be the case for e.g. AMD. Not sure how best to handle this.
If the UI says "cores", we should report the number of cores, not the number of threads. And we need to be careful about doubling things when another layer has already doubled them (trust the libraries we're using to do a better job of handling things like intel vs amd than we can)
However, the gopsutil library doesn't look like a reliable source for this information. On linux as far as I can tell, it gives you threads instead of cores (one cpuInfo object per thread, and cpuInfo.Cores is always 1). Darwin, on the other hand, returns one cpuInfo object per socket, sets cpuInfo.Cores correctly, but doesn't report threads at all. There's an open issue about it being incorrect on windows too.
|
Thanks for pointing out these problems Ben. I changed it to use https://golang.org/pkg/runtime/#NumCPU, which returns 8 on my Mac (4 cores * 2 for hyperthreading), which seems to indicate that it's returning "threads", although the Go docs say "logical CPUs". Like the gopsutil library, the Go stdlib has has different implementations for different OSs (https://github.com/golang/go/search?p=2&q=ncpu&unscoped_q=ncpu), but I have more confidence in the reliability of something in the stdlib. As for what to call it, "threads" may be the correct term, but without qualification I think people would probably think it's referring to how many OS threads are running, so I'm inclined to go with "Cores". cc @couchand — yet another column to decide whether to put in :) |
There was a problem hiding this comment.
As for what to call it, "threads" may be the correct term, but without qualification I think people would probably think it's referring to how many OS threads are running, so I'm inclined to go with "Cores".
Agreed that "threads" is confusing, but we can't call it "cores" because that has a specific, different meaning in this context. I think "CPUs" may be the best label to use here.
(And I think we do want to report CPUs/threads instead of cores - CPUs are tangible things (units of execution and scheduling), cores are just an implementation detail)
Reviewable status:
Release note: None
Release note (admin ui change): show number of CPUs for each node on the overview page.
|
Ok, changed it to CPUs. That does evoke a unit of execution & scheduling without getting into the implementation details; seems like the Go authors may have come to the same conclusion by calling the stdlib function @petermattis I know you had asked for this; do you agree on terminology? |
That's a bikeshed I do not wish to paint. I'll trust the folks on this thread to choose a good color. |
|
Closed in favor of #28189, so we can see a few related new metrics in the node list. |
Also, there might be a better way to do this than saving it in NodeSummary every ten seconds (i.e. just on the RPC's codepath). Thoughts, @mrtracy?
TODOs: