bin/ps: Make the rtprio option actually show realtime priorities

[salvadore]

Summary

Fix the rtprio option that for some reason was progessively becoming an option showing the priority class of threads. In particular:

• use the constants defined in sys/sys/rtprio.h instead of those defined in sys/sys/priority.h: this helps making clearer that the code actually is about realtime priorities and not standard scheduler priorities;
• remove the PRI_ITHD case that has nothing to do with realtime priorities;
• convert the priority levels to realtime priority levels using the same formulas used for pri_to_rtp function in sys/kern/kern_resource.c.
• remove outdated note "101 = not a realtime process" in the man page and replace it with a more useful reference to man 1 rtprio.

[salvadore]

@mckusick: As you can see the patch I am submitting is different from what we discussed by mail. As suggested in the summary of the review, this is due to the fact that I think realtime priorities are a different concept than standard scheduler priorities: looking at man 1 rtprio, at sys/sys/rtprio.h and at sys/kern/kern_resource.c, I think realtime priorities work as follow.

Real time priorities are defined only for threads in userspace. If a user thread is in TIMESHARED class, then it has a 'normal' realtime priority, meaning it is not an idle or realtime thread, that user application did not specify anything about priority. Then,

• using for example rtprio, a user application can change the realtime priority type of a thread to 'REALTIME' and set its realtime priority level, which will then be added to PRI_MIN_REALTIME to get the standard priority;
• using for example idprio, a user application can change the realtime priorty type of a thread to 'IDLE' and set its realtime priority level (aka know as idle priority level in this case), which will then be added to PRI_MIN_IDLE to get the standard priority.

I hope all of this makes sense.

I have choosed to keep not printing priority levels for normal threads as they are not modifiable by the user using rtprio/idprio. The priority for such threads can of course still be printed using -o pri.

[bcr]

head/bin/ps/ps.1

651–652

I'd use a cross-reference here: https://mandoc.bsd.lv/man/mdoc.7.html#Xr_2

[salvadore]

Thanks bcr, I have put the cross-reference as you suggested.

[bcr]

Great, thank you!

[mckusick]

This looks like a big improvement. My one suggestion is that you add the priority value for normal (as level - PRI_MIN_TIMESHARE).

[salvadore]

I added the level for the normal case as suggested. Thanks mckusick for your feedback!

[mckusick]

One other suggestion is to change "normal" to "timeshare" or perhaps "timeshr" as that name is more descriptive of what is being reported.

[salvadore]

In D25266#558504, @mckusick wrote:

One other suggestion is to change "normal" to "timeshare" or perhaps "timeshr" as that name is more descriptive of what is being reported.

I think it is better to keep "normal", at least for now. It seems to me that this keyword has a consolidate meaning in the realtime priority context (although it does coincide with the timeshare priority class in standard priority context) and it is already used in man 1 rtprio and, most importantly, in the RTP_PRIO_NORMAL constant. If we want to change 'normal' to 'timeshare' or 'timeshr', then I think we should also change RTP_PRIO_NORMAL to RTP_PRIO_TIMESHARE and this would require changing all the following files where RTP_PRIO_NORMAL is present:

bin/ps/print.c (I introduced it)
usr.sbin/rtprio/rtprio.1
usr.sbin/rtprio/rtprio.c
tools/regression/priv/priv_sched_rtprio.c
lib/libc/sys/rtprio.2
lib/libthr/thread/thr_kern.c
sys/kern/ksched.c
sys/kern/kern_thr.c
sys/kern/kern_resource.c
sys/sys/rtprio.h
contrib/ntp/ntpd/ntpd.c

However I think it is better to keep the RTP_PRIO_NORMAL name and thus the "normal" keyword in the output of ps: it helps making clear that realtime priorities are not the same thing than standard priorities, although they are closely related.

[mckusick]

Your argument has pursuaded me. Looks good to go.

[salvadore]

@mckusick: Thanks! Does that count as a src committer approval? I ask because as ports committer, according to the committer guide, I need a src committer approval to commit to the base tree, but you are not listed in share/misc/committers-src.dot.

I will also have to wait for a mentor approval from gerald or tcberner: as a ports committer, I am still mentored.

@gerald, @tcberner: Can I assume for this review and for future reviews on the base tree that whenever I get a src committer approval on the base tree I automatically get a mentor approval, just as we already do on the doc tree?

[salvadore]

Also, can someone please point me to some documentation about MFC on the base tree? I can't find any which is complete enough.

In particular, I think it is safe to set MFC after: 1 week for this patch, isn't it? I don't break compatibility, most recent commits I saw set MFC after: 1 week and I don't think this patch requires more.

[mckusick]

Interesting that I never added myself to the committers-src.dot file.
For reference, the file that actually controls access to doing src commits is /usr/src/svnadmin/conf/access.
For interesting statistics on the history of commits, fetch these files:

fetch https://people.freebsd.org/~rodrigo/commits-base.txt
fetch https://people.freebsd.org/~rodrigo/commits-doc.txt
fetch https://people.freebsd.org/~rodrigo/commits-ports.txt

I agree that there should be documentation of MFC guidelines somewhere.
For a change of this sort an MFC delay in the range of 1-2 weeks is reasonable.

[bcr]

In D25266#559123, @salvadore wrote:

Also, can someone please point me to some documentation about MFC on the base tree? I can't find any which is complete enough.

In particular, I think it is safe to set MFC after: 1 week for this patch, isn't it? I don't break compatibility, most recent commits I saw set MFC after: 1 week and I don't think this patch requires more.

For the technical parts of doing an MFC, refer to section 5.4.3. Merging with SVN in the Committers Guide.

[gerald]

In D25266#559092, @salvadore wrote:

@gerald, @tcberner: Can I assume for this review and for future reviews on the base tree that whenever I get a src committer approval on the base tree I automatically get a mentor approval

Yes.

Just make sure, please, that the src committer knows you come from the ports side. (When I approve changes from src committers on the ports side, I tend to watch out for details more carefully and provide some extra support and guidance wrt. commit message and other practices specific to ports, for example.)

[salvadore]

@mckusick, @bcr: Thanks for the pointers! At the end I set MFC after: 2 weeks, choosing the maximal value suggested by mckusick. It's true that it's an easy patch, but it's also true that nothing was broken, so there is no urgency.

@gerald: Thanks! I'll make sure src committers always know I am a ports committer, don't worry.