kern/intr: replace mutex with shared-exclusive lock

[ehem_freebsd_m5p.com]

Summary

Reading event names doesn't require exclusive access. Shared-exclusive
locks can also be held while sleeping.

[mjg]

Is this called enough to warrant locking change?

More importantly though, if the change is to be made, it should be to use rwlocks. The current code does not have unbounded sleeps, but conversion to sx would make witness possibly complain about it to consumers.

[ehem_freebsd_m5p.com]

What is here doesn't warrant the locking change. Notice though at D36610 the lock needs to be held (shared) when passing data to userspace (which can sleep). The three places the lock is needed shouldn't be used in critical sections (intr_lookup() is very slow).

[ehem_freebsd_m5p.com]

Worthy of noting sys/x86/x86/intr_machdep.c also uses a shared-exclusive lock in a similar location in a similar way (hw.intrs sysctl). That seems to suggest a shared-exclusive lock is quite adequate for this use.

[ehem_freebsd_m5p.com]

@jhb, I hope to do pretty much the same thing as D7784, but this time for the kernel portion instead of the x86 architecture portion. The reasoning is pretty well identical to D7784, this is cold-path and one portion does want to sleep, so shared-exclusive is quite appropriate. One distinction is there is no need to split any locks.

[mjg]

Apologies for the very late response!

I had a look at the the consumer you mentioned.

You may notice the sysctl handler starts with:

error = sysctl_wire_old_buffer(req, 0);
if (error != 0)
        return (error);

this makes the target memory readily available for access, guaranteed 0 shenanigans, which also means that handler does *not* run into the problem I mentioned earlier in this review.

I don't know why the code uses sx locks, pehraps it is an attempt of appeasing a bad assertion. The lock most likely does not have to be sx and the code should be patched away from it.

[ehem_freebsd_m5p.com]

I believe the main reason to opt for sx locks here (and the reason the x86 interrupt system uses one) is the exclusive callers are cold path. Almost solid-Helium cold. Most systems intr_event_create() will be called a number of times during kernel boot, but then never called again. As such this is a place to reduce memory use, even at the cost of substantial processor use.

If sysctl_wire_old_buffer() was added as a separate commit under someone else's name I wouldn't object. Right now the exclusive callers seem too rare to optimize for.

[mjg]

In D38450#888285, @ehem_freebsd_m5p.com wrote:

I believe the main reason to opt for sx locks here (and the reason the x86 interrupt system uses one) is the exclusive callers are cold path. Almost solid-Helium cold. Most systems intr_event_create() will be called a number of times during kernel boot, but then never called again. As such this is a place to reduce memory use, even at the cost of substantial processor use.

If sysctl_wire_old_buffer() was added as a separate commit under someone else's name I wouldn't object. Right now the exclusive callers seem too rare to optimize for.

sx locks have almost literally the same perf characteristics as rw locks and mutexes (including memory use) as far as any of this is concerned. There is most definitely no memory usage vs cpu time tradeoff when employing them.

Looking at the D7784 review again, looks like sx was introduced to work around a limitation of the driver where avoiding sleep would be too cumbersome -- may very well be true. I don't know why the author mentioned cost -- perhaps they were worried about extended sleep which would indeed negatively affect performance if the lock was under frequent use?

I don't see any considerations of the sort for this one. I once more note even in the above case the code wires user pages to prevent the case I mentioned -- an indefinite stall while holding the lock triggerabble by the user in sufficiently bad circumstances.

[ehem_freebsd_m5p.com]

Yet the infinite stall isn't a concern for 99.44% of systems. The two functions needing the lock exclusively are intr_event_create() and intr_event_destroy(), which most systems will only call during kernel start (and perhaps shutdown). During multiuser, those won't be called and there isn't a worry. As such sx is appropriate since an indefinite sleep isn't a concern, so this should avoid an extra unneeded restriction.

I'm concerned a generic kernel wires 2GB. Something is causing an awful lot of memory to get wired on current kernels and I don't want the blame for worsening the situation. Right now if a sysctl_wire_old_buffer() was added I would want that under someone else's name.

EDIT: Add to this, the sysctl_wire_old_buffer() would be closer to D36610.