Page MenuHomeFreeBSD

Provide a wrapper structure around ucred
Needs ReviewPublic

Authored by mjg on Dec 14 2019, 2:48 AM.

Details

Reviewers
kib
jeff
jhb
Summary

struct ucred gets refed and unrefed all the time. falloc_install during buildkernel accounts for 1740339 calls, almost all of which use the same creds.

Reduce ping pong by providing a wrapper structure allocated on fork which can be referenced by threads in given process. This opens up reduction of the refing cost in the vm as well.

Note the current code contains an avoidable ref on td_ucred/p_ucred which can be eliminated later.

Consumers were mostly patched by coccinelle.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 28147

Event Timeline

mjg created this revision.Dec 14 2019, 2:48 AM
mjg edited the summary of this revision. (Show Details)Dec 14 2019, 2:49 AM
jeff added a reviewer: jhb.Dec 14 2019, 2:51 AM

What are the chances you'd be willing to change the swap/vm object creds to use this as well?

mjg added a comment.Dec 14 2019, 3:09 AM

That code needs a clean up I'm not up for. In particular it keeps passing cred around as arguments even though it's almost always from curthread. I think someone familiar with the area should sit down with this.

kib added a comment.Dec 14 2019, 4:36 PM

Can you explain how fork works WRT p_credwrap ?

sys/kern/init_main.c
534

And where is the newcredwrap value used ?

mjg added a comment.Dec 14 2019, 9:42 PM
In D22811#499523, @kib wrote:

Can you explain how fork works WRT p_credwrap ?

Anything installing credentials (including initial ones on fork) allocates a new credwrap. The point is to allow multiple credential changes while allowing all credwraps to get garbage collected when the time comes with standard refcounting.

sys/kern/init_main.c
534

that's my bad. it should be in proc0 of course. it happens to be harmless since this kernel threads don't use the file descriptor table.

kib added a comment.Dec 16 2019, 4:57 PM
In D22811#499597, @mjg wrote:
In D22811#499523, @kib wrote:

Can you explain how fork works WRT p_credwrap ?

Anything installing credentials (including initial ones on fork) allocates a new credwrap. The point is to allow multiple credential changes while allowing all credwraps to get garbage collected when the time comes with standard refcounting.

But where does this happen in kern_fork.c ? Specifically, which line/call ?

mjg added a comment.Dec 17 2019, 5:25 AM

That's proc_set_cred_init

kib added a comment.Tue, Jan 7, 1:04 PM

It seems that you patched only one place to use the wrapper for real (files), all other places have to maintain two counters. Do you intend to finish this work so that p_ucred only referenced on fork and dereferenced on exit ? If not, this should be regularized anyway with clear written rules for preference of using p/td_ucred vs. credwrap. I think that 'finished' form should eliminate p_ucred at all.

sys/kern/kern_prot.c
1965

This is somewhat beyond even the 'just a hack' level. Please use sizeof(struct credwrap) there, if needed, extend the structure so tail fills the whole cache line.

sys/sys/file.h
208

Just curious, the additional indirection for f_cred access does not have a visible cost ?