virtio_net: set/cancel work on ndo_open/ndo_stop

Michael S. Tsirkin noticed that we could run the refill work after
ndo_close, which can re-enable napi - we don't disable it until
virtnet_remove.  This is clearly wrong, so move the workqueue control
to ndo_open and ndo_stop (aka. virtnet_open and virtnet_close).

One subtle point: virtnet_probe() could simply fail if it couldn't
allocate a receive buffer, but that's less polite in virtnet_open() so
we schedule a refill as we do in the normal receive path if we run out
of memory.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/net/virtio_net.c |   17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Michael will you integrate these patches from Rusty and submit them
to me along with other stuff you might have?

Or would you like me to apply them to net-next directly?

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Dec 29, 2011 at 02:38:06PM -0500, David Miller wrote:
> 
> Michael will you integrate these patches from Rusty and submit them
> to me along with other stuff you might have?
> 
> Or would you like me to apply them to net-next directly?
> 
> Thanks.

For personal reasons, my availability in this merge cycle is limited, so
net-next directly's better.

Thanks,

From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Thu, 29 Dec 2011 22:31:50 +0200

> On Thu, Dec 29, 2011 at 02:38:06PM -0500, David Miller wrote:
>> 
>> Michael will you integrate these patches from Rusty and submit them
>> to me along with other stuff you might have?
>> 
>> Or would you like me to apply them to net-next directly?
>> 
>> Thanks.
> 
> For personal reasons, my availability in this merge cycle is limited, so
> net-next directly's better.

Ok.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Dec 29, 2011 at 09:12:38PM +1030, Rusty Russell wrote:
> Michael S. Tsirkin noticed that we could run the refill work after
> ndo_close, which can re-enable napi - we don't disable it until
> virtnet_remove.  This is clearly wrong, so move the workqueue control
> to ndo_open and ndo_stop (aka. virtnet_open and virtnet_close).
> 
> One subtle point: virtnet_probe() could simply fail if it couldn't
> allocate a receive buffer, but that's less polite in virtnet_open() so
> we schedule a refill as we do in the normal receive path if we run out
> of memory.
> 
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Doh.
napi_disable does not prevent the following
napi_schedule, does it?

Can someone confirm that I am not seeing things please?

And this means this hack does not work:
try_fill_recv can still run in parallel with
napi, corrupting the vq.

I suspect we need to resurrect a patch that used a
dedicated flag to avoid this race.

Comments?

