Page MenuHomeFreeBSD

devd: Avoid setting devctl_queue when stopping devd
AcceptedPublic

Authored by markj on Fri, Jan 10, 8:37 PM.

Details

Reviewers
imp
emaste
Summary

Right now, setting devctl_queue=0 destroys the devctl UMA zone, which
can't be recreated. Avoid that when stopping devd, since this prevents
devd from being started again without a reboot.

PR: 283708

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 61660
Build 58544: arc lint + arc unit

Event Timeline

markj requested review of this revision.Fri, Jan 10, 8:37 PM
markj created this revision.
This revision is now accepted and ready to land.Fri, Jan 10, 9:04 PM

How should we keep track of the need for a proper kernel fix?

I'd strongly prefer we just don't do this and fix the kernel not delete the una zone

diff --git a/sys/kern/kern_devctl.c b/sys/kern/kern_devctl.c
index d83bc380c2fe..6df120792b08 100644
--- a/sys/kern/kern_devctl.c
+++ b/sys/kern/kern_devctl.c
@@ -560,18 +560,20 @@ sysctl_devctl_queue(SYSCTL_HANDLER_ARGS)
        }

        /*
-        * XXX It's hard to grow or shrink the UMA zone. Only allow
-        * disabling the queue size for the moment until underlying
-        * UMA issues can be sorted out.
+        * At runtime, though, it's hard to grow or shrink the UMA
+        * zone. Only allow toggling between zero and the initially
+        * configured size. The queue size itself isn't settable at
+        * runtime. If we were disabled at boot, we can't change
+        * anything.
         */
-       if (q != 0)
+       if (devsoftc.zone == NULL)
+               return (EINVAL);
+       if (q != 0 && q != uma_zone_get_max(devsoftc.zone))
                return (EINVAL);
        if (q == devctl_queue_length)
                return (0);
        mtx_lock(&devsoftc.mtx);
-       devctl_queue_length = 0;
-       uma_zdestroy(devsoftc.zone);
-       devsoftc.zone = 0;
+       devctl_queue_length = q;
        mtx_unlock(&devsoftc.mtx);
        return (0);
 }

I think will do the trick. I've only lightly tested it though.

libexec/rc/rc.d/devd
38

So even with the kernel fix, I'm thinking this outlived its usefulness.

I think we need to fix the documentation to reflect the move to UMA.

devd_disable will not run devd, but allows for it to be turned on at the cost of memory.

But to never run devd, and save the memory, you have to set hw.bus.devctl_queue=0 as a tuneable.

I'm torn, though. When devd isn't running, we do want to avoid all the work of generating the messages to send via devd. Though honestly, we still do most of it even when disabled. Ideally we'd somehow avoid it all.

And since we preallocate, it doesn't matter if we keep it around in the kernel or not: we use the same amount of memory. So we'd save a little work to set this when devd isn't running. And setting it to 0 is a one-way trip now. So maybe we just want to delete the devd_offcmd and devd_off entirely.

Though we'd have to document this a lot better. devd(8) doesn't offer this sysctl as a tuneable. And if it's set to zero, maybe devd_enable should be no? Or maybe we should whine about the error. I'm unsure which is better: loud complaints for something that's easy to ignore, or too much DWIM causing devd to fail silently.