Fix and improve NICE on FreeBSD

[herrhotzenplotz]

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]

Here's a before and after screenshot

[clausecker]

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]

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]

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.

[herrhotzenplotz]

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.

[BenBE]

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]

@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]

@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]

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]

@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.

[herrhotzenplotz]

Example - now with a SCHEDCLASS_ enum.

[Explorer09]

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]

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]

@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)?
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.

[herrhotzenplotz]

@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:

• rtprio(2)
• (get|set)priority(2)

[herrhotzenplotz]

@BenBE @Explorer09 any news on this?

[BenBE]

@BenBE @Explorer09 any news on this?

There are still two code comments open and need fixing. The first one is about the static assert and the table indices, which should be handled similar to how it is done in other places in the htop code; second there's the handling of niceness values and sorting AFAICS. Lastly, there's the issue of rebasing the code to be up-to-date with the latest main branch. Once these have been resolved, leave a quick comment and I'll take a closer look at merging. As I read the discussions at the moment, there's still some aspects open. Not sure if @Explorer09 has further notes to add.