I like it. The only thing that could be better in CTASSERT that exactly one of MPSAFE or NEEDGIANT is set.

I disagree with this approach.

Instead I suggest the following:
0. on registration add a warning that given sysctl is not marked as mpsafe [optional]

1. keep converting existing sysctls and adding the CTLFLAG_MPSAFE flag. worst case giant can be pushed into specific handlers
2. once it seems everything is converted require the flag to be set for sysctl to be registered, special handling for giant is removed from sysctl itself
3. phase out the flag some time later. coccinelle can easily do it.

The advantage of this approach is that it lets us know what has been audited at compile time...

I don't see any benefit. "general" kernel does not use giant and everything there is almost guaranteed to be trivially annotated as _MPSAFE (just someone has to inspect all cases). the only fishy stuff may be in syscons or some drivers. If in doubt, giant handling can be pushed into these handlers. Basically almost everything inspected is instantly resolvable one way or the other.

Setting aside the issue of whether having a flag is a good idea, I'm not sure this is the right name. Really what we're trying to indicate after we get rid of Giant is whether these sysctls are reentrant or not (and need external mutual exclusion)... right? Maybe something like NEEDMUTEX rather than Giant in particular? /bikeshed

(As always, I might be totally mistaken. Let me know if I’m wrong and hopefully I learn something.)

Scenario where a sysctl ends up being called from another sysctl like this sounds like a programming error.

In D23110#506565, @mjg wrote:

Scenario where a sysctl ends up being called from another sysctl like this sounds like a programming error.

Reentrant was the wrong word. Threadsafe? Concurrent? Something like that.

I'm not going to die for a name, but! we still have D_NEEDGIANT and the name still describe what is being done in this case.

Also, based on my (and probably not only mine) experience, if there is no whining there is little incentive to work on boring fixes and this is what most of the work here will be.

In D23110#506566, @cem wrote:

In D23110#506565, @mjg wrote:

Scenario where a sysctl ends up being called from another sysctl like this sounds like a programming error.

Reentrant was the wrong word. Threadsafe? Concurrent? Something like that.

mpsafe sysctls already have to provide their own locking in this regard. the only guarantee provided by the sysctl mechanism is that the handler will not get unloaded while executed

In D23110#506569, @kaktus wrote:

Also, based on my (and probably not only mine) experience, if there is no whining there is little incentive to work on boring fixes and this is what most of the work here will be.

Every single case to be inspected by hand immediately gets CTLFLAG_MPSAFE. Either it did not need giant to begin with or (in the worst case) you can put giant locking into the handler. As such, the proposed flag does not add any value. It's just extra level of churn before the entire ordeal is removed.

In D23110#506563, @cem wrote:

Setting aside the issue of whether having a flag is a good idea, I'm not sure this is the right name. Really what we're trying to indicate after we get rid of Giant is whether these sysctls are reentrant or not (and need external mutual exclusion)... right? Maybe something like NEEDMUTEX rather than Giant in particular? /bikeshed

(As always, I might be totally mistaken. Let me know if I’m wrong and hopefully I learn something.)

Yea, but the NEEDGIANT name means a grep

In D23110#506589, @mjg wrote:

In D23110#506569, @kaktus wrote:

Also, based on my (and probably not only mine) experience, if there is no whining there is little incentive to work on boring fixes and this is what most of the work here will be.

Every single case to be inspected by hand immediately gets CTLFLAG_MPSAFE. Either it did not need giant to begin with or (in the worst case) you can put giant locking into the handler. As such, the proposed flag does not add any value. It's just extra level of churn before the entire ordeal is removed.

Except that assumes one person is doing the auditing. There will be several and the tag let's them all run asynchronously as well as the fix. Churn in the modern SCM doesn't matter. We have high rates of normal change, much of it changing things that have been changed since the last branch. Churn mattered on CVS, but doesn't, really, if done right.

In D23110#506612, @imp wrote:

In D23110#506589, @mjg wrote:

