| Message ID | 20170113025100.4535.35887.stgit@john-Precision-Tower-5810 |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, 12 Jan 2017 18:51:00 -0800 John Fastabend <john.fastabend@gmail.com> wrote: > > -static void free_receive_bufs(struct virtnet_info *vi) > +static void free_receive_bufs(struct virtnet_info *vi, bool need_lock) > { > struct bpf_prog *old_prog; > int i; > > - rtnl_lock(); > + if (need_lock) > + rtnl_lock(); > for (i = 0; i < vi->max_queue_pairs; i++) { > while (vi->rq[i].pages) > __free_pages(get_a_page(&vi->rq[i], GFP_KERNEL), 0); > @@ -1879,7 +1880,8 @@ static void free_receive_bufs(struct virtnet_info *vi) > if (old_prog) > bpf_prog_put(old_prog); > } > - rtnl_unlock(); > + if (need_lock) > + rtnl_unlock(); > } Conditional locking is bad idea; sparse complains about it and is later source of bugs. The more typical way of doing this in kernel is: void _foo(some args) { ASSERT_RTNL(); ... } void foo(some args) { rtnl_lock(); _foo(some args) rtnl_unlock(); }
On 17-01-13 08:34 AM, Stephen Hemminger wrote: > On Thu, 12 Jan 2017 18:51:00 -0800 > John Fastabend <john.fastabend@gmail.com> wrote: > >> >> -static void free_receive_bufs(struct virtnet_info *vi) >> +static void free_receive_bufs(struct virtnet_info *vi, bool need_lock) >> { >> struct bpf_prog *old_prog; >> int i; >> >> - rtnl_lock(); >> + if (need_lock) >> + rtnl_lock(); >> for (i = 0; i < vi->max_queue_pairs; i++) { >> while (vi->rq[i].pages) >> __free_pages(get_a_page(&vi->rq[i], GFP_KERNEL), 0); >> @@ -1879,7 +1880,8 @@ static void free_receive_bufs(struct virtnet_info *vi) >> if (old_prog) >> bpf_prog_put(old_prog); >> } >> - rtnl_unlock(); >> + if (need_lock) >> + rtnl_unlock(); >> } > > Conditional locking is bad idea; sparse complains about it and is later source > of bugs. The more typical way of doing this in kernel is: OK I'll use the normal form. > > void _foo(some args) > { > ASSERT_RTNL(); > > ... > } > > void foo(some args) > { > rtnl_lock(); > _foo(some args) > rtnl_unlock(); > } > >
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index d97bb71..43cb2e0 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -1864,12 +1864,13 @@ static void virtnet_free_queues(struct virtnet_info *vi) kfree(vi->sq); } -static void free_receive_bufs(struct virtnet_info *vi) +static void free_receive_bufs(struct virtnet_info *vi, bool need_lock) { struct bpf_prog *old_prog; int i; - rtnl_lock(); + if (need_lock) + rtnl_lock(); for (i = 0; i < vi->max_queue_pairs; i++) { while (vi->rq[i].pages) __free_pages(get_a_page(&vi->rq[i], GFP_KERNEL), 0); @@ -1879,7 +1880,8 @@ static void free_receive_bufs(struct virtnet_info *vi) if (old_prog) bpf_prog_put(old_prog); } - rtnl_unlock(); + if (need_lock) + rtnl_unlock(); } static void free_receive_page_frags(struct virtnet_info *vi) @@ -2351,14 +2353,14 @@ static int virtnet_probe(struct virtio_device *vdev) return err; } -static void remove_vq_common(struct virtnet_info *vi) +static void remove_vq_common(struct virtnet_info *vi, bool lock) { vi->vdev->config->reset(vi->vdev); /* Free unused buffers in both send and recv, if any. */ free_unused_bufs(vi); - free_receive_bufs(vi); + free_receive_bufs(vi, lock); free_receive_page_frags(vi); @@ -2376,7 +2378,7 @@ static void virtnet_remove(struct virtio_device *vdev) unregister_netdev(vi->dev); - remove_vq_common(vi); + remove_vq_common(vi, true); free_percpu(vi->stats); free_netdev(vi->dev); @@ -2401,7 +2403,7 @@ static int virtnet_freeze(struct virtio_device *vdev) napi_disable(&vi->rq[i].napi); } - remove_vq_common(vi); + remove_vq_common(vi, true); return 0; }
For XDP use case and to allow ethtool reset tests it is useful to be able to use reset routines from contexts where rtnl lock is already held. Signed-off-by: John Fastabend <john.r.fastabend@intel.com> --- drivers/net/virtio_net.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)