Re: [PATCH 0/2] eventfd: simplify signal helpers
On Mon, Jul 17, 2023 at 04:52:03PM -0600, Alex Williamson wrote: > On Mon, 17 Jul 2023 19:12:16 -0300 > Jason Gunthorpe wrote: > > > On Mon, Jul 17, 2023 at 01:08:31PM -0600, Alex Williamson wrote: > > > > > What would that mechanism be? We've been iterating on getting the > > > serialization and buffering correct, but I don't know of another means > > > that combines the notification with a value, so we'd likely end up with > > > an eventfd only for notification and a separate ring buffer for > > > notification values. > > > > All FDs do this. You just have to make a FD with custom > > file_operations that does what this wants. The uAPI shouldn't be able > > to tell if the FD is backing it with an eventfd or otherwise. Have the > > kernel return the FD instead of accepting it. Follow the basic design > > of eg mlx5vf_save_fops > > Sure, userspace could poll on any fd and read a value from it, but at > that point we're essentially duplicating a lot of what eventfd provides > for a minor(?) semantic difference over how the counter value is > interpreted. Using an actual eventfd allows the ACPI notification to > work as just another interrupt index within the existing vfio IRQ > uAPI. Yes, duplicated, sort of, whatever the "ack" is to allow pushing a new value can be revised to run as part of the read. But I don't really view it as a minor difference. eventfd is a counter. It should not be abused otherwise, even if it can be made to work. It really isn't an IRQ if it is pushing an async message w/data. Jason
Re: [PATCH 0/2] eventfd: simplify signal helpers
On Mon, 17 Jul 2023 19:12:16 -0300 Jason Gunthorpe wrote: > On Mon, Jul 17, 2023 at 01:08:31PM -0600, Alex Williamson wrote: > > > What would that mechanism be? We've been iterating on getting the > > serialization and buffering correct, but I don't know of another means > > that combines the notification with a value, so we'd likely end up with > > an eventfd only for notification and a separate ring buffer for > > notification values. > > All FDs do this. You just have to make a FD with custom > file_operations that does what this wants. The uAPI shouldn't be able > to tell if the FD is backing it with an eventfd or otherwise. Have the > kernel return the FD instead of accepting it. Follow the basic design > of eg mlx5vf_save_fops Sure, userspace could poll on any fd and read a value from it, but at that point we're essentially duplicating a lot of what eventfd provides for a minor(?) semantic difference over how the counter value is interpreted. Using an actual eventfd allows the ACPI notification to work as just another interrupt index within the existing vfio IRQ uAPI. Thanks, Alex
Re: [PATCH 0/2] eventfd: simplify signal helpers
On Mon, Jul 17, 2023 at 01:08:31PM -0600, Alex Williamson wrote: > What would that mechanism be? We've been iterating on getting the > serialization and buffering correct, but I don't know of another means > that combines the notification with a value, so we'd likely end up with > an eventfd only for notification and a separate ring buffer for > notification values. All FDs do this. You just have to make a FD with custom file_operations that does what this wants. The uAPI shouldn't be able to tell if the FD is backing it with an eventfd or otherwise. Have the kernel return the FD instead of accepting it. Follow the basic design of eg mlx5vf_save_fops Jason
Re: [PATCH 0/2] eventfd: simplify signal helpers
On Mon, 17 Jul 2023 10:29:34 +0200 Grzegorz Jaszczyk wrote: > pt., 14 lip 2023 o 09:05 Christian Brauner napisał(a): > > > > On Thu, Jul 13, 2023 at 11:10:54AM -0600, Alex Williamson wrote: > > > On Thu, 13 Jul 2023 12:05:36 +0200 > > > Christian Brauner wrote: > > > > > > > Hey everyone, > > > > > > > > This simplifies the eventfd_signal() and eventfd_signal_mask() helpers > > > > by removing the count argument which is effectively unused. > > > > > > We have a patch under review which does in fact make use of the > > > signaling value: > > > > > > https://lore.kernel.org/all/20230630155936.3015595-1-...@semihalf.com/ > > > > Huh, thanks for the link. > > > > Quoting from > > https://patchwork.kernel.org/project/kvm/patch/20230307220553.631069-1-...@semihalf.com/#25266856 > > > > > Reading an eventfd returns an 8-byte value, we generally only use it > > > as a counter, but it's been discussed previously and IIRC, it's possible > > > to use that value as a notification value. > > > > So the goal is to pipe a specific value through eventfd? But it is > > explicitly a counter. The whole thing is written around a counter and > > each write and signal adds to the counter. > > > > The consequences are pretty well described in the cover letter of > > v6 https://lore.kernel.org/all/20230630155936.3015595-1-...@semihalf.com/ > > > > > Since the eventfd counter is used as ACPI notification value > > > placeholder, the eventfd signaling needs to be serialized in order to > > > not end up with notification values being coalesced. Therefore ACPI > > > notification values are buffered and signalized one by one, when the > > > previous notification value has been consumed. > > > > But isn't this a good indication that you really don't want an eventfd > > but something that's explicitly designed to associate specific data with > > a notification? Using eventfd in that manner requires serialization, > > buffering, and enforces ordering. What would that mechanism be? We've been iterating on getting the serialization and buffering correct, but I don't know of another means that combines the notification with a value, so we'd likely end up with an eventfd only for notification and a separate ring buffer for notification values. As this series demonstrates, the current in-kernel users only increment the counter and most userspace likely discards the counter value, which makes the counter largely a waste. While perhaps unconventional, there's no requirement that the counter may only be incremented by one, nor any restriction that I see in how userspace must interpret the counter value. As I understand the ACPI notification proposal that Grzegorz links below, a notification with an interpreted value allows for a more direct userspace implementation when dealing with a series of discrete notification with value events. Thanks, Alex > > I have no skin in the game aside from having to drop this conversion > > which I'm fine to do if there are actually users for this btu really, > > that looks a lot like abusing an api that really wasn't designed for > > this. > > https://patchwork.kernel.org/project/kvm/patch/20230307220553.631069-1-...@semihalf.com/ > was posted at the beginig of March and one of the main things we've > discussed was the mechanism for propagating acpi notification value. > We've endup with eventfd as the best mechanism and have actually been > using it from v2. I really do not want to waste this effort, I think > we are quite advanced with v6 now. Additionally we didn't actually > modify any part of eventfd support that was in place, we only used it > in a specific (and discussed beforehand) way.
Re: [PATCH 0/2] eventfd: simplify signal helpers
pt., 14 lip 2023 o 09:05 Christian Brauner napisał(a): > > On Thu, Jul 13, 2023 at 11:10:54AM -0600, Alex Williamson wrote: > > On Thu, 13 Jul 2023 12:05:36 +0200 > > Christian Brauner wrote: > > > > > Hey everyone, > > > > > > This simplifies the eventfd_signal() and eventfd_signal_mask() helpers > > > by removing the count argument which is effectively unused. > > > > We have a patch under review which does in fact make use of the > > signaling value: > > > > https://lore.kernel.org/all/20230630155936.3015595-1-...@semihalf.com/ > > Huh, thanks for the link. > > Quoting from > https://patchwork.kernel.org/project/kvm/patch/20230307220553.631069-1-...@semihalf.com/#25266856 > > > Reading an eventfd returns an 8-byte value, we generally only use it > > as a counter, but it's been discussed previously and IIRC, it's possible > > to use that value as a notification value. > > So the goal is to pipe a specific value through eventfd? But it is > explicitly a counter. The whole thing is written around a counter and > each write and signal adds to the counter. > > The consequences are pretty well described in the cover letter of > v6 https://lore.kernel.org/all/20230630155936.3015595-1-...@semihalf.com/ > > > Since the eventfd counter is used as ACPI notification value > > placeholder, the eventfd signaling needs to be serialized in order to > > not end up with notification values being coalesced. Therefore ACPI > > notification values are buffered and signalized one by one, when the > > previous notification value has been consumed. > > But isn't this a good indication that you really don't want an eventfd > but something that's explicitly designed to associate specific data with > a notification? Using eventfd in that manner requires serialization, > buffering, and enforces ordering. > > I have no skin in the game aside from having to drop this conversion > which I'm fine to do if there are actually users for this btu really, > that looks a lot like abusing an api that really wasn't designed for > this. https://patchwork.kernel.org/project/kvm/patch/20230307220553.631069-1-...@semihalf.com/ was posted at the beginig of March and one of the main things we've discussed was the mechanism for propagating acpi notification value. We've endup with eventfd as the best mechanism and have actually been using it from v2. I really do not want to waste this effort, I think we are quite advanced with v6 now. Additionally we didn't actually modify any part of eventfd support that was in place, we only used it in a specific (and discussed beforehand) way.
Re: [PATCH 0/2] eventfd: simplify signal helpers
On Fri, Jul 14, 2023 at 09:05:21AM +0200, Christian Brauner wrote: > I have no skin in the game aside from having to drop this conversion > which I'm fine to do if there are actually users for this btu really, > that looks a lot like abusing an api that really wasn't designed for > this. Yeah, I think so too. The ACPI thing should use its own FD if it wants to feed actual data.. Jason
Re: [PATCH 0/2] eventfd: simplify signal helpers
On Thu, Jul 13, 2023 at 11:10:54AM -0600, Alex Williamson wrote: > On Thu, 13 Jul 2023 12:05:36 +0200 > Christian Brauner wrote: > > > Hey everyone, > > > > This simplifies the eventfd_signal() and eventfd_signal_mask() helpers > > by removing the count argument which is effectively unused. > > We have a patch under review which does in fact make use of the > signaling value: > > https://lore.kernel.org/all/20230630155936.3015595-1-...@semihalf.com/ Huh, thanks for the link. Quoting from https://patchwork.kernel.org/project/kvm/patch/20230307220553.631069-1-...@semihalf.com/#25266856 > Reading an eventfd returns an 8-byte value, we generally only use it > as a counter, but it's been discussed previously and IIRC, it's possible > to use that value as a notification value. So the goal is to pipe a specific value through eventfd? But it is explicitly a counter. The whole thing is written around a counter and each write and signal adds to the counter. The consequences are pretty well described in the cover letter of v6 https://lore.kernel.org/all/20230630155936.3015595-1-...@semihalf.com/ > Since the eventfd counter is used as ACPI notification value > placeholder, the eventfd signaling needs to be serialized in order to > not end up with notification values being coalesced. Therefore ACPI > notification values are buffered and signalized one by one, when the > previous notification value has been consumed. But isn't this a good indication that you really don't want an eventfd but something that's explicitly designed to associate specific data with a notification? Using eventfd in that manner requires serialization, buffering, and enforces ordering. I have no skin in the game aside from having to drop this conversion which I'm fine to do if there are actually users for this btu really, that looks a lot like abusing an api that really wasn't designed for this.
Re: [PATCH 0/2] eventfd: simplify signal helpers
On Thu, 13 Jul 2023 12:05:36 +0200 Christian Brauner wrote: > Hey everyone, > > This simplifies the eventfd_signal() and eventfd_signal_mask() helpers > by removing the count argument which is effectively unused. We have a patch under review which does in fact make use of the signaling value: https://lore.kernel.org/all/20230630155936.3015595-1-...@semihalf.com/ Thanks, Alex
[PATCH 0/2] eventfd: simplify signal helpers
Hey everyone, This simplifies the eventfd_signal() and eventfd_signal_mask() helpers by removing the count argument which is effectively unused. --- --- base-commit: 6be357f00aad4189130147fdc6f568cf776a4909 change-id: 20230713-vfs-eventfd-signal-0b0d167ad6ec
