Page MenuHomeFreeBSD

sysctl_kern_proc_umask: fast path when operating on curproc
ClosedPublic

Authored by kaktus on Nov 6 2017, 1:45 PM.

Details

Summary

Make sysctl_kern_proc_umask execute fast path when requested pid in curproc->p_pid or 0, avoiding unnecessary locking. Update libc consumer to skip calling getpid().

Sponsored by: Mysterious Code Ltd.

Test Plan

rebuild and reinstall kernel and libc, run program using KERN_PROC_UMASK oid, ie. procstat -s pid, observe UMASK field.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

kaktus created this revision.Nov 6 2017, 1:45 PM
kaktus edited the summary of this revision. (Show Details)Nov 6 2017, 2:01 PM
robak added a reviewer: kib.Nov 6 2017, 2:02 PM
robak added a comment.Nov 6 2017, 2:19 PM

Mind adding testing instructions?

kaktus edited the test plan for this revision. (Show Details)Nov 6 2017, 2:22 PM
mjg requested changes to this revision.Nov 6 2017, 5:44 PM

For consistency this should also remove SLOCK/SUNLOCK in the lookup case, otherwise LGTM.

This revision now requires changes to proceed.Nov 6 2017, 5:44 PM
robak added a comment.Nov 6 2017, 7:16 PM
In D12972#269385, @mjg wrote:

For consistency this should also remove SLOCK/SUNLOCK in the lookup case, otherwise LGTM.

In such case I'm stopping the tests (large poudriere builds) until fixed version is submitted.

kaktus updated this revision to Diff 34864.Nov 6 2017, 7:41 PM

Address @mjg comments and remove extra FILEDESC locks.

kaktus edited the test plan for this revision. (Show Details)Nov 7 2017, 11:27 AM
robak accepted this revision.Nov 7 2017, 11:45 AM

The updated version seems to survive large Poudriere builds and works as described in test instructions. Waiting for @mjg or @kib to approve it before committing.

mjg accepted this revision.Nov 7 2017, 12:00 PM
This revision is now accepted and ready to land.Nov 7 2017, 12:00 PM
robak closed this revision.Nov 7 2017, 3:18 PM

Committed in r325516.

cem added a subscriber: cem.Nov 7 2017, 5:53 PM
cem added inline comments.
sys/kern/kern_proc.c
2781–2783

What is the justification for removing this locking? Mjg comment above says "for consistency," but I'm not sure with what. Thanks!

kaktus added inline comments.Nov 7 2017, 9:17 PM
sys/kern/kern_proc.c
2781–2783

To consistency with the new fast path and other places in kernel that read fd_cmask without acquiring the lock. fd_cmask is declared as short, hence in small enough to guarantee atomic read.

cem added inline comments.Nov 7 2017, 9:34 PM
sys/kern/kern_proc.c
2781–2783

Thanks!