don't include offline CPUs in summary for OpenBSD #580

sthen · 2021-03-27T13:30:28Z

By default, OpenBSD disables SMT (hyperthreading) cpu pseudo-cores.
This can be changed at runtime by setting the hw.smt sysctl so they
may become active later, therefore they are still present in cpu
stat structures but are marked as offline.

As done with native top(1), this drops them from the cpu summary
graphs.

openbsd/OpenBSDProcessList.h

BenBE · 2021-03-28T20:36:41Z

Can you squash your commits? Makes the history cleaner and easier to follow.

sthen · 2021-03-29T12:47:46Z

On 2021/03/28 13:37, BenBE wrote: Can you squash your commits? Makes the history cleaner and easier to follow. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.*

If you can tell me what to type to do that, yes. (I don't normally use git).

BenBE · 2021-03-29T14:57:26Z

When on your branch:

# Fetch latest changes
git fetch --all

# Apply latest changes
git pull --rebase your-repo your-branch

# Base local changes on upstream master
git rebase -i htop-upstream/master

# Push the modified branch to your repo to update this PR.
git push --force-with-lease your-repo your-branch

In the editor that opens change the second line from pick to fixup. See also the notes given in the descriptive text in that editor.

cgzones · 2021-03-30T21:33:19Z

openbsd/OpenBSDProcessList.c

+
+      if (cpu_index_c == pl->cpuCount)
+         break;
+   }


What is this loop about?
cpu_index_c is being increased and the loop is aborted when it reaches a specific value, but there are no further implications.

Hmm, looks like I missed a bit when I was merging the patch which was originally done against 3.0.1. It will work on Intel CPUs without that loop (the skipped cpus are the higher numbered ones) but they're interspersed on AMD (skipped would be 1,3,5,7,..) I just pushed an updated version that I think should do the trick (I don't have an AMD cpu with SMT to test that part with though)

I asked someone to test on AMD, this version is looking ok.

cgzones · 2021-04-14T15:43:20Z

openbsd/OpenBSDProcessList.c

@@ -53,6 +57,13 @@ ProcessList* ProcessList_new(UsersTable* usersTable, Hashtable* pidMatchList, ui
      pl->cpuCount = 1;
   }
   opl->cpus = xCalloc(pl->cpuCount + 1, sizeof(CPUData));
+   opl->cpuIndex = xRealloc(opl->cpuIndex, pl->cpuCount * sizeof(int));


*opl was initialized in this function, no need to realloc (just malloc (or calloc, see later comment))

cgzones · 2021-04-14T15:43:40Z

openbsd/OpenBSDProcessList.c

+   size = sizeof(cpu_stats);
+   for (unsigned int i = 0; i < ncpu; i++) {
+      ncmib[2] = i;
+      sysctl(ncmib, 3, &cpu_stats, &size, NULL, 0);


please check for a sysctl failure

cgzones · 2021-04-14T15:51:15Z

openbsd/OpenBSDProcessList.h

@@ -43,7 +43,7 @@ typedef struct OpenBSDProcessList_ {

   CPUData* cpus;
   int cpuSpeed;
-
+   int* cpuIndex;


Maybe include the index in the CPUData struct (the one unused int of the average entry is negligible)

By default, OpenBSD disables SMT (hyperthreading) cpu pseudo-cores. This can be changed at runtime by setting the hw.smt sysctl so they may become active later, therefore they are still present in cpu stat structures but are marked as offline. As done with native top(1), this drops them from the cpu summary graphs.

BenBE · 2021-04-17T23:24:23Z

openbsd/OpenBSDProcessList.c

@@ -76,6 +86,21 @@ ProcessList* ProcessList_new(UsersTable* usersTable, Hashtable* pidMatchList, ui

   opl->cpuSpeed = -1;

+   size = sizeof(cpu_stats);
+   for (unsigned int i = 0; i < ncpu; i++) {
+      ncmib[2] = i;


Move actual initialization of ncmib here and make it const.

Suggested change

ncmib[2] = i;

const int ncmib[] = { CTL_KERN, KERN_CPUSTATS, i };

BenBE · 2021-04-17T23:26:12Z

openbsd/OpenBSDProcessList.c

   int r;
+   unsigned int cpu_index_c = 0, ncpu;


Single variable per line …

BenBE · 2021-04-17T23:27:18Z

openbsd/OpenBSDProcessList.c

+   size = sizeof(cpu_stats);
+   for (unsigned int i = 0; i < ncpu; i++) {
+      ncmib[2] = i;
+      if (sysctl(ncmib, 3, &cpu_stats, &size, NULL, 0) < 0) {


Shouldn't size be re-initialized inside the loop?

sthen · 2021-04-18T09:52:00Z

Since you have a pretty clear idea of what changes you want, could one of you fix it up please? Most of my development work is with OpenBSD and we don't use git so what is probably a 2 minute job for you takes me about half an hour each time to figure out how to rebase the commit history how you want it. Thanks!

cgzones · 2021-04-18T14:59:53Z

Applied with adjustments via feec16c.
Thanks for your contribution.

sthen · 2021-04-18T19:41:15Z

Many thanks.

Currently htop does not support offline CPUs and hot-swapping, e.g. via echo 0 > /sys/devices/system/cpu/cpu2/online Split the current single cpuCount variable into activeCPUs and existingCPUs. Supersedes: htop-dev#650 Related: htop-dev#580

cgzones reviewed Mar 28, 2021

View reviewed changes

openbsd/OpenBSDProcessList.h Outdated Show resolved Hide resolved

cgzones reviewed Mar 30, 2021

View reviewed changes

cgzones requested changes Apr 14, 2021

View reviewed changes

BenBE requested changes Apr 17, 2021

View reviewed changes

BenBE added BSD 🐡 Issues related to *BSD bug 🐛 Something isn't working enhancement Extension or improvement to existing feature labels Apr 18, 2021

BenBE added this to the 3.0.6 milestone Apr 18, 2021

cgzones closed this Apr 18, 2021

sthen deleted the openbsd_cpuonline branch April 18, 2021 19:41

cgzones mentioned this pull request Jun 12, 2021

Rework CPU counting #656

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

don't include offline CPUs in summary for OpenBSD #580

don't include offline CPUs in summary for OpenBSD #580

sthen commented Mar 27, 2021

BenBE commented Mar 28, 2021

sthen commented Mar 29, 2021 via email

BenBE commented Mar 29, 2021

cgzones Mar 30, 2021

sthen Apr 3, 2021 •

edited

Loading

sthen Apr 3, 2021

cgzones Apr 14, 2021

cgzones Apr 14, 2021

cgzones Apr 14, 2021

BenBE Apr 17, 2021

BenBE Apr 17, 2021

BenBE Apr 17, 2021

sthen commented Apr 18, 2021

cgzones commented Apr 18, 2021

sthen commented Apr 18, 2021

	ncmib[2] = i;
	const int ncmib[] = { CTL_KERN, KERN_CPUSTATS, i };

don't include offline CPUs in summary for OpenBSD #580

don't include offline CPUs in summary for OpenBSD #580

Conversation

sthen commented Mar 27, 2021

BenBE commented Mar 28, 2021

sthen commented Mar 29, 2021 via email

BenBE commented Mar 29, 2021

Choose a reason for hiding this comment

sthen Apr 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sthen commented Apr 18, 2021

cgzones commented Apr 18, 2021

sthen commented Apr 18, 2021

sthen Apr 3, 2021 •

edited

Loading