> ---
>  drivers/net/virtio_net.c |   17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -439,7 +439,13 @@ static int add_recvbuf_mergeable(struct 
>  	return err;
>  }
>  
> -/* Returns false if we couldn't fill entirely (OOM). */
> +/*
> + * Returns false if we couldn't fill entirely (OOM).
> + *
> + * Normally run in the receive path, but can also be run from ndo_open
> + * before we're receiving packets, or from refill_work which is
> + * careful to disable receiving (using napi_disable).
> + */
>  static bool try_fill_recv(struct virtnet_info *vi, gfp_t gfp)
>  {
>  	int err;
> @@ -719,6 +725,10 @@ static int virtnet_open(struct net_devic
>  {
>  	struct virtnet_info *vi = netdev_priv(dev);
>  
> +	/* Make sure we have some buffers: if oom use wq. */
> +	if (!try_fill_recv(vi, GFP_KERNEL))
> +		schedule_delayed_work(&vi->refill, 0);
> +
>  	virtnet_napi_enable(vi);
>  	return 0;
>  }
> @@ -772,6 +782,8 @@ static int virtnet_close(struct net_devi
>  {
>  	struct virtnet_info *vi = netdev_priv(dev);
>  
> +	/* Make sure refill_work doesn't re-enable napi! */
> +	cancel_delayed_work_sync(&vi->refill);
>  	napi_disable(&vi->napi);
>  
>  	return 0;
> @@ -1082,7 +1094,6 @@ static int virtnet_probe(struct virtio_d
>  
>  unregister:
>  	unregister_netdev(dev);
> -	cancel_delayed_work_sync(&vi->refill);
>  free_vqs:
>  	vdev->config->del_vqs(vdev);
>  free_stats:
> @@ -1121,9 +1132,7 @@ static void __devexit virtnet_remove(str
>  	/* Stop all the virtqueues. */
>  	vdev->config->reset(vdev);
>  
> -
>  	unregister_netdev(vi->dev);
> -	cancel_delayed_work_sync(&vi->refill);
>  
>  	/* Free unused buffers in both send and recv, if any. */
>  	free_unused_bufs(vi);
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, Apr 04, 2012 at 12:32:29PM +0300, Michael S. Tsirkin wrote:
> On Thu, Dec 29, 2011 at 09:12:38PM +1030, Rusty Russell wrote:
> > Michael S. Tsirkin noticed that we could run the refill work after
> > ndo_close, which can re-enable napi - we don't disable it until
> > virtnet_remove.  This is clearly wrong, so move the workqueue control
> > to ndo_open and ndo_stop (aka. virtnet_open and virtnet_close).
> > 
> > One subtle point: virtnet_probe() could simply fail if it couldn't
> > allocate a receive buffer, but that's less polite in virtnet_open() so
> > we schedule a refill as we do in the normal receive path if we run out
> > of memory.
> > 
> > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> 
> Doh.
> napi_disable does not prevent the following
> napi_schedule, does it?
> 
> Can someone confirm that I am not seeing things please?

Yes, I *was* seeing things. After napi_disable, NAPI_STATE_SCHED
is set to napi_schedule does nothing.
Sorry about the noise.

> And this means this hack does not work:
> try_fill_recv can still run in parallel with
> napi, corrupting the vq.
> 
> I suspect we need to resurrect a patch that used a
> dedicated flag to avoid this race.
> 
> Comments?
> 
> > ---
> >  drivers/net/virtio_net.c |   17 +++++++++++++----
> >  1 file changed, 13 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -439,7 +439,13 @@ static int add_recvbuf_mergeable(struct 
> >  	return err;
> >  }
> >  
> > -/* Returns false if we couldn't fill entirely (OOM). */
> > +/*
> > + * Returns false if we couldn't fill entirely (OOM).
> > + *
> > + * Normally run in the receive path, but can also be run from ndo_open
> > + * before we're receiving packets, or from refill_work which is
> > + * careful to disable receiving (using napi_disable).
> > + */
> >  static bool try_fill_recv(struct virtnet_info *vi, gfp_t gfp)
> >  {
> >  	int err;
> > @@ -719,6 +725,10 @@ static int virtnet_open(struct net_devic
> >  {
> >  	struct virtnet_info *vi = netdev_priv(dev);
> >  
> > +	/* Make sure we have some buffers: if oom use wq. */
> > +	if (!try_fill_recv(vi, GFP_KERNEL))
> > +		schedule_delayed_work(&vi->refill, 0);
> > +
> >  	virtnet_napi_enable(vi);
> >  	return 0;
> >  }
> > @@ -772,6 +782,8 @@ static int virtnet_close(struct net_devi
> >  {
> >  	struct virtnet_info *vi = netdev_priv(dev);
> >  
> > +	/* Make sure refill_work doesn't re-enable napi! */
> > +	cancel_delayed_work_sync(&vi->refill);
> >  	napi_disable(&vi->napi);
> >  
> >  	return 0;
> > @@ -1082,7 +1094,6 @@ static int virtnet_probe(struct virtio_d
> >  
> >  unregister:
> >  	unregister_netdev(dev);
> > -	cancel_delayed_work_sync(&vi->refill);
> >  free_vqs:
> >  	vdev->config->del_vqs(vdev);
> >  free_stats:
> > @@ -1121,9 +1132,7 @@ static void __devexit virtnet_remove(str
> >  	/* Stop all the virtqueues. */
> >  	vdev->config->reset(vdev);
> >  
> > -
> >  	unregister_netdev(vi->dev);
> > -	cancel_delayed_work_sync(&vi->refill);
> >  
> >  	/* Free unused buffers in both send and recv, if any. */
> >  	free_unused_bufs(vi);
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 04/04/2012 05:32 PM, Michael S. Tsirkin wrote:
> On Thu, Dec 29, 2011 at 09:12:38PM +1030, Rusty Russell wrote:
>> >  Michael S. Tsirkin noticed that we could run the refill work after
>> >  ndo_close, which can re-enable napi - we don't disable it until
>> >  virtnet_remove.  This is clearly wrong, so move the workqueue control
>> >  to ndo_open and ndo_stop (aka. virtnet_open and virtnet_close).
>> >  
>> >  One subtle point: virtnet_probe() could simply fail if it couldn't
>> >  allocate a receive buffer, but that's less polite in virtnet_open() so
>> >  we schedule a refill as we do in the normal receive path if we run out
>> >  of memory.
>> >  
>> >  Signed-off-by: Rusty Russell<rusty@rustcorp.com.au>
> Doh.
> napi_disable does not prevent the following
> napi_schedule, does it?
>
> Can someone confirm that I am not seeing things please?
Looks like napi_disable() does prevent the following scheduling, as 
napi_schedule_prep() returns true only when there's an 0 -> 1 transition 
of NAPI_STATE_SCHED bit.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html