Enable iterating all sysctls, even ones with CTLFLAG_SKIP
ClosedPublic
Actions

Authored by freqlabs on Sep 25 2020, 6:33 PM.

Details

Reviewers

jhb

Commits

rS366465: Enable iterating all sysctls, even ones with CTLFLAG_SKIP

Summary

Add an "nextnoskip" sysctl that allows for listing of sysctls intended to be normally skipped for cost reasons.

This makes it so the names/descriptions of those sysctls can be discovered with sysctl -aN/sysctl -ad/sysctl -at.

It also makes it so children are visited when a node flagged with CTLFLAG_SKIP is explicitly requested.

The intended use case is to mark the root "kstat" node with CTLFLAG_SKIP so that the extensive and expensive stats are skipped by default but may still be easily obtained without having to know them all (which may not even be possible) and request each one-by-one.

Test Plan

With the appropriate change in ZFS to give the root kstat node CTLFLAG_SKIP:

➜  ~ sysctl -a | grep kstat
➜  ~ sysctl -aN | grep kstat | wc -l
     688
➜  ~ sysctl -ad | grep kstat | wc -l
     688
➜  ~ sysctl -at | grep kstat | wc -l
     688
➜  ~ sysctl -N kstat | wc -l
     688
➜  ~ sysctl -d kstat | wc -l
     689
➜  ~ sysctl kstat | wc -l
   13590

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

freqlabs created this revision.Sep 25 2020, 6:33 PM

Herald added a subscriber: imp. · View Herald TranscriptSep 25 2020, 6:33 PM

freqlabs requested review of this revision.Sep 25 2020, 6:33 PM

freqlabs added inline comments.Sep 25 2020, 6:36 PM

sys/kern/kern_sysctl.c
2736	I think this could be useful, but I'm not sure it's actually a good idea

freqlabs added inline comments.Sep 25 2020, 6:38 PM

sbin/sysctl/sysctl.c
1205	Maybe tflag too?

asiciliano added a subscriber: asiciliano.Sep 25 2020, 6:51 PM

freqlabs edited the test plan for this revision. (Show Details)Sep 25 2020, 6:52 PM

Also visit all with tflag

freqlabs edited the test plan for this revision. (Show Details)Sep 25 2020, 7:01 PM

jhb added a subscriber: np.Sep 25 2020, 8:21 PM

jhb added inline comments.Sep 25 2020, 9:28 PM

sbin/sysctl/sysctl.c
1205	So the `oid` thing here might have an unusual effect. Specifically, suppose I have the following nodes: foo (CTLFLAG_SKIP) foo.bar foo.bar.baz (CTLFLAG_SKIP) I think your change might make it so that 'sysctl foo' would still do `foo.bar.baz` whereas I what we want is that 'sysctl foo' would ignore the SKIP flag on 'foo' but still honor it for any descendants. (It should also be `oid != NULL`).
sys/kern/kern_sysctl.c
1179	You could perhaps reduce some code duplication by replacing `true` with `oidp->oid_number == CTL_SYSCTL_ALLNEXT` and then reusing this handler function for the ALLNEXT oid.
1219	Could possibly call this "next_noskip" rather than "allnext", but "allnext" is probably fine.
2736	I would probably not make this change.

kaktus added a subscriber: kaktus.Sep 25 2020, 11:15 PM

Addressed some feedback.

This will now skip descendent nodes marked CTLFLAG_SKIP, but not their children :/
Will have to think some more about that.

freqlabs edited the summary of this revision. (Show Details)Sep 29 2020, 7:57 PM

freqlabs marked 2 inline comments as done.

Thanks I think this looks good to me.

This revision is now accepted and ready to land.Sep 30 2020, 8:06 PM

Looks like I'm going to need to add this to py-sysctl, too. It's using sysctl(3) so won't benefit from the fixed sysctl(8). A few of the Python-based tools in OpenZFS use py-sysctl on FreeBSD to get at kstat sysctls, which are exactly what I have planned to SKIP.

I intend to commit this once https://github.com/william-gr/py-sysctl/pull/8 has merged and tagged and the port has been updated.

freqlabs mentioned this in D26684: devel/py-sysctl: Update to 0.3.0.Oct 5 2020, 5:56 PM

freqlabs mentioned this in rP551534: devel/py-sysctl: Update to 0.3.0.Oct 5 2020, 6:22 PM

Closed by commit rS366465: Enable iterating all sysctls, even ones with CTLFLAG_SKIP (authored by freqlabs). · Explain WhyOct 5 2020, 8:13 PM

This revision was automatically updated to reflect the committed changes.

freqlabs added a commit: rS366465: Enable iterating all sysctls, even ones with CTLFLAG_SKIP.