[PATCH] hyperv: Add netpoll support

[PATCH] hyperv: Add netpoll support

[Richard Weinberger]

In order to have at least a netconsole to debug kernel issues on
Windows Azure this patch implements netpoll support.
Sending packets is easy, netvsc_start_xmit() does already everything
needed.
To receive we need to trigger the channel callback which is usally
called via tasklet_schedule().

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/net/hyperv/netvsc_drv.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 4fd71b7..367b71e 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -736,6 +736,17 @@ static int netvsc_set_mac_addr(struct net_device *ndev, void *p)
 	return err;
 }
 
+#ifdef CONFIG_NET_POLL_CONTROLLER
+static void netvsc_poll_controller(struct net_device *net)
+{
+	struct net_device_context *net_device_ctx = netdev_priv(net);
+	struct hv_device *dev = net_device_ctx->device_ctx;
+
+	local_bh_disable();
+	netvsc_channel_cb(dev->channel);
+	local_bh_enable();
+}
+#endif
 
 static const struct ethtool_ops ethtool_ops = {
 	.get_drvinfo	= netvsc_get_drvinfo,
@@ -751,6 +762,9 @@ static const struct net_device_ops device_ops = {
 	.ndo_validate_addr =		eth_validate_addr,
 	.ndo_set_mac_address =		netvsc_set_mac_addr,
 	.ndo_select_queue =		netvsc_select_queue,
+#ifdef CONFIG_NET_POLL_CONTROLLER
+	.ndo_poll_controller =		netvsc_poll_controller,
+#endif
 };
 
 /*
-- 
2.0.1

RE: [PATCH] hyperv: Add netpoll support

[Haiyang Zhang]

> -----Original Message-----
> From: Richard Weinberger [mailto:richard@nod.at]
> Sent: Tuesday, July 8, 2014 5:32 AM
> To: KY Srinivasan; Haiyang Zhang
> Cc: devel@linuxdriverproject.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; Richard Weinberger
> Subject: [PATCH] hyperv: Add netpoll support
> 
> In order to have at least a netconsole to debug kernel issues on
> Windows Azure this patch implements netpoll support.
> Sending packets is easy, netvsc_start_xmit() does already everything
> needed.
> To receive we need to trigger the channel callback which is usally
> called via tasklet_schedule().
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  drivers/net/hyperv/netvsc_drv.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/net/hyperv/netvsc_drv.c
> b/drivers/net/hyperv/netvsc_drv.c
> index 4fd71b7..367b71e 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -736,6 +736,17 @@ static int netvsc_set_mac_addr(struct net_device
> *ndev, void *p)
>  	return err;
>  }
> 
> +#ifdef CONFIG_NET_POLL_CONTROLLER
> +static void netvsc_poll_controller(struct net_device *net)
> +{
> +	struct net_device_context *net_device_ctx = netdev_priv(net);
> +	struct hv_device *dev = net_device_ctx->device_ctx;
> +
> +	local_bh_disable();
> +	netvsc_channel_cb(dev->channel);

This can only poll the primary channel not the sub channels.

Thanks,
- Haiyang

Re: [PATCH] hyperv: Add netpoll support

[Richard Weinberger]

Am 08.07.2014 19:55, schrieb Haiyang Zhang:
> 
> 
>> -----Original Message-----
>> From: Richard Weinberger [mailto:richard@nod.at]
>> Sent: Tuesday, July 8, 2014 5:32 AM
>> To: KY Srinivasan; Haiyang Zhang
>> Cc: devel@linuxdriverproject.org; netdev@vger.kernel.org; linux-
>> kernel@vger.kernel.org; Richard Weinberger
>> Subject: [PATCH] hyperv: Add netpoll support
>>
>> In order to have at least a netconsole to debug kernel issues on
>> Windows Azure this patch implements netpoll support.
>> Sending packets is easy, netvsc_start_xmit() does already everything
>> needed.
>> To receive we need to trigger the channel callback which is usally
>> called via tasklet_schedule().
>>
>> Signed-off-by: Richard Weinberger <richard@nod.at>
>> ---
>>  drivers/net/hyperv/netvsc_drv.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/net/hyperv/netvsc_drv.c
>> b/drivers/net/hyperv/netvsc_drv.c
>> index 4fd71b7..367b71e 100644
>> --- a/drivers/net/hyperv/netvsc_drv.c
>> +++ b/drivers/net/hyperv/netvsc_drv.c
>> @@ -736,6 +736,17 @@ static int netvsc_set_mac_addr(struct net_device
>> *ndev, void *p)
>>  	return err;
>>  }
>>
>> +#ifdef CONFIG_NET_POLL_CONTROLLER
>> +static void netvsc_poll_controller(struct net_device *net)
>> +{
>> +	struct net_device_context *net_device_ctx = netdev_priv(net);
>> +	struct hv_device *dev = net_device_ctx->device_ctx;
>> +
>> +	local_bh_disable();
>> +	netvsc_channel_cb(dev->channel);
> 
> This can only poll the primary channel not the sub channels.

Sub channels in terms of one channel per VCPU as KY said?

*confused*,
//richard

RE: [PATCH] hyperv: Add netpoll support

[Haiyang Zhang]

> -----Original Message-----
> From: Richard Weinberger [mailto:richard@nod.at]
> Sent: Tuesday, July 8, 2014 2:40 PM
> To: Haiyang Zhang; KY Srinivasan
> Cc: devel@linuxdriverproject.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH] hyperv: Add netpoll support
> 
> Am 08.07.2014 19:55, schrieb Haiyang Zhang:
> >
> >
> >> -----Original Message-----
> >> From: Richard Weinberger [mailto:richard@nod.at]
> >> Sent: Tuesday, July 8, 2014 5:32 AM
> >> To: KY Srinivasan; Haiyang Zhang
> >> Cc: devel@linuxdriverproject.org; netdev@vger.kernel.org; linux-
> >> kernel@vger.kernel.org; Richard Weinberger
> >> Subject: [PATCH] hyperv: Add netpoll support
> >>
> >> In order to have at least a netconsole to debug kernel issues on
> >> Windows Azure this patch implements netpoll support.
> >> Sending packets is easy, netvsc_start_xmit() does already everything
> >> needed.
> >> To receive we need to trigger the channel callback which is usally
> >> called via tasklet_schedule().
> >>
> >> Signed-off-by: Richard Weinberger <richard@nod.at>
> >> ---
> >>  drivers/net/hyperv/netvsc_drv.c | 14 ++++++++++++++
> >>  1 file changed, 14 insertions(+)
> >>
> >> diff --git a/drivers/net/hyperv/netvsc_drv.c
> >> b/drivers/net/hyperv/netvsc_drv.c
> >> index 4fd71b7..367b71e 100644
> >> --- a/drivers/net/hyperv/netvsc_drv.c
> >> +++ b/drivers/net/hyperv/netvsc_drv.c
> >> @@ -736,6 +736,17 @@ static int netvsc_set_mac_addr(struct net_device
> >> *ndev, void *p)
> >>  	return err;
> >>  }
> >>
> >> +#ifdef CONFIG_NET_POLL_CONTROLLER
> >> +static void netvsc_poll_controller(struct net_device *net)
> >> +{
> >> +	struct net_device_context *net_device_ctx = netdev_priv(net);
> >> +	struct hv_device *dev = net_device_ctx->device_ctx;
> >> +
> >> +	local_bh_disable();
> >> +	netvsc_channel_cb(dev->channel);
> >
> > This can only poll the primary channel not the sub channels.
> 
> Sub channels in terms of one channel per VCPU as KY said?
> 
> *confused*,

Since it's used only for debugging, polling the subchannels may not be
necessary.

Regarding the CPU binding, KY will reply you in another email.

Thanks,
- Haiyang

RE: [PATCH] hyperv: Add netpoll support

[KY Srinivasan]

> -----Original Message-----
> From: Richard Weinberger [mailto:richard@nod.at]
> Sent: Tuesday, July 8, 2014 2:32 AM
> To: KY Srinivasan; Haiyang Zhang
> Cc: devel@linuxdriverproject.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; Richard Weinberger
> Subject: [PATCH] hyperv: Add netpoll support
> 
> In order to have at least a netconsole to debug kernel issues on Windows
> Azure this patch implements netpoll support.
> Sending packets is easy, netvsc_start_xmit() does already everything
> needed.
> To receive we need to trigger the channel callback which is usally called via
> tasklet_schedule().
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  drivers/net/hyperv/netvsc_drv.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/net/hyperv/netvsc_drv.c
> b/drivers/net/hyperv/netvsc_drv.c index 4fd71b7..367b71e 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -736,6 +736,17 @@ static int netvsc_set_mac_addr(struct net_device
> *ndev, void *p)
>  	return err;
>  }
> 
> +#ifdef CONFIG_NET_POLL_CONTROLLER
> +static void netvsc_poll_controller(struct net_device *net) {
> +	struct net_device_context *net_device_ctx = netdev_priv(net);
> +	struct hv_device *dev = net_device_ctx->device_ctx;
> +
> +	local_bh_disable();
> +	netvsc_channel_cb(dev->channel);
> +	local_bh_enable();
> +}
> +#endif

Each channel is bound to a specific VCPU in the guest and the channel callback is expected to be delivered on
the VCPU the channel is bound to. This code is not satisfying that requirement.

Regards,

K. Y

Re: [PATCH] hyperv: Add netpoll support

[Richard Weinberger]

Am 08.07.2014 20:01, schrieb KY Srinivasan:
> 
> 
>> -----Original Message-----
>> From: Richard Weinberger [mailto:richard@nod.at]
>> Sent: Tuesday, July 8, 2014 2:32 AM
>> To: KY Srinivasan; Haiyang Zhang
>> Cc: devel@linuxdriverproject.org; netdev@vger.kernel.org; linux-
>> kernel@vger.kernel.org; Richard Weinberger
>> Subject: [PATCH] hyperv: Add netpoll support
>>
>> In order to have at least a netconsole to debug kernel issues on Windows
>> Azure this patch implements netpoll support.
>> Sending packets is easy, netvsc_start_xmit() does already everything
>> needed.
>> To receive we need to trigger the channel callback which is usally called via
>> tasklet_schedule().
>>
>> Signed-off-by: Richard Weinberger <richard@nod.at>
>> ---
>>  drivers/net/hyperv/netvsc_drv.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/net/hyperv/netvsc_drv.c
>> b/drivers/net/hyperv/netvsc_drv.c index 4fd71b7..367b71e 100644
>> --- a/drivers/net/hyperv/netvsc_drv.c
>> +++ b/drivers/net/hyperv/netvsc_drv.c
>> @@ -736,6 +736,17 @@ static int netvsc_set_mac_addr(struct net_device
>> *ndev, void *p)
>>  	return err;
>>  }
>>
>> +#ifdef CONFIG_NET_POLL_CONTROLLER
>> +static void netvsc_poll_controller(struct net_device *net) {
>> +	struct net_device_context *net_device_ctx = netdev_priv(net);
>> +	struct hv_device *dev = net_device_ctx->device_ctx;
>> +
>> +	local_bh_disable();
>> +	netvsc_channel_cb(dev->channel);
>> +	local_bh_enable();
>> +}
>> +#endif
> 
> Each channel is bound to a specific VCPU in the guest and the channel callback is expected to be delivered on
> the VCPU the channel is bound to. This code is not satisfying that requirement.

But struct hv_device has only one channel attribute. How does this work with multiple VCPUs?

Anyways, what solution to you propose?

Thanks,
//richard

RE: [PATCH] hyperv: Add netpoll support

[KY Srinivasan]

> -----Original Message-----
> From: Richard Weinberger [mailto:richard@nod.at]
> Sent: Tuesday, July 8, 2014 11:39 AM
> To: KY Srinivasan; Haiyang Zhang
> Cc: devel@linuxdriverproject.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH] hyperv: Add netpoll support
> 
> Am 08.07.2014 20:01, schrieb KY Srinivasan:
> >
> >
> >> -----Original Message-----
> >> From: Richard Weinberger [mailto:richard@nod.at]
> >> Sent: Tuesday, July 8, 2014 2:32 AM
> >> To: KY Srinivasan; Haiyang Zhang
> >> Cc: devel@linuxdriverproject.org; netdev@vger.kernel.org; linux-
> >> kernel@vger.kernel.org; Richard Weinberger
> >> Subject: [PATCH] hyperv: Add netpoll support
> >>
> >> In order to have at least a netconsole to debug kernel issues on
> >> Windows Azure this patch implements netpoll support.
> >> Sending packets is easy, netvsc_start_xmit() does already everything
> >> needed.
> >> To receive we need to trigger the channel callback which is usally
> >> called via tasklet_schedule().
> >>
> >> Signed-off-by: Richard Weinberger <richard@nod.at>
> >> ---
> >>  drivers/net/hyperv/netvsc_drv.c | 14 ++++++++++++++
> >>  1 file changed, 14 insertions(+)
> >>
> >> diff --git a/drivers/net/hyperv/netvsc_drv.c
> >> b/drivers/net/hyperv/netvsc_drv.c index 4fd71b7..367b71e 100644
> >> --- a/drivers/net/hyperv/netvsc_drv.c
> >> +++ b/drivers/net/hyperv/netvsc_drv.c
> >> @@ -736,6 +736,17 @@ static int netvsc_set_mac_addr(struct net_device
> >> *ndev, void *p)
> >>  	return err;
> >>  }
> >>
> >> +#ifdef CONFIG_NET_POLL_CONTROLLER
> >> +static void netvsc_poll_controller(struct net_device *net) {
> >> +	struct net_device_context *net_device_ctx = netdev_priv(net);
> >> +	struct hv_device *dev = net_device_ctx->device_ctx;
> >> +
> >> +	local_bh_disable();
> >> +	netvsc_channel_cb(dev->channel);
> >> +	local_bh_enable();
> >> +}
> >> +#endif
> >
> > Each channel is bound to a specific VCPU in the guest and the channel
> > callback is expected to be delivered on the VCPU the channel is bound to.
> This code is not satisfying that requirement.
> 
> But struct hv_device has only one channel attribute. How does this work with
> multiple VCPUs?
> 
> Anyways, what solution to you propose?

The VCPU the channel is bound to is available in the channel state. You could use the following code
Fragment to ensure that the call is made on the "right" cpu:

smp_call_function_single(dev->channel->target_cpu,
                                         netvsc_channel_cb, dev->channel, true);

Hope this helps.

K. Y

Re: [PATCH] hyperv: Add netpoll support

[Richard Weinberger]

Am 08.07.2014 22:03, schrieb KY Srinivasan:
> The VCPU the channel is bound to is available in the channel state. You could use the following code
> Fragment to ensure that the call is made on the "right" cpu:
> 
> smp_call_function_single(dev->channel->target_cpu,
>                                          netvsc_channel_cb, dev->channel, true);

This won't work as netpoll runs with IRQs disabled.
->ndo_poll_controller() has to make sure that SKBs can be received and transmitted
while IRQs are off. I thought calling the channel callback by hand would be enough
to receive SKBs.

Thanks,
//richard

Re: [PATCH] hyperv: Add netpoll support

[Francois Romieu]

Richard Weinberger <richard@nod.at> :
[...]
> This won't work as netpoll runs with IRQs disabled.
> ->ndo_poll_controller() has to make sure that SKBs can be received and transmitted
> while IRQs are off. I thought calling the channel callback by hand would be
> enough to receive SKBs.

What are you taking about ? netconsole does not need to receive.

hyperv start_xmit handler almost does its own Tx completion as you have
noticed. The situation is imho close to a virtual device one as was veth
in bb446c19fefd7b4435adb12a9dd7666adc5b553a.

-- 
Ueimor

Re: [PATCH] hyperv: Add netpoll support

[Richard Weinberger]

Am 09.07.2014 00:47, schrieb Francois Romieu:
> Richard Weinberger <richard@nod.at> :
> [...]
>> This won't work as netpoll runs with IRQs disabled.
>> ->ndo_poll_controller() has to make sure that SKBs can be received and transmitted
>> while IRQs are off. I thought calling the channel callback by hand would be
>> enough to receive SKBs.
> 
> What are you taking about ? netconsole does not need to receive.

Isn't netconsole is only one user of netpoll?
Of course netconsole needs only to transmit SKBs.
But if you look at other ->ndo_poll_controller implementations
you'll notice that they care also about receiving.

> hyperv start_xmit handler almost does its own Tx completion as you have
> noticed. The situation is imho close to a virtual device one as was veth
> in bb446c19fefd7b4435adb12a9dd7666adc5b553a.

Bad commit reference: bb446c19fefd7b4435adb12a9dd7666adc5b553a

:-(

Thanks,
//richard

Re: [PATCH] hyperv: Add netpoll support

[Francois Romieu]

Richard Weinberger <richard@nod.at> :
> Am 09.07.2014 00:47, schrieb Francois Romieu:
[...]
> > What are you taking about ? netconsole does not need to receive.
> 
> Isn't netconsole is only one user of netpoll ?

Out of tree users are irrelevant. See netpoll related comments in
cd6362befe4cc7bf589a5236d2a780af2d47bcc9

> Of course netconsole needs only to transmit SKBs.
> But if you look at other ->ndo_poll_controller implementations
> you'll notice that they care also about receiving.

It's just the long, illuminating history of netpoll :o)

Some limited Rx netpoll support may be done but it needs more work
than was originally advertised.

> > hyperv start_xmit handler almost does its own Tx completion as you have
> > noticed. The situation is imho close to a virtual device one as was veth
> > in bb446c19fefd7b4435adb12a9dd7666adc5b553a.
> 
> Bad commit reference: bb446c19fefd7b4435adb12a9dd7666adc5b553a

Sorry, it currently belongs to davem's net-next.

-- 
Ueimor

Re: [PATCH] hyperv: Add netpoll support

[Richard Weinberger]

Am 09.07.2014 01:43, schrieb Francois Romieu:
> Richard Weinberger <richard@nod.at> :
>> Am 09.07.2014 00:47, schrieb Francois Romieu:
> [...]
>>> What are you taking about ? netconsole does not need to receive.
>>
>> Isn't netconsole is only one user of netpoll ?
> 
> Out of tree users are irrelevant. See netpoll related comments in
> cd6362befe4cc7bf589a5236d2a780af2d47bcc9

Thanks lot for pointing this out!

>> Of course netconsole needs only to transmit SKBs.
>> But if you look at other ->ndo_poll_controller implementations
>> you'll notice that they care also about receiving.
> 
> It's just the long, illuminating history of netpoll :o)
> 
> Some limited Rx netpoll support may be done but it needs more work
> than was originally advertised.
> 
>>> hyperv start_xmit handler almost does its own Tx completion as you have
>>> noticed. The situation is imho close to a virtual device one as was veth
>>> in bb446c19fefd7b4435adb12a9dd7666adc5b553a.
>>
>> Bad commit reference: bb446c19fefd7b4435adb12a9dd7666adc5b553a
> 
> Sorry, it currently belongs to davem's net-next.

Found it already. :)

Thanks,
//richard

Re: [PATCH] hyperv: Add netpoll support

[Richard Weinberger]

Am 09.07.2014 00:47, schrieb Francois Romieu:
> Richard Weinberger <richard@nod.at> :
> [...]
>> This won't work as netpoll runs with IRQs disabled.
>> ->ndo_poll_controller() has to make sure that SKBs can be received and transmitted
>> while IRQs are off. I thought calling the channel callback by hand would be
>> enough to receive SKBs.
> 
> What are you taking about ? netconsole does not need to receive.
> 
> hyperv start_xmit handler almost does its own Tx completion as you have
> noticed. The situation is imho close to a virtual device one as was veth
> in bb446c19fefd7b4435adb12a9dd7666adc5b553a.

Ah, net-next.git.
My first (in-house) patch had the same empty poll controller as tun.c and now veth.c have.
If we are fine with tx only, I'll happily resend an updated patch with an empty poll controller. :-)

Thanks,
//richard