Details

Reviewers

oshogbo
allanjude
kib
markj
zlei

Commits

rG1b8d70b2eb71: kern: Introduce kern.pid_max_limit sysctl.

Summary

Obtain maximum PID from kern.pid_max sysctl.
Use kern.pid_max and hw.ncpu to calculate column widths in top(1).
Always expect kern.pid_max to exists, instead of using hardcoded PID maximum value.

With those changes the kernel can be compiled with a higher PID_MAX value, eg. 999999.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

pjd created this revision.Dec 16 2023, 2:10 AM

Herald added a subscriber: imp. · View Herald TranscriptDec 16 2023, 2:10 AM

pjd requested review of this revision.Dec 16 2023, 2:10 AM

pjd updated this revision to Diff 131484.Dec 16 2023, 5:34 AM

pjd retitled this revision from Don't use hardcoded PID_MAX value. to Don't hardcode maximum PID..

pjd edited the summary of this revision. (Show Details)

If you manually upload diffs, please generate them with something like git diff -U999999 <whatever> so that the full content is available.

I think most of the changes are fine except I do not quite like the top modifications. Imagine that you just booted the machine, then all pids are short (4 digits). After some time, the display flips because you get the first 5-digid pid. Then again, depending on the display filtering criteria, pid column width would oscillate.

For NO_PID, also look at usr.sbin/bsnmpd/modules/snmp_hostres/hostres_swrun_tbl.c and contrib/sendmail/src/sendmail.h (later is probably fine).

In D43077#982024, @kib wrote:

If you manually upload diffs, please generate them with something like git diff -U999999 <whatever> so that the full content is available.

Will do.

I think most of the changes are fine except I do not quite like the top modifications. Imagine that you just booted the machine, then all pids are short (4 digits). After some time, the display flips because you get the first 5-digid pid. Then again, depending on the display filtering criteria, pid column width would oscillate.

No, the PID column width won't change, because we look at the maximum PID, which is 99999, so it will be 5 digits all the time.

I should have include the motivation for this change. The idea here is to be able to compile the kernel with a higher PID_MAX, eg. 999999 and have userland adjust properly without recompiling.

I have a system with a high number of processes (~15000) and have a lot of lock contention on allproc_lock, etc. I was hoping I might be able to reduce contention if I increase PID name space by an order of magnitude, so it will be easier to find a free PID. I don't think it helped, but along the way I discovered some userland tools that assume PID can't be higher than 99999.

Remove hardcoded PID_MAX from sched_{g,s}etaffinity().
Remove hardcoded NO_PID from bsnmpd.

Harbormaster completed remote builds in B54973: Diff 131486.Dec 16 2023, 7:41 AM

Add missing include.

Harbormaster completed remote builds in B54974: Diff 131487.Dec 16 2023, 7:48 AM

In D43077#982025, @kib wrote:

For NO_PID, also look at usr.sbin/bsnmpd/modules/snmp_hostres/hostres_swrun_tbl.c and contrib/sendmail/src/sendmail.h (later is probably fine).

I've added changes to bsnmpd and sched_{get,set}affinity(). sendmail defines NO_PID as 0, so this is unrelated to my changes.

patmaddox added a subscriber: patmaddox.Dec 16 2023, 12:28 PM

pjd edited the summary of this revision. (Show Details)Dec 16 2023, 11:54 PM

kib accepted this revision.Dec 17 2023, 11:51 AM

This revision is now accepted and ready to land.Dec 17 2023, 11:51 AM

zlei requested changes to this revision.Dec 17 2023, 3:12 PM

zlei added a subscriber: zlei.

zlei added inline comments.

