The UMA usage looks ok to me. NOFREE is an unfortunately big hammer, but it's really the only one you have if you want to guarantee successful M_NOWAIT allocations. This topic has come up like three or four times in different contexts in the past month so it seems worth it to fix properly, i.e., provide a reliable mechanism that guarantees up to N simultaneous allocations from a reserved pool. uma_zone_reserve() gets part of the way there but has holes:

• keg_drain() ignores the reservation (I'm testing a patch for this);
• uma_zfree() doesn't replenish the reservation, so a subsequent alloc may not see the freed item even when the reservation is depleted (I have an idea on how to fix it).

Ping? Are we good? Or have I missed something ?

Will do another spin... sbuf isn't setup for 'callouts' like this :(

Though I think I'll fix the sbuf buf in setpos...

I think one key problem is that there are setups where non-"global" root can provoke event notification -- not only this poses question about proper sanitization of data, it poses the question what to do with flooding the device. For example jailed root can be allowed to mount/unmount filesystems which now triggers a HUGE message.

The current notion of "there is a message or not" is in my opinion wasteful. A circular buffer where you alloc upfront the bytes you need OR block waiting for more OR abort would work out better. In the simplest case you would populate the reserved area under the global lock. The current patch blindly requires 1KB per event (presumably motivated with the mount/unmount stuff) while most events are way shorter.

The format of space separated key=value is also problematic in that it is bigger than it needs to be, but that's perhaps for a different patchset.

In D26140#582298, @mjg wrote:

I think one key problem is that there are setups where non-"global" root can provoke event notification -- not only this poses question about proper sanitization of data, it poses the question what to do with flooding the device. For example jailed root can be allowed to mount/unmount filesystems which now triggers a HUGE message.

Ignore that. The mount messages need to be redone. But, these issues are more important. The buffers are a little larger to cope in the interim. The mount messages are wrong in a lot of other ways as well, and I need to puzzle through how best to cope (there's both too much information (some of it is bogus, some could be gleaned by other means) and too little (there needs to be some jail ID at least, for example). That's beyond the scope of this patch.

The current notion of "there is a message or not" is in my opinion wasteful. A circular buffer where you alloc upfront the bytes you need OR block waiting for more OR abort would work out better. In the simplest case you would populate the reserved area under the global lock. The current patch blindly requires 1KB per event (presumably motivated with the mount/unmount stuff) while most events are way shorter.

Yea, but the cost is only 1M, and likely more like 25k-50k once my rework is done. But one step at a time.

Also, the policy stuff about what to drop when needs some careful though. I've received feedback over the years that we need better low memory behavior, and it would be better to toss oldest message rather than newest if we can't allocate memory, which is what prompted this design. In a typical system, even when there's a flurry of events, there's not a huge demand since devd is good about keeping up... Though I'm taking to heart that some thought might be needed as we expand to different domains that exist on the system (aka jails) and we need to be mindful of starvation and other similar things...

For now, though, I think it's adequate to allow other things to make progress. Especially since the current cost is 1MB (tiny on modern systems, and also configurable at boot time if I'm wrong).

The format of space separated key=value is also problematic in that it is bigger than it needs to be, but that's perhaps for a different patchset.

That's not going to change in the near term. There's a lot of user-facing scripts that rely on it, and doing something different would require a lot of careful redesign which we don't have time to do before 13 branches. I have a number of cleanups that I'd like to do that would hide much of the key=value stuff from the in-kernel code. This might allow us to do something smarter in the future, but there's a lot of usage in the kernel that I'd like to clean up first.

tl;dr: you're not wrong in the long term, but in the short term I'd like to compromise to make progress towards the long-term goals.

In D26140#582304, @imp wrote:

In D26140#582298, @mjg wrote:

I think one key problem is that there are setups where non-"global" root can provoke event notification -- not only this poses question about proper sanitization of data, it poses the question what to do with flooding the device. For example jailed root can be allowed to mount/unmount filesystems which now triggers a HUGE message.

Ignore that. The mount messages need to be redone. But, these issues are more important. The buffers are a little larger to cope in the interim. The mount messages are wrong in a lot of other ways as well, and I need to puzzle through how best to cope (there's both too much information (some of it is bogus, some could be gleaned by other means) and too little (there needs to be some jail ID at least, for example). That's beyond the scope of this patch.

The current notion of "there is a message or not" is in my opinion wasteful. A circular buffer where you alloc upfront the bytes you need OR block waiting for more OR abort would work out better. In the simplest case you would populate the reserved area under the global lock. The current patch blindly requires 1KB per event (presumably motivated with the mount/unmount stuff) while most events are way shorter.

Yea, but the cost is only 1M, and likely more like 25k-50k once my rework is done. But one step at a time.

Also, the policy stuff about what to drop when needs some careful though. I've received feedback over the years that we need better low memory behavior, and it would be better to toss oldest message rather than newest if we can't allocate memory, which is what prompted this design. In a typical system, even when there's a flurry of events, there's not a huge demand since devd is good about keeping up... Though I'm taking to heart that some thought might be needed as we expand to different domains that exist on the system (aka jails) and we need to be mindful of starvation and other similar things...

So the question is what messages can be treated as optional to deliver and what messages (if any) as mandatory.

I just don't see any win from having a NOFREE zone for it, seems avoidably wasteful but if this is an intermediate state to be cleaned up soon(tm) it's probably fine. Still, I think there is a flag to disable per-CPU caching and it would be nice to use it here.

For now, though, I think it's adequate to allow other things to make progress. Especially since the current cost is 1MB (tiny on modern systems, and also configurable at boot time if I'm wrong).

The format of space separated key=value is also problematic in that it is bigger than it needs to be, but that's perhaps for a different patchset.

That's not going to change in the near term. There's a lot of user-facing scripts that rely on it, and doing something different would require a lot of careful redesign which we don't have time to do before 13 branches. I have a number of cleanups that I'd like to do that would hide much of the key=value stuff from the in-kernel code. This might allow us to do something smarter in the future, but there's a lot of usage in the kernel that I'd like to clean up first.

tl;dr: you're not wrong in the long term, but in the short term I'd like to compromise to make progress towards the long-term goals.