Page MenuHomeFreeBSD

proc: implement pid hash locks and an iterator
ClosedPublic

Authored by mjg on Nov 2 2018, 2:03 PM.
Tags
None
Referenced Files
F81727436: D17817.diff
Sat, Apr 20, 1:42 PM
Unknown Object (File)
Thu, Apr 11, 7:27 PM
Unknown Object (File)
Thu, Apr 11, 2:47 PM
Unknown Object (File)
Tue, Apr 9, 9:59 PM
Unknown Object (File)
Sun, Apr 7, 2:43 AM
Unknown Object (File)
Sun, Mar 31, 8:16 PM
Unknown Object (File)
Sat, Mar 23, 1:30 AM
Unknown Object (File)
Sat, Mar 23, 1:30 AM
Subscribers

Details

Summary

poudriere -j 128 performs frequent killpg and process list export calls, both of which traverse the entire list with allproc lock held. This stalls forks, exits and waits as each of these operations takes the lock effectively giving very high contention on proctree lock which when held waits for allproc.

This change implements a very basic lock array for the pid hash and an iterator function which takes a callback. A 30 minute poudriere run has seen drop in proctree wait time to 5% of the previous kernel. Note the traversal still does not scale, but it stops being a bottleneck to the rest of the kernel.

Removal of zombie processes is postponed until reaping so that process list exporting can still see them.

pfind and other functions will be updated later to use the newly introduced locks.

Relevant locks before (30 minutes):

13411440062 (sx:proctree)
98113595 (sx:allproc)

after:

573040146 (sx:proctree)
268533 (sx:allproc)
211663 (sx:pidhash)

Commit will be split into 3 parts, to cover the mechanism itself and then the two users.

Later on the code can be modified to postpone insertion into the hash so that PRS_NEW checks can be removed from consumers.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

mjg edited the summary of this revision. (Show Details)
mjg added a reviewer: kib.
mjg set the repository for this revision to rS FreeBSD src repository - subversion.

Are you proposing to replace allproc_lock by the per-hash chain lock ? Then isn't it possible for killpg to miss both a process and its child, if the process forks and exits, and child is added to some other hash chain ?

sys/kern/kern_proc.c
1503 ↗(On Diff #49934)

{} are excessive, there and in all one-line bodies of the blocks below.

killpg is already unreliable. if the child is spotted as PRS_NEW it will be explicitly omitted, so I don't think this constitutes a regression in functionality

In D17817#381203, @mjg wrote:

killpg is already unreliable. if the child is spotted as PRS_NEW it will be explicitly omitted, so I don't think this constitutes a regression in functionality

Huh, Yes, and this is, I dare to say, a vulnerability. It might allow the user to leave a process behind when e.g. a session is terminated.

It should be fixed instead of adding the code which makes the bug unfixable.

I don't think the same problem is a concern for ps/top, so it can be discussed further in a different review. I can simply drop killpg conversion from the patchset (and remove the spurious curly braces).

Do you see any issues with the rest of the patch?

In D17817#381257, @mjg wrote:

I don't think the same problem is a concern for ps/top, so it can be discussed further in a different review. I can simply drop killpg conversion from the patchset (and remove the spurious curly braces).

Do you see any issues with the rest of the patch?

Ok.

sys/kern/kern_proc.c
1456 ↗(On Diff #49934)

int error, i, j;
alphabetical order

1458 ↗(On Diff #49934)

Is this line needed ?

1467 ↗(On Diff #49934)

error != 0

1468 ↗(On Diff #49934)

replace goto by { unlock; return (error);} ?

  • address feedback
  • drop killpg changes
kib added inline comments.
sys/kern/kern_proc.c
193 ↗(On Diff #50499)

The blank line is excessive.

This revision is now accepted and ready to land.Nov 21 2018, 5:47 PM
This revision was automatically updated to reflect the committed changes.