Page MenuHomeFreeBSD

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

Authored by salvadore on Jun 14 2020, 9:51 AM.

Details

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.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

@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 added inline comments.
head/bin/ps/ps.1
651–652

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

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

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

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

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

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.

Your argument has pursuaded me. Looks good to go.

This revision is now accepted and ready to land.Jun 18 2020, 10:04 PM

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

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.

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.

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

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