Every single case to be inspected by hand immediately gets CTLFLAG_MPSAFE. Either it did not need giant to begin with or (in the worst case) you can put giant locking into the handler. As such, the proposed flag does not add any value. It's just extra level of churn before the entire ordeal is removed.

Except that assumes one person is doing the auditing. There will be several and the tag let's them all run asynchronously as well as the fix. Churn in the modern SCM doesn't matter. We have high rates of normal change, much of it changing things that have been changed since the last branch. Churn mattered on CVS, but doesn't, really, if done right.

With the patch as proposed here people would have to look for CTLFLAG_NEEDGIANT sysctls. With stock kernel people have to look for sysctls *without* the CTLFLAG_MPSAFE flag. Both cases have the same coopoeration issues, just the method of figuring out whether given handler needs to be worked on is different.

In D23110#506613, @mjg wrote:

In D23110#506612, @imp wrote:

In D23110#506589, @mjg wrote:

Every single case to be inspected by hand immediately gets CTLFLAG_MPSAFE. Either it did not need giant to begin with or (in the worst case) you can put giant locking into the handler. As such, the proposed flag does not add any value. It's just extra level of churn before the entire ordeal is removed.

Except that assumes one person is doing the auditing. There will be several and the tag let's them all run asynchronously as well as the fix. Churn in the modern SCM doesn't matter. We have high rates of normal change, much of it changing things that have been changed since the last branch. Churn mattered on CVS, but doesn't, really, if done right.

With the patch as proposed here people would have to look for CTLFLAG_NEEDGIANT sysctls.

RIght. Another fruit of this work would be audit of all places which have this flag set, and handling of trivial cases where the flag can be removed.

So the result should be an material and easy to asses estimation of the efforts to clean sysctls from Giant.

With stock kernel people have to look for sysctls *without* the CTLFLAG_MPSAFE flag. Both cases have the same coopoeration issues, just the method of figuring out whether given handler needs to be worked on is different.

No, grepping for absence in multi-line vague text is not a solution. More, there is a lot of things to grep, e.g. dynamic sysctl oids allocation and so on. Also, I object against fixing cases of NEEDGIANT by pushing Giant into the handler, there is no point doing this. Handlers must be fixed to not require Giant, in the course of mpsafe work.

I do want sysctls be on par with cdevsw in this regard, where it is absolutely obvious when needs Giant and what does not.

But again, the main benefit of tht work would be the audit, and I was both surprised and pleased when I saw that kaktus started it.

Yes, many of the sysctls can be trivially be made / already are but aren't marked as MPSAFE but this was mostly sed + vim macros + a bit of manual fixes to add the flag where MPSAFE isn't set (yet).
The question is: should fixing this be done in this (already huge) diff or as a separate work later.

In D23110#506677, @kaktus wrote:

Yes, many of the sysctls can be trivially be made / already are but aren't marked as MPSAFE but this was mostly sed + vim macros + a bit of manual fixes to add the flag where MPSAFE isn't set (yet).
The question is: should fixing this be done in this (already huge) diff or as a separate work later.

Of course adding MPSAFE (or removing NEEDGIANT) should be done in the next patch. I listed some of them only to demonstrate how your work to add NEEDGIANT causes easy and straightforward fixes.

A question because I should update my tools.
If a node has not the CLT_MFSAFE flags then it has to have the CTL_NEEDGIANT, is it right?
(So I should add a new "flag column" to deskutils/sysctlview and handle/show the new flag with "nsysctl -aNG" sysutils/nsysctl)

Thank you in advance

In D23110#506672, @kib wrote:

In D23110#506613, @mjg wrote:

In D23110#506612, @imp wrote:

In D23110#506589, @mjg wrote:

Every single case to be inspected by hand immediately gets CTLFLAG_MPSAFE. Either it did not need giant to begin with or (in the worst case) you can put giant locking into the handler. As such, the proposed flag does not add any value. It's just extra level of churn before the entire ordeal is removed.

