Fix and improve NICE on FreeBSD #1461

herrhotzenplotz · 2024-04-20T17:51:43Z

Previously the niceness value on FreeBSD was utterly broken (to my and others annoyance):

it displayed out-of-range values for idletime and realtime scheduled processes
there was no indicator for such scheduled processes

This fixes:

The calculation of the niceness value (see https://github.com/freebsd/freebsd-src/blob/main/usr.bin/top/machine.c#L1182)
Add a column (SCHEDCLASS) that tells whether a process is idletime, realtime, timeshared or otherwise scheduled

I have tested this with other people on a few architectures and different versions of FreeBSD and it seems to be working just fine.

Please tell me if I can improve on this or if something is utterly wrong and I should fix.

swills · 2024-04-20T18:07:52Z

Here's a before and after screenshot

clausecker · 2024-04-20T18:40:27Z

Very cool! Does this also fix that rtprio tasks were not shown in blue on the CPU usage bar?
@BenBE, what do you think of this? Is there a way to show for the scheduling class what level the process has within the scheduling class?

herrhotzenplotz · 2024-04-20T18:44:09Z

Very cool! Does this also fix that rtprio tasks were not shown in blue on the CPU usage bar?

No

Is there a way to show for the scheduling class what level the process has within the scheduling class?

Doesn't the niceness indicate that now?

clausecker · 2024-04-20T18:52:25Z

Is there a way to show for the scheduling class what level the process has within the scheduling class?

I think niceness and priority class are orthogonal process. E.g. I can use idprio 5 to put a process into idle priority class 5, which means it only runs if no process of lower idle priority class is runnable.

BenBE

Where does this touch the displayed NICE value?

freebsd/FreeBSDProcessTable.c

herrhotzenplotz · 2024-04-20T19:33:28Z

Where does this touch the displayed NICE value?

I don't quite understand what you mean by that. It computes the (now correct) niceness in the switch statement now.

The nice handling on FreeBSD was broken for multiple reasons: - Idletime and Realtime scheduled processes were not properly detected and insane values were reported back - There was no indicator for processes that were scheduled in idletime or realtime This now adds a platform specific field for the scheduling type which one can add to interpret the niceness of a process better. Signed-off-by: Nico Sonack <nsonack@herrhotzenplotz.de>

BenBE · 2024-04-20T20:13:43Z

Where does this touch the displayed NICE value?

I don't quite understand what you mean by that. It computes the (now correct) niceness in the switch statement now.

Missed that when I had a first quick look at the code. Did some further changes. Apart from this LGTM; didn't test though. Maybe @fraggerfox + @clausecker can take a look&test?

IIRC we already have a more or less "global" column for the priority class that could/should be re-used here. Also, is that value different from the one shown by SCHEDULERPOLICY?

@clausecker For the processes to be shown in blue for the CPU time, they need to have "lower than normal" priority IIRC. If they previously didn't due to this bug, they now should be accounted accordingly.

clausecker · 2024-04-20T20:16:08Z

@BenBE On FreeBSD, everything in the "idle priority" scheduler class has "lower than normal" priority. Maybe new colours for "idle priority" and "real time priority" could be helpful instead.

BenBE · 2024-04-20T20:20:50Z

@BenBE On FreeBSD, everything in the "idle priority" scheduler class has "lower than normal" priority. Maybe new colours for "idle priority" and "real time priority" could be helpful instead.

Okay, this would thus require a second change (commit) to update the accounting on the CPU meter. Will need to look up how that code works exactly though …

herrhotzenplotz · 2024-04-20T20:21:39Z

IIRC we already have a more or less "global" column for the priority class that could/should be re-used here. Also, is that value different from the one shown by SCHEDULERPOLICY?

Yes that value is different.

BenBE · 2024-04-20T20:32:51Z

@BenBE On FreeBSD, everything in the "idle priority" scheduler class has "lower than normal" priority. Maybe new colours for "idle priority" and "real time priority" could be helpful instead.

Okay, this would thus require a second change (commit) to update the accounting on the CPU meter. Will need to look up how that code works exactly though …

Okay, quick update:

The CPU meter takes the NICE time for detailled CPU usage statistics from the values set in Platform_setCPUValues. These are set via FreeBSDMachine_scanCPU AFAICS.

freebsd/FreeBSDProcessTable.c

freebsd/FreeBSDProcess.c

herrhotzenplotz · 2024-04-21T12:18:31Z

Example - now with a SCHEDCLASS_ enum.

freebsd/FreeBSDProcess.c

Explorer09 · 2024-04-21T12:30:41Z

freebsd/FreeBSDProcess.c

@@ -73,22 +74,40 @@ void Process_delete(Object* cast) {
   free(this);
 }

+static_assert(MAX_SCHEDCLASS == 5, "The lookup table below isn't in sync with the SCHEDCLASS_ enum. Please update.");
+static const char FreeBSD_schedclassChars[MAX_SCHEDCLASS] = {


The static assert is probably unnecessary.

It's good to give some rationale on how the characters are chosen this way, because this thing is likely added into htop documentation. E.g. is the blank character (SCHEDCLASS_TIMESHARE) considered normal for a FreeBSD process, and why is SCHEDCLASS_ITHD represented as a hyphen?

I've put in the static assert for a few reasons:

leave a compiler hint whenever someone changes the schedclass enum

and thus prevent access to uninitialised data when such changes happen

Yes, it is unlikely this enum will ever change - this is just being less hostile to whoever works on this code and to prevent potential bugs.

@herrhotzenplotz There are other mapping tables from an enum in htop codebase like this, and none of them needs a static assert. It's unlikely you need one here either.
When an array is initialized with a "named index" initializer like this, unspecified indices would have zero value. If you need an assert, do so on the mapping side so that the mapped character isn't '\0'.

There are other mapping tables from an enum in htop codebase like this, and none of them needs a static assert.

Well I'd say this would be a problem with those "other mapping tables". Having the static_assert in place does no harm and only improves things. I don't see why anyone would willingly remove this simple assurance.

If you need an assert, do so on the mapping side so that the mapped character isn't '\0'.

I can certainly add this runtime assertion.

Well I'd say this would be a problem with those "other mapping tables". Having the static_assert in place does no harm and only improves things. I don't see why anyone would willingly remove this simple assurance.

First, it's not my decision, I only request this for the sake of code consistency. Second, having an assertion of MAX_SCHEDCLASS == 5 isn't helpful and it partly defeats the purpose of defining the MAX_SCHEDCLASS enum value (which is meant to increase over time).

First, it's not my decision, I only request this for the sake of code consistency.

Sacrificing being prone to bugs for code consistency seems like a very bad thing to me.

Second, having an assertion of MAX_SCHEDCLASS == 5 isn't helpful

Yes it is helpful. The point is to stop compilation, let the developer update the lookup table, correct the static assertion and continue. This is what the assertion message tells. This is a pretty common way to guide someone making changes in large C codebases.

and it partly defeats the purpose of defining the MAX_SCHEDCLASS enum value (which is meant to increase over time).

The reason to define MAX_SCHEDCLASS is to define the static array of the correct size and to range-check incoming enum values.

@herrhotzenplotz Would you raise this issue to a separate discussion?
IMO, a long term solution is for compiler to introduce an array/struct attribute to reject any implicit initialization of array/struct members.

While I personally don't have any objections against this static_assert there are two reasons against this in this particular case:

The htop source is targeted to be compile-able by any compliant C99 compiler. As static_assert was not introduced before C11 (as _Static_assert) or C23 proper this violates the language standard we are aiming for.

The rest of the code base merely assumes for indexed initializers that if there's a MAX_FOO or NUM_FOO constant in an enum it's the last one (i.e. the one with the highest value). Thus having entries defined implicitly below that maximum value is totally fine and expected. The most prominent example is the set of internal IDs for columns.

If you really want to check, I'm totally fine with the runtime check as suggested by @Explorer09.

@BenBE I am fully aware of the fact that C99 does not have static_assert. However, your code base has got compatibility code:

htop/Compat.h

Lines 71 to 73 in 4c0e965

# define _Static_assert(expr, msg) \

extern int (*__Static_assert_function (void)) \

[!!sizeof (struct { int __error_if_negative: (expr) ? 2 : -1; })]

In fact, this static_assert macro was used during my testing.

freebsd/FreeBSDProcess.h

BenBE · 2024-04-21T22:48:00Z

freebsd/FreeBSDProcess.c

@@ -73,22 +74,40 @@ void Process_delete(Object* cast) {
   free(this);
 }

+static_assert(MAX_SCHEDCLASS == 5, "The lookup table below isn't in sync with the SCHEDCLASS_ enum. Please update.");
+static const char FreeBSD_schedclassChars[MAX_SCHEDCLASS] = {


While I personally don't have any objections against this static_assert there are two reasons against this in this particular case:

The htop source is targeted to be compile-able by any compliant C99 compiler. As static_assert was not introduced before C11 (as _Static_assert) or C23 proper this violates the language standard we are aiming for.

The rest of the code base merely assumes for indexed initializers that if there's a MAX_FOO or NUM_FOO constant in an enum it's the last one (i.e. the one with the highest value). Thus having entries defined implicitly below that maximum value is totally fine and expected. The most prominent example is the set of internal IDs for columns.

If you really want to check, I'm totally fine with the runtime check as suggested by @Explorer09.

freebsd/FreeBSDProcess.h

freebsd/FreeBSDProcessTable.c

freebsd/FreeBSDProcess.c

Explorer09 · 2024-04-27T09:45:05Z

freebsd/FreeBSDProcessTable.c

+
+         default:
+            fp->sched_class = SCHEDCLASS_UNKNOWN;
+            proc->nice = kproc->ki_nice - NZERO;


I see implementation differences between yours in this PR and the FreeBSD top source code you cited.

Specifically, when the sched_class is unknown, FreeBSD top simply display "?" for the nice value. It doesn't use the kproc->ki_nice - NZERO as if it were a regular ("time sharing") process.

Is this behavior difference intentional?

I don't see how to represent an unknown nice value since proc->nice is an integer in htop. Setting it to zero would be wrong.

If you have a sentinel value would it be sensible to display N/A in that case?

I guess that makes sense.

Explorer09 · 2024-04-27T09:58:25Z

While I have no more problem with the code qualities of this PR, there is one potential issue that I am not sure if it's worth addressing.

Since it becomes apparent that FreeBSD doesn't use a simple integer nice value for process priority, the sorting of NICE field in htop should no longer be simple integer sorting.

Then I saw this "FreeBSD process priority to nice value" conversation table that I can't tell of it's accurate (from here https://git.quacker.org/d/freebsd-nq/commit/de916c8b747ae5d875788555f6d39adb3fc91e4a):

	    /*
	     * normal time      -> nice value -20 - +20
	     * real time 0 - 31 -> nice value -52 - -21
	     * idle time 0 - 31 -> nice value +21 - +52
	     */

For example, "real time" 31 and "idle time" 31 should definitely not be ordered close together when sorted by NICE. Similarly, "normal time" 20 and "real time" 20 are definitely different.

herrhotzenplotz · 2024-04-27T10:53:00Z

Since it becomes apparent that FreeBSD doesn't use a simple integer nice value for process priority, the sorting of NICE field in htop should no longer be simple integer sorting.

That is a good point.

Then I saw this "FreeBSD process priority to nice value" conversation table that I can't tell of it's accurate (from here https://git.quacker.org/d/freebsd-nq/commit/de916c8b747ae5d875788555f6d39adb3fc91e4a):

This link does not seem to be an official src mirror and it also references code from 2006 which is very much not up-to-date. The current top code looks very different (e.g.: https://codeberg.org/FreeBSD/freebsd-src/src/branch/main/usr.bin/top/machine.c)

Explorer09 · 2024-04-27T11:13:47Z

@herrhotzenplotz

This link does not seem to be an official src mirror and it also references code from 2006 which is very much not up-to-date. The current top code looks very different (e.g.: https://codeberg.org/FreeBSD/freebsd-src/src/branch/main/usr.bin/top/machine.c)

Good reference. I have two questions to ask, if you can help answering those for me.

From what I've seen, there are PRI and NICE fields in FreeBSD top; and they are formatted like the following

                sbuf_printf(procbuf, "%3d ", pp->ki_pri.pri_level - PZERO);
		sbuf_printf(procbuf, "%4s", format_nice(pp));

According to the compare_prio() function in the lower part of the source code, only the PRI field is used for sorting purpose, and the NICE field never used for sorting.

Is the PRI field ever used in the FreeBSD code of htop (this project)?
Is any of the priority fields modifiable in top, by users or administrators? I wish to have an idea on how "Nice-" and "Nice+" commands in htop can work there in FreeBSD.

herrhotzenplotz · 2024-04-27T14:17:47Z

@herrhotzenplotz

This link does not seem to be an official src mirror and it also references code from 2006 which is very much not up-to-date. The current top code looks very different (e.g.: https://codeberg.org/FreeBSD/freebsd-src/src/branch/main/usr.bin/top/machine.c)

Good reference. I have two questions to ask, if you can help answering those for me.

From what I've seen, there are PRI and NICE fields in FreeBSD top; and they are formatted like the following
                sbuf_printf(procbuf, "%3d ", pp->ki_pri.pri_level - PZERO);
		sbuf_printf(procbuf, "%4s", format_nice(pp));
According to the compare_prio() function in the lower part of the source code, only the PRI field is used for sorting purpose, and the NICE field never used for sorting.
1. Is the `PRI` field ever used in the FreeBSD code of htop (this project)?

Yes, the proc->priority field is exactly this value. In fact if you want to sort by "real scheduling priority" you can sort by this value.

2. Is any of the priority fields modifiable in `top`, by users or administrators? I wish to have an idea on how "`Nice-`" and "`Nice+`" commands in htop can work there in FreeBSD.

It is possible, though I typically use the renice, rtprio and idprio commands to accomplish this: https://man.freebsd.org/cgi/man.cgi?rtprio . I don't think you can alter the priority of idletime or realtime scheduled processes from within top (please correct me if I am wrong).

Also: calling renice on an idletime- or realtime scheduled process has no effect.

Relevant syscalls are:

BenBE added enhancement Extension or improvement to existing feature FreeBSD 👹 FreeBSD related issues labels Apr 20, 2024

BenBE added this to the 3.4.0 milestone Apr 20, 2024

BenBE requested changes Apr 20, 2024

View reviewed changes

freebsd/FreeBSDProcessTable.c Outdated Show resolved Hide resolved

herrhotzenplotz force-pushed the freebsd-nice-improved branch from f1e16fe to 3a5dca2 Compare April 20, 2024 19:39

BenBE force-pushed the freebsd-nice-improved branch from 3a5dca2 to 84cb20b Compare April 20, 2024 20:01

Explorer09 reviewed Apr 21, 2024

View reviewed changes

freebsd/FreeBSDProcessTable.c Outdated Show resolved Hide resolved

Explorer09 reviewed Apr 21, 2024

View reviewed changes

freebsd/FreeBSDProcess.c Show resolved Hide resolved

herrhotzenplotz force-pushed the freebsd-nice-improved branch from 84cb20b to fab1bf7 Compare April 21, 2024 12:13

Explorer09 reviewed Apr 21, 2024

View reviewed changes

herrhotzenplotz force-pushed the freebsd-nice-improved branch 2 times, most recently from 3c8e03c to 7b8a4e3 Compare April 21, 2024 13:56

herrhotzenplotz mentioned this pull request Apr 21, 2024

Preventing implicitly initialised values in lookup tables and structs #1462

Open

BenBE requested changes Apr 21, 2024

View reviewed changes

herrhotzenplotz force-pushed the freebsd-nice-improved branch from 7b8a4e3 to 3107a08 Compare April 27, 2024 09:00

Explorer09 reviewed Apr 27, 2024

View reviewed changes

freebsd/FreeBSDProcessTable.c Outdated Show resolved Hide resolved

Explorer09 reviewed Apr 27, 2024

View reviewed changes

freebsd/FreeBSDProcess.c Outdated Show resolved Hide resolved

Explorer09 reviewed Apr 27, 2024

View reviewed changes

herrhotzenplotz force-pushed the freebsd-nice-improved branch from 3107a08 to d045e1d Compare May 5, 2024 11:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix and improve NICE on FreeBSD #1461

Fix and improve NICE on FreeBSD #1461

herrhotzenplotz commented Apr 20, 2024

swills commented Apr 20, 2024

clausecker commented Apr 20, 2024

herrhotzenplotz commented Apr 20, 2024

clausecker commented Apr 20, 2024

BenBE left a comment

herrhotzenplotz commented Apr 20, 2024

BenBE commented Apr 20, 2024

clausecker commented Apr 20, 2024

BenBE commented Apr 20, 2024

herrhotzenplotz commented Apr 20, 2024

BenBE commented Apr 20, 2024

herrhotzenplotz commented Apr 21, 2024

Explorer09 Apr 21, 2024

herrhotzenplotz Apr 21, 2024

Explorer09 Apr 21, 2024

herrhotzenplotz Apr 21, 2024

Explorer09 Apr 21, 2024

herrhotzenplotz Apr 21, 2024

Explorer09 Apr 21, 2024

herrhotzenplotz Apr 21, 2024

BenBE Apr 21, 2024

herrhotzenplotz Apr 22, 2024

BenBE Apr 21, 2024

Explorer09 Apr 27, 2024

herrhotzenplotz Apr 27, 2024

BenBE Apr 27, 2024

herrhotzenplotz Apr 27, 2024

Explorer09 commented Apr 27, 2024

herrhotzenplotz commented Apr 27, 2024

Explorer09 commented Apr 27, 2024

herrhotzenplotz commented Apr 27, 2024

	# define _Static_assert(expr, msg) \
	extern int (*__Static_assert_function (void)) \
	[!!sizeof (struct { int __error_if_negative: (expr) ? 2 : -1; })]

Fix and improve NICE on FreeBSD #1461

Are you sure you want to change the base?

Fix and improve NICE on FreeBSD #1461

Conversation

herrhotzenplotz commented Apr 20, 2024

swills commented Apr 20, 2024

clausecker commented Apr 20, 2024

herrhotzenplotz commented Apr 20, 2024

clausecker commented Apr 20, 2024

BenBE left a comment

Choose a reason for hiding this comment

herrhotzenplotz commented Apr 20, 2024

BenBE commented Apr 20, 2024

clausecker commented Apr 20, 2024

BenBE commented Apr 20, 2024

herrhotzenplotz commented Apr 20, 2024

BenBE commented Apr 20, 2024

herrhotzenplotz commented Apr 21, 2024

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

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

Explorer09 commented Apr 27, 2024

herrhotzenplotz commented Apr 27, 2024

Explorer09 commented Apr 27, 2024

herrhotzenplotz commented Apr 27, 2024