bin/pkill/pkill.c
837 ↗	(On Diff #131487)	Currently MIB `kern.pid_max` is writable tunable. In case it got decreased , i.e. # sysctl kern.pid_max=1000 kern.pid_max:99999 -> 1000 Then pkill may no longer be able to kill previously spawned progress, say pid 2000. The MIB `kern.pid_max` should only affect the process of forking new progress IIUC.
884 ↗	(On Diff #131487)	Then pkill may no longer be able to kill previously spawned progress, say pid 2000. I'm not quite sure but it seems the right way should ignore the `MAX_PID` or `pidmax()`.

This revision now requires changes to proceed.Dec 17 2023, 3:12 PM

kib added inline comments.Dec 17 2023, 5:06 PM

bin/pkill/pkill.c
884 ↗	(On Diff #131487)	Problem is that pids > MAX_PID are tids.

zlei added inline comments.Dec 18 2023, 2:11 AM

bin/pkill/pkill.c

884 ↗

(On Diff #131487)

Problem is that pids > MAX_PID are tids.

You are right.
The maximum possible pid (the kernel support) is hardcoded to MAX_PID.

See sys/sys/proc.h:

/*
 * We use process IDs <= pid_max <= PID_MAX; PID_MAX + 1 must also fit
 * in a pid_t, as it is used to represent "no process group".
 */
#define PID_MAX         99999
#define NO_PID          100000
#define THREAD0_TID     NO_PID
extern pid_t pid_max;

And,

# sysctl kern.pid_max=100000
kern.pid_max: 99999
sysctl: kern.pid_max=100000: Invalid argument

I'm not quite sure but it seems the right way should ignore the MAX_PID or pidmax().

is wrong.

Then we actually want a readonly MIB for the compile macro PID_MAX.

SYSCTL_INT(kern, OID_AUTO, compile_pid_max, CTLFLAG_RD,
    SYSCTL_NULL_INT_PTR, PID_MAX, "Compile maximum allowed pid");

and then userland read it instead of kern.pid_max.

markj accepted this revision.Dec 18 2023, 3:00 PM

markj added inline comments.

usr.sbin/bsnmpd/modules/snmp_hostres/hostres_swrun_tbl.c
434 ↗	(On Diff #131487)	Fix the whitespace while you're here?
559 ↗	(On Diff #131487)	At least write an error message here? Looks like other code in this file (also in the caller) uses syslog(3) for such things.

kib added inline comments.Dec 18 2023, 3:21 PM

bin/pkill/pkill.c
884 ↗	(On Diff #131487)	This may be in fact the right solution, and it is quite good for the problem at hands.

pjd marked 2 inline comments as done.Dec 18 2023, 9:41 PM

pjd added inline comments.

bin/pkill/pkill.c
837 ↗	(On Diff #131487)	Good catch. I wonder if there is a real value in having this writable, instead of just read-only tunable. @kib I think you have introduced kern.pid_max. What was the use case for it? IIRC one use case was to be able to run FreeBSD 1.x jails under newer kernel where FreeBSD 1.x had a lower PID_MAX? Still, adding read-only sysctl might be the best choice here.
884 ↗	(On Diff #131487)	Not checking if the PID in pidfile is greater than MAX_PID in pkill is IMHO best solution or am I missing something? Even if we have value greater than MAX_PID in the pidfile, kill(2) only operates on PIDs and not on TIDs. Even if we introduce kern.compile_pid_max or similar I still think it would be best to just remove this condition.

pjd added inline comments.Dec 18 2023, 10:16 PM

lib/libc/gen/sched_setaffinity.c
47 ↗	(On Diff #131487)	As I understand this check is here to allow to use newer libc with an older kernel. If we decide to add new sysctl we are going to need a new P_OSREL_ macro and check it here, right?

Remove MAX_PID check from pkill.

Harbormaster completed remote builds in B54999: Diff 131544.Dec 19 2023, 2:37 AM

I believe that zlei@ note should be handled globally for this patch. The width of the pid field for ps/top/anything else should be determined by the number of decimal digits in the value of the new sysctl.

lib/libc/gen/sched_setaffinity.c
47 ↗	(On Diff #131487)	Not necessarily, simply doing the new sysctl and falling back to hard-coded value is enough.

Introduce kern.pid_max_limit.

Harbormaster completed remote builds in B55082: Diff 131717.Dec 22 2023, 6:59 AM

zlei added inline comments.Dec 25 2023, 4:05 AM

sys/kern/kern_mib.c
757	For the kernel part, I'd support if you would like to commit them separately. Those can be also MFCed easily.

pjd added inline comments.Dec 26 2023, 12:15 AM

sys/kern/kern_mib.c
757	I can do that, that's not a problem.

pjd added inline comments.Dec 28 2023, 7:44 AM

sys/kern/kern_mib.c
757	@zlei If you are ok with the change if it will be done in two commits, can you accept PR?

kib added inline comments.Dec 28 2023, 9:00 AM

sys/kern/kern_mib.c
757	Please go ahead with the kernel change. Consider it 'reviewed by kib' if needed. The libc change should be put into the next review.

oshogbo accepted this revision.Dec 28 2023, 9:59 AM

zlei accepted this revision.Dec 29 2023, 4:03 AM

zlei added inline comments.

sys/kern/kern_mib.c
757	@zlei If you are ok with the change if it will be done in two commits, can you accept PR? @pjd The change to kernel part looks good to me. Just go ahead.

This revision is now accepted and ready to land.Dec 29 2023, 4:03 AM

Closed by commit rG1b8d70b2eb71: kern: Introduce kern.pid_max_limit sysctl. (authored by pjd). · Explain WhyDec 30 2023, 6:20 AM

This revision was automatically updated to reflect the committed changes.

pjd added a commit: rG1b8d70b2eb71: kern: Introduce kern.pid_max_limit sysctl..

Don't hardcode maximum PID.
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 132004

sys/kern/kern_mib.c

sys/sys/proc.h

Don't hardcode maximum PID.ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 132004

sys/kern/kern_mib.c

sys/sys/proc.h

Don't hardcode maximum PID.
ClosedPublic
Actions

Revision Contents
Changeset List