Except that assumes one person is doing the auditing. There will be several and the tag let's them all run asynchronously as well as the fix. Churn in the modern SCM doesn't matter. We have high rates of normal change, much of it changing things that have been changed since the last branch. Churn mattered on CVS, but doesn't, really, if done right.

With the patch as proposed here people would have to look for CTLFLAG_NEEDGIANT sysctls.

RIght. Another fruit of this work would be audit of all places which have this flag set, and handling of trivial cases where the flag can be removed.

So the result should be an material and easy to asses estimation of the efforts to clean sysctls from Giant.

But there is no need to patch the kernel to get that information. See below.

With stock kernel people have to look for sysctls *without* the CTLFLAG_MPSAFE flag. Both cases have the same coopoeration issues, just the method of figuring out whether given handler needs to be worked on is different.

No, grepping for absence in multi-line vague text is not a solution. More, there is a lot of things to grep, e.g. dynamic sysctl oids allocation and so on.

I don't know what kind of script kaktus used to generate his patch, I hacked up the following:

find . -name '*.[ch]' | xargs -P1 awk '/SYSCTL_PROC/ { ins = 1; } /SYSCTL_ADD_PROC/ { ins = 1; } ins { line = line $0; } /;/ && ins { ins = 0; print FILENAME " " line; line = "" }' | grep -v MPSAFE

Prints stuff like this:

./fs/nfsclient/nfs_clnfsiod.c SYSCTL_PROC(_vfs_nfs, OID_AUTO, iodmin, CTLTYPE_UINT | CTLFLAG_RW, 0, sizeof (nfs_iodmin), sysctl_iodmin, "IU", "Min number of nfsiod kthreads to keep as spares");
./fs/nfsclient/nfs_clnfsiod.c SYSCTL_PROC(_vfs_nfs, OID_AUTO, iodmax, CTLTYPE_UINT | CTLFLAG_RW, 0, sizeof (ncl_iodmax), sysctl_iodmax, "IU", "Max number of nfsiod kthreads");
./fs/nfsserver/nfs_nfsdport.c SYSCTL_PROC(_vfs_nfsd, OID_AUTO, dsdirsize, CTLTYPE_UINT | CTLFLAG_RW, 0, sizeof(nfsrv_dsdirsize), sysctl_dsdirsize, "IU", "Number of dsN subdirs on the DS servers");

It probably misses entries but should catch everything seen in this patch and more. Everyone interested in removing some of this stuff can just run it.

Once nothing in the tree is found like this one can add a CTASSERT the flag is set and a runtime check to panic during registration of sysctls which don't have it.

Also interested people can run dtrace to see what is taking giant at runtime on their boxes:

dtrace -n 'fbt::sysctl_root_handler_locked:entry /(args[0]->oid_kind & 0x00040000) == 0/ { @[stringof(args[0]->oid_name), args[0]->oid_descr[0] != 0 ? stringof(args[0]->oid_descr) : "NULL", sym((uintptr_t)args[0]->oid_handler)] = count(); }' -o out

sysctl -a > /dev/null

I cleaned up some immediate offenders.

Also, I object against fixing cases of NEEDGIANT by pushing Giant into the handler, there is no point doing this. Handlers must be fixed to not require Giant, in the course of mpsafe work.

I don't suggest just plopping Giant in the handler. Rather vast majority of cases will straight up just need the annotation added. I expect few problematic cases where Giant is actually used and I don't think they should be holding off removal of giant support from sysctl itself.

I do want sysctls be on par with cdevsw in this regard, where it is absolutely obvious when needs Giant and what does not.

But it is clear (but the condition is inverted). Instead of grepping for a flag one has to grep for lack of the flag, which as noted above is not a problem.

But again, the main benefit of tht work would be the audit, and I was both surprised and pleased when I saw that kaktus started it.

In D23110#506834, @alfix86_gmail.com wrote:

A question because I should update my tools.
If a node has not the CLT_MFSAFE flags then it has to have the CTL_NEEDGIANT, is it right?

Exactly.

