* [PATCH] uio_hv_generic: Set event for all channels on the device
@ 2025-02-27 0:45 longli
2025-02-28 19:11 ` Michael Kelley
0 siblings, 1 reply; 3+ messages in thread
From: longli @ 2025-02-27 0:45 UTC (permalink / raw)
To: K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui,
Greg Kroah-Hartman, linux-hyperv, linux-kernel
Cc: Long Li
From: Long Li <longli@microsoft.com>
Hyper-V may offer a non latency sensitive device with subchannels without
monitor bit enabled. The decision is entirely on the Hyper-V host not
configurable within guest.
When a device has subchannels, also signal events for the subchannel
if its monitor bit is disabled.
Signed-off-by: Long Li <longli@microsoft.com>
---
drivers/uio/uio_hv_generic.c | 30 ++++++++++++++++++++++++------
1 file changed, 24 insertions(+), 6 deletions(-)
diff --git a/drivers/uio/uio_hv_generic.c b/drivers/uio/uio_hv_generic.c
index 3976360d0096..8b6df598a728 100644
--- a/drivers/uio/uio_hv_generic.c
+++ b/drivers/uio/uio_hv_generic.c
@@ -65,6 +65,16 @@ struct hv_uio_private_data {
char send_name[32];
};
+static void set_event(struct vmbus_channel *channel, s32 irq_state)
+{
+ channel->inbound.ring_buffer->interrupt_mask = !irq_state;
+ if (!channel->offermsg.monitor_allocated && irq_state) {
+ /* MB is needed for host to see the interrupt mask first */
+ virt_mb();
+ vmbus_setevent(channel);
+ }
+}
+
/*
* This is the irqcontrol callback to be registered to uio_info.
* It can be used to disable/enable interrupt from user space processes.
@@ -79,12 +89,13 @@ hv_uio_irqcontrol(struct uio_info *info, s32 irq_state)
{
struct hv_uio_private_data *pdata = info->priv;
struct hv_device *dev = pdata->device;
+ struct vmbus_channel *primary, *sc;
- dev->channel->inbound.ring_buffer->interrupt_mask = !irq_state;
- virt_mb();
+ primary = dev->channel;
+ set_event(primary, irq_state);
- if (!dev->channel->offermsg.monitor_allocated && irq_state)
- vmbus_setevent(dev->channel);
+ list_for_each_entry(sc, &primary->sc_list, sc_list)
+ set_event(sc, irq_state);
return 0;
}
@@ -95,12 +106,19 @@ hv_uio_irqcontrol(struct uio_info *info, s32 irq_state)
static void hv_uio_channel_cb(void *context)
{
struct vmbus_channel *chan = context;
- struct hv_device *hv_dev = chan->device_obj;
- struct hv_uio_private_data *pdata = hv_get_drvdata(hv_dev);
+ struct hv_device *hv_dev;
+ struct hv_uio_private_data *pdata;
chan->inbound.ring_buffer->interrupt_mask = 1;
virt_mb();
+ /*
+ * The callback may come from a subchannel, in which case look
+ * for the hv device in the primary channel
+ */
+ hv_dev = chan->primary_channel ?
+ chan->primary_channel->device_obj : chan->device_obj;
+ pdata = hv_get_drvdata(hv_dev);
uio_event_notify(&pdata->info);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* RE: [PATCH] uio_hv_generic: Set event for all channels on the device
2025-02-27 0:45 [PATCH] uio_hv_generic: Set event for all channels on the device longli
@ 2025-02-28 19:11 ` Michael Kelley
2025-02-28 20:14 ` Long Li
0 siblings, 1 reply; 3+ messages in thread
From: Michael Kelley @ 2025-02-28 19:11 UTC (permalink / raw)
To: longli@linuxonhyperv.com, K. Y. Srinivasan, Haiyang Zhang,
Wei Liu, Dexuan Cui, Greg Kroah-Hartman,
linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org
Cc: Long Li
From: longli@linuxonhyperv.com <longli@linuxonhyperv.com> Sent: Wednesday, February 26, 2025 4:46 PM
>
> Hyper-V may offer a non latency sensitive device with subchannels without
> monitor bit enabled. The decision is entirely on the Hyper-V host not
> configurable within guest.
>
> When a device has subchannels, also signal events for the subchannel
> if its monitor bit is disabled.
>
> Signed-off-by: Long Li <longli@microsoft.com>
> ---
> drivers/uio/uio_hv_generic.c | 30 ++++++++++++++++++++++++------
> 1 file changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/uio/uio_hv_generic.c b/drivers/uio/uio_hv_generic.c
> index 3976360d0096..8b6df598a728 100644
> --- a/drivers/uio/uio_hv_generic.c
> +++ b/drivers/uio/uio_hv_generic.c
> @@ -65,6 +65,16 @@ struct hv_uio_private_data {
> char send_name[32];
> };
>
> +static void set_event(struct vmbus_channel *channel, s32 irq_state)
> +{
> + channel->inbound.ring_buffer->interrupt_mask = !irq_state;
> + if (!channel->offermsg.monitor_allocated && irq_state) {
> + /* MB is needed for host to see the interrupt mask first */
> + virt_mb();
> + vmbus_setevent(channel);
A minor point, but vmbus_setevent() checks the "monitor_allocated"
flag, and if not set, then calls vmbus_set_event(). Since
monitor_allocated() is already checked here, couldn't vmbus_set_event()
be called directly? The existing code calls vmbus_setevent() so keeping
vmbus_setevent() is less of a change, but it is still doing a redundant
check.
> + }
> +}
> +
> /*
> * This is the irqcontrol callback to be registered to uio_info.
> * It can be used to disable/enable interrupt from user space processes.
> @@ -79,12 +89,13 @@ hv_uio_irqcontrol(struct uio_info *info, s32 irq_state)
> {
> struct hv_uio_private_data *pdata = info->priv;
> struct hv_device *dev = pdata->device;
> + struct vmbus_channel *primary, *sc;
>
> - dev->channel->inbound.ring_buffer->interrupt_mask = !irq_state;
> - virt_mb();
> + primary = dev->channel;
> + set_event(primary, irq_state);
>
> - if (!dev->channel->offermsg.monitor_allocated && irq_state)
> - vmbus_setevent(dev->channel);
> + list_for_each_entry(sc, &primary->sc_list, sc_list)
> + set_event(sc, irq_state);
Walking the sc_list usually requires holding vmbus_connection.channel_mutex.
Is there a reason it's safe to walk the list here without the mutex?
>
> return 0;
> }
> @@ -95,12 +106,19 @@ hv_uio_irqcontrol(struct uio_info *info, s32 irq_state)
> static void hv_uio_channel_cb(void *context)
> {
> struct vmbus_channel *chan = context;
> - struct hv_device *hv_dev = chan->device_obj;
> - struct hv_uio_private_data *pdata = hv_get_drvdata(hv_dev);
> + struct hv_device *hv_dev;
> + struct hv_uio_private_data *pdata;
>
> chan->inbound.ring_buffer->interrupt_mask = 1;
> virt_mb();
>
> + /*
> + * The callback may come from a subchannel, in which case look
> + * for the hv device in the primary channel
> + */
> + hv_dev = chan->primary_channel ?
> + chan->primary_channel->device_obj : chan->device_obj;
This certainly looks correct and necessary. But how did this work in the
past? Wouldn't DPDK running on a synthetic NIC have gotten callbacks on
a subchannel? I'm just trying to understand whether this a bug fix, or if
not, what new scenario requires this change. I'm not understanding how
this change is related to the commit message.
Michael
> + pdata = hv_get_drvdata(hv_dev);
> uio_event_notify(&pdata->info);
> }
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: [PATCH] uio_hv_generic: Set event for all channels on the device
2025-02-28 19:11 ` Michael Kelley
@ 2025-02-28 20:14 ` Long Li
0 siblings, 0 replies; 3+ messages in thread
From: Long Li @ 2025-02-28 20:14 UTC (permalink / raw)
To: Michael Kelley, longli@linuxonhyperv.com, KY Srinivasan,
Haiyang Zhang, Wei Liu, Dexuan Cui, Greg Kroah-Hartman,
linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org
> From: longli@linuxonhyperv.com <longli@linuxonhyperv.com> Sent:
> Wednesday, February 26, 2025 4:46 PM
> >
> > Hyper-V may offer a non latency sensitive device with subchannels
> > without monitor bit enabled. The decision is entirely on the Hyper-V
> > host not configurable within guest.
> >
> > When a device has subchannels, also signal events for the subchannel
> > if its monitor bit is disabled.
> >
> > Signed-off-by: Long Li <longli@microsoft.com>
> > ---
> > drivers/uio/uio_hv_generic.c | 30 ++++++++++++++++++++++++------
> > 1 file changed, 24 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/uio/uio_hv_generic.c
> > b/drivers/uio/uio_hv_generic.c index 3976360d0096..8b6df598a728
> 100644
> > --- a/drivers/uio/uio_hv_generic.c
> > +++ b/drivers/uio/uio_hv_generic.c
> > @@ -65,6 +65,16 @@ struct hv_uio_private_data {
> > char send_name[32];
> > };
> >
> > +static void set_event(struct vmbus_channel *channel, s32 irq_state) {
> > + channel->inbound.ring_buffer->interrupt_mask = !irq_state;
> > + if (!channel->offermsg.monitor_allocated && irq_state) {
> > + /* MB is needed for host to see the interrupt mask first */
> > + virt_mb();
> > + vmbus_setevent(channel);
>
> A minor point, but vmbus_setevent() checks the "monitor_allocated"
> flag, and if not set, then calls vmbus_set_event(). Since
> monitor_allocated() is already checked here, couldn't vmbus_set_event() be
> called directly? The existing code calls vmbus_setevent() so keeping
> vmbus_setevent() is less of a change, but it is still doing a redundant check.
Will change it to vmbus_set_event().
>
> > + }
> > +}
> > +
> > /*
> > * This is the irqcontrol callback to be registered to uio_info.
> > * It can be used to disable/enable interrupt from user space processes.
> > @@ -79,12 +89,13 @@ hv_uio_irqcontrol(struct uio_info *info, s32
> > irq_state) {
> > struct hv_uio_private_data *pdata = info->priv;
> > struct hv_device *dev = pdata->device;
> > + struct vmbus_channel *primary, *sc;
> >
> > - dev->channel->inbound.ring_buffer->interrupt_mask = !irq_state;
> > - virt_mb();
> > + primary = dev->channel;
> > + set_event(primary, irq_state);
> >
> > - if (!dev->channel->offermsg.monitor_allocated && irq_state)
> > - vmbus_setevent(dev->channel);
> > + list_for_each_entry(sc, &primary->sc_list, sc_list)
> > + set_event(sc, irq_state);
>
> Walking the sc_list usually requires holding
> vmbus_connection.channel_mutex.
> Is there a reason it's safe to walk the list here without the mutex?
You are right, we need a lock here. Will be adding it.
>
> >
> > return 0;
> > }
> > @@ -95,12 +106,19 @@ hv_uio_irqcontrol(struct uio_info *info, s32
> > irq_state) static void hv_uio_channel_cb(void *context) {
> > struct vmbus_channel *chan = context;
> > - struct hv_device *hv_dev = chan->device_obj;
> > - struct hv_uio_private_data *pdata = hv_get_drvdata(hv_dev);
> > + struct hv_device *hv_dev;
> > + struct hv_uio_private_data *pdata;
> >
> > chan->inbound.ring_buffer->interrupt_mask = 1;
> > virt_mb();
> >
> > + /*
> > + * The callback may come from a subchannel, in which case look
> > + * for the hv device in the primary channel
> > + */
> > + hv_dev = chan->primary_channel ?
> > + chan->primary_channel->device_obj : chan->device_obj;
>
> This certainly looks correct and necessary. But how did this work in the past?
> Wouldn't DPDK running on a synthetic NIC have gotten callbacks on a
> subchannel? I'm just trying to understand whether this a bug fix, or if not,
> what new scenario requires this change. I'm not understanding how this
> change is related to the commit message.
>
> Michael
In the past, host always offers sub channels with monitor bit enabled, and the interrupt mask on the channel is always set so we never got callbacks.
This has changed in that the host may offer channels without monitoring. This change also requires patches at DPDK netvsc driver to work on unmonitored channels. I think it's a new scenario that requires changes at both ends.
Long
>
> > + pdata = hv_get_drvdata(hv_dev);
> > uio_event_notify(&pdata->info);
> > }
> >
> > --
> > 2.34.1
> >
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-02-28 20:14 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-27 0:45 [PATCH] uio_hv_generic: Set event for all channels on the device longli
2025-02-28 19:11 ` Michael Kelley
2025-02-28 20:14 ` Long Li
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).