@mjg You are completely missing the point I think. grep -r CTLFLAG_NEEDGIANT is much easier for many folks to run (and remember, and derive off the top of their head) than the script you came up with. FWIW, I would like to use a similar approach with bus_setup_intr() to tag the ones that still need giant explicitly to make them easier to find. It is true that when finishing marking syscalls, I did not add a Giant flag but just converted them all over, but that was a much smaller set of things to examine. I'd much rather us at this point move to a model where we tag things as needing Giant rather than not needing Giant.

Sorry, I'm lost. What is the purpose of this flag?

If it has a real value for the code path (define a specific functionaliy, which is not already present), I'm fine (modulo naming discussions).

If it is only annotating something for a group of deverlopers, I'd suggest an inline comment in all the files.
If the group of interested developers is not significant, I'd suggest a separate todo-file somewhere in the development area containing all the syscalls in question.

In D23110#508042, @jhb wrote:

@mjg You are completely missing the point I think. grep -r CTLFLAG_NEEDGIANT is much easier for many folks to run (and remember, and derive off the top of their head) than the script you came up with.

I'm pretty sure I'm not missing the point. Giant-locked handlers are still in the tree mostly because there was no to push to take care of them. Given someone's idea to remove Giant before 13.0 ships now there is a push to do it. The very same people who would have to be told to grep for CTLFLAG_NEEDGIANT can be told to run the one-liner I provided above. I really don't see how this is supposed to be more problematic. Presumably there would be an e-mail urging people to clear this up, then they can easily reference it if needed. Or if they use the one-liner they will probably easily find it later in their shell history.

In reality I suspect a small group of people will take care of majority of all handlers, 3-4 people will fix some instances on their own volition and the rest will have to be chased down to either remove an obscure driver or ungiantify it (along with a diff which pushes giant into the handler).

FWIW, I would like to use a similar approach with bus_setup_intr() to tag the ones that still need giant explicitly to make them easier to find.

I have no opinion about that code.

For sysctls, vast majority of handlers is already MPSAFE but not yet annotated as such and this is why I think the new flag does not add anything.

In D23110#508108, @lutz_donnerhacke.de wrote:

If it is only annotating something for a group of deverlopers, I'd suggest an inline comment in all the files.

It's hard to assert on a comment line.

Because the write path in sysctl_handle_string() and sysctl_handle_opaque() isn't locked yet?

In D23110#508324, @kaktus wrote:

Because the write path in sysctl_handle_string() and sysctl_handle_opaque() isn't locked yet?

Are you sure that it makes any sense/difference ? Note that sysctl_new_in() is just copyin, so kernel fetches something user-controllable directly into the location.

Anyway, if you consider it is useful to add a locking there, do it in this patch, please.

D23110#506589, @mjg wrote:
Also interested people can run dtrace to see what is taking giant at runtime on their boxes:

dtrace -n 'fbt::sysctl_root_handler_locked:entry /(args[0]->oid_kind & 0x00040000) == 0/ { @[stringof(args[0]->oid_name), args[0]->oid_descr[0] != 0 ? stringof(args[0]->oid_descr) : "NULL", sym((uintptr_t)args[0]->oid_handler)] = count(); }' -o out

sysctl -a > /dev/null

just a tip, easier and more precise because "sysctl -a" gets the "next leaf" while "nsysctl -aI" gets the "next leaf or next internal node":

nsysctl -aNGI | /usr/bin/grep -v MPSAFE

I want this to be split into three patches:

1. sysctl infra: CTLFLAG_NEEDGIANT, related code, locking for strings, and empty definition of SYSCTL_ENFORCE_FLAGS()
2. addition of NEEDGIANT for everything that should be marked as such
3. proper definition of SYSCTL_ENFORCE_FLAGS

We can commit 1 shortly. Part 2 can be committed by pieces, e.g. after review of maintainers of touched subsystem if you feel the need of the review. Then finally we will turn the switch by 3, some time later.

OK, I'll cut it into smaller batches.