[RFC] virtio_net: add local_bh_disable() around u64_stats_update_begin

[RFC] virtio_net: add local_bh_disable() around u64_stats_update_begin

[Sebastian Andrzej Siewior]

on 32bit, lockdep notices:
| ================================
| WARNING: inconsistent lock state
| 4.19.0-rc8+ #9 Tainted: G        W
| --------------------------------
| inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage.
| ip/1106 [HC0[0]:SC1[1]:HE1:SE0] takes:
| (ptrval) (&syncp->seq#2){+.?.}, at: net_rx_action+0xc8/0x380
| {SOFTIRQ-ON-W} state was registered at:
|   lock_acquire+0x7e/0x170
|   try_fill_recv+0x5fa/0x700
|   virtnet_open+0xe0/0x180
|   __dev_open+0xae/0x130
|   __dev_change_flags+0x17f/0x200
|   dev_change_flags+0x23/0x60
|   do_setlink+0x2bb/0xa20
|   rtnl_newlink+0x523/0x830
|   rtnetlink_rcv_msg+0x14b/0x470
|   netlink_rcv_skb+0x6e/0xf0
|   rtnetlink_rcv+0xd/0x10
|   netlink_unicast+0x16e/0x1f0
|   netlink_sendmsg+0x1af/0x3a0
|   ___sys_sendmsg+0x20f/0x240
|   __sys_sendmsg+0x39/0x80
|   sys_socketcall+0x13a/0x2a0
|   do_int80_syscall_32+0x50/0x180
|   restore_all+0x0/0xb2
| irq event stamp: 3326
| hardirqs last  enabled at (3326): [<c159e6d0>] net_rx_action+0x80/0x380
| hardirqs last disabled at (3325): [<c159e6aa>] net_rx_action+0x5a/0x380
| softirqs last  enabled at (3322): [<c14b440d>] virtnet_napi_enable+0xd/0x60
| softirqs last disabled at (3323): [<c101d63d>] call_on_stack+0xd/0x50
|
| other info that might help us debug this:
|  Possible unsafe locking scenario:
|
|        CPU0
|        ----
|   lock(&syncp->seq#2);
|   <Interrupt>
|     lock(&syncp->seq#2);
|
|  *** DEADLOCK ***

This is the "up" path which is not a hotpath. There is also
refill_work().
It might be unwise to add the local_bh_disable() to try_fill_recv()
because if it is used mostly in BH so that local_bh_en+dis might be a
waste of cycles.

Adding local_bh_disable() around try_fill_recv() for the non-BH call
sites would render GFP_KERNEL pointless.

Also, ptr->var++ is not an atomic operation even on 64bit CPUs. Which
means if try_fill_recv() runs on CPU0 (via virtnet_receive()) then the
worker might run on CPU1.

Do we care or is this just stupid stats?  Any suggestions?

This warning appears since commit 461f03dc99cf6 ("virtio_net: Add kick stats").

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/net/virtio_net.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index dab504ec5e502..d782160cfa882 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1206,9 +1206,11 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
 			break;
 	} while (rq->vq->num_free);
 	if (virtqueue_kick_prepare(rq->vq) && virtqueue_notify(rq->vq)) {
+		local_bh_disable();
 		u64_stats_update_begin(&rq->stats.syncp);
 		rq->stats.kicks++;
 		u64_stats_update_end(&rq->stats.syncp);
+		local_bh_enable();
 	}
 
 	return !oom;
-- 
2.19.1

Re: [RFC] virtio_net: add local_bh_disable() around u64_stats_update_begin

[Stephen Hemminger]

On Tue, 16 Oct 2018 18:55:45 +0200
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> on 32bit, lockdep notices:
> | ================================
> | WARNING: inconsistent lock state
> | 4.19.0-rc8+ #9 Tainted: G        W
> | --------------------------------
> | inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage.
> | ip/1106 [HC0[0]:SC1[1]:HE1:SE0] takes:
> | (ptrval) (&syncp->seq#2){+.?.}, at: net_rx_action+0xc8/0x380
> | {SOFTIRQ-ON-W} state was registered at:
> |   lock_acquire+0x7e/0x170
> |   try_fill_recv+0x5fa/0x700
> |   virtnet_open+0xe0/0x180
> |   __dev_open+0xae/0x130
> |   __dev_change_flags+0x17f/0x200
> |   dev_change_flags+0x23/0x60
> |   do_setlink+0x2bb/0xa20
> |   rtnl_newlink+0x523/0x830
> |   rtnetlink_rcv_msg+0x14b/0x470
> |   netlink_rcv_skb+0x6e/0xf0
> |   rtnetlink_rcv+0xd/0x10
> |   netlink_unicast+0x16e/0x1f0
> |   netlink_sendmsg+0x1af/0x3a0
> |   ___sys_sendmsg+0x20f/0x240
> |   __sys_sendmsg+0x39/0x80
> |   sys_socketcall+0x13a/0x2a0
> |   do_int80_syscall_32+0x50/0x180
> |   restore_all+0x0/0xb2
> | irq event stamp: 3326
> | hardirqs last  enabled at (3326): [<c159e6d0>] net_rx_action+0x80/0x380
> | hardirqs last disabled at (3325): [<c159e6aa>] net_rx_action+0x5a/0x380
> | softirqs last  enabled at (3322): [<c14b440d>] virtnet_napi_enable+0xd/0x60
> | softirqs last disabled at (3323): [<c101d63d>] call_on_stack+0xd/0x50
> |
> | other info that might help us debug this:
> |  Possible unsafe locking scenario:
> |
> |        CPU0
> |        ----
> |   lock(&syncp->seq#2);
> |   <Interrupt>
> |     lock(&syncp->seq#2);
> |
> |  *** DEADLOCK ***
> 
> This is the "up" path which is not a hotpath. There is also
> refill_work().
> It might be unwise to add the local_bh_disable() to try_fill_recv()
> because if it is used mostly in BH so that local_bh_en+dis might be a
> waste of cycles.
> 
> Adding local_bh_disable() around try_fill_recv() for the non-BH call
> sites would render GFP_KERNEL pointless.
> 
> Also, ptr->var++ is not an atomic operation even on 64bit CPUs. Which
> means if try_fill_recv() runs on CPU0 (via virtnet_receive()) then the
> worker might run on CPU1.
> 
> Do we care or is this just stupid stats?  Any suggestions?
> 
> This warning appears since commit 461f03dc99cf6 ("virtio_net: Add kick stats").
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/net/virtio_net.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index dab504ec5e502..d782160cfa882 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1206,9 +1206,11 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
>  			break;
>  	} while (rq->vq->num_free);
>  	if (virtqueue_kick_prepare(rq->vq) && virtqueue_notify(rq->vq)) {
> +		local_bh_disable();
>  		u64_stats_update_begin(&rq->stats.syncp);
>  		rq->stats.kicks++;
>  		u64_stats_update_end(&rq->stats.syncp);
> +		local_bh_enable();
>  	}
>  
>  	return !oom;

Since there already is u64_stats_update_begin_irqsave inline, why not introduce
u64_stats_update_begin_bh which encapsulates the local_bh_disable

Re: [RFC] virtio_net: add local_bh_disable() around u64_stats_update_begin

[Stephen Hemminger]

On Tue, 16 Oct 2018 18:55:45 +0200
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> on 32bit, lockdep notices:
> | ================================
> | WARNING: inconsistent lock state
> | 4.19.0-rc8+ #9 Tainted: G        W
> | --------------------------------
> | inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage.
> | ip/1106 [HC0[0]:SC1[1]:HE1:SE0] takes:
> | (ptrval) (&syncp->seq#2){+.?.}, at: net_rx_action+0xc8/0x380
> | {SOFTIRQ-ON-W} state was registered at:
> |   lock_acquire+0x7e/0x170
> |   try_fill_recv+0x5fa/0x700
> |   virtnet_open+0xe0/0x180
> |   __dev_open+0xae/0x130
> |   __dev_change_flags+0x17f/0x200
> |   dev_change_flags+0x23/0x60
> |   do_setlink+0x2bb/0xa20
> |   rtnl_newlink+0x523/0x830
> |   rtnetlink_rcv_msg+0x14b/0x470
> |   netlink_rcv_skb+0x6e/0xf0
> |   rtnetlink_rcv+0xd/0x10
> |   netlink_unicast+0x16e/0x1f0
> |   netlink_sendmsg+0x1af/0x3a0
> |   ___sys_sendmsg+0x20f/0x240
> |   __sys_sendmsg+0x39/0x80
> |   sys_socketcall+0x13a/0x2a0
> |   do_int80_syscall_32+0x50/0x180
> |   restore_all+0x0/0xb2
> | irq event stamp: 3326
> | hardirqs last  enabled at (3326): [<c159e6d0>] net_rx_action+0x80/0x380
> | hardirqs last disabled at (3325): [<c159e6aa>] net_rx_action+0x5a/0x380
> | softirqs last  enabled at (3322): [<c14b440d>] virtnet_napi_enable+0xd/0x60
> | softirqs last disabled at (3323): [<c101d63d>] call_on_stack+0xd/0x50
> |
> | other info that might help us debug this:
> |  Possible unsafe locking scenario:
> |
> |        CPU0
> |        ----
> |   lock(&syncp->seq#2);
> |   <Interrupt>
> |     lock(&syncp->seq#2);
> |
> |  *** DEADLOCK ***
> 
> This is the "up" path which is not a hotpath. There is also
> refill_work().
> It might be unwise to add the local_bh_disable() to try_fill_recv()
> because if it is used mostly in BH so that local_bh_en+dis might be a
> waste of cycles.
> 
> Adding local_bh_disable() around try_fill_recv() for the non-BH call
> sites would render GFP_KERNEL pointless.
> 
> Also, ptr->var++ is not an atomic operation even on 64bit CPUs. Which
> means if try_fill_recv() runs on CPU0 (via virtnet_receive()) then the
> worker might run on CPU1.
> 
> Do we care or is this just stupid stats?  Any suggestions?
> 
> This warning appears since commit 461f03dc99cf6 ("virtio_net: Add kick stats").
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/net/virtio_net.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index dab504ec5e502..d782160cfa882 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1206,9 +1206,11 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
>  			break;
>  	} while (rq->vq->num_free);
>  	if (virtqueue_kick_prepare(rq->vq) && virtqueue_notify(rq->vq)) {
> +		local_bh_disable();
>  		u64_stats_update_begin(&rq->stats.syncp);
>  		rq->stats.kicks++;
>  		u64_stats_update_end(&rq->stats.syncp);
> +		local_bh_enable();
>  	}
>  
>  	return !oom;

Since there already is u64_stats_update_begin_irqsave inline, why not introduce
u64_stats_update_begin_bh which encapsulates the local_bh_disable

Re: [RFC] virtio_net: add local_bh_disable() around u64_stats_update_begin

[Stephen Hemminger]

On Tue, 16 Oct 2018 18:55:45 +0200
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> Also, ptr->var++ is not an atomic operation even on 64bit CPUs. Which
> means if try_fill_recv() runs on CPU0 (via virtnet_receive()) then the
> worker might run on CPU1.

On modern CPU's increment of native types is atomic but not locked.
u64_stats_update_begin is a no-op on UP and also if BIT_PER_LONG != 32

Re: [RFC] virtio_net: add local_bh_disable() around u64_stats_update_begin

[Stephen Hemminger]

On Tue, 16 Oct 2018 18:55:45 +0200
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> Also, ptr->var++ is not an atomic operation even on 64bit CPUs. Which
> means if try_fill_recv() runs on CPU0 (via virtnet_receive()) then the
> worker might run on CPU1.

On modern CPU's increment of native types is atomic but not locked.
u64_stats_update_begin is a no-op on UP and also if BIT_PER_LONG != 32

Re: [RFC] virtio_net: add local_bh_disable() around u64_stats_update_begin

[Sebastian Andrzej Siewior]

On 2018-10-16 10:59:30 [-0700], Stephen Hemminger wrote:
> Since there already is u64_stats_update_begin_irqsave inline, why not introduce
> u64_stats_update_begin_bh which encapsulates the local_bh_disable

CPU0				CPU1
refill_work()			virtnet_receive()
 try_fill_recv()		 try_fill_recv()
  u64_stats_update_begin_bh()	  u64_stats_update_begin_bh()

both CPUs may operate on the `rq'.

Sebastian

Re: [RFC] virtio_net: add local_bh_disable() around u64_stats_update_begin

[Sebastian Andrzej Siewior]

On 2018-10-16 11:01:14 [-0700], Stephen Hemminger wrote:
> On Tue, 16 Oct 2018 18:55:45 +0200
> Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> 
> > Also, ptr->var++ is not an atomic operation even on 64bit CPUs. Which
> > means if try_fill_recv() runs on CPU0 (via virtnet_receive()) then the
> > worker might run on CPU1.
> 
> On modern CPU's increment of native types is atomic but not locked.
> u64_stats_update_begin is a no-op on UP and also if BIT_PER_LONG != 32

On ARM64 you have load, inc, store. So if two CPUs increment the counter
simultaneously we might lose one increment. That is why I asked if we
care or not.

Sebastian

Re: [RFC] virtio_net: add local_bh_disable() around u64_stats_update_begin

[Sebastian Andrzej Siewior]

On 2018-10-16 11:01:14 [-0700], Stephen Hemminger wrote:
> On Tue, 16 Oct 2018 18:55:45 +0200
> Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> 
> > Also, ptr->var++ is not an atomic operation even on 64bit CPUs. Which
> > means if try_fill_recv() runs on CPU0 (via virtnet_receive()) then the
> > worker might run on CPU1.
> 
> On modern CPU's increment of native types is atomic but not locked.
> u64_stats_update_begin is a no-op on UP and also if BIT_PER_LONG != 32

On ARM64 you have load, inc, store. So if two CPUs increment the counter
simultaneously we might lose one increment. That is why I asked if we
care or not.

Sebastian

Re: [RFC] virtio_net: add local_bh_disable() around u64_stats_update_begin

[Stephen Hemminger]

On Tue, 16 Oct 2018 20:42:07 +0200
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> On 2018-10-16 11:01:14 [-0700], Stephen Hemminger wrote:
> > On Tue, 16 Oct 2018 18:55:45 +0200
> > Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> >   
> > > Also, ptr->var++ is not an atomic operation even on 64bit CPUs. Which
> > > means if try_fill_recv() runs on CPU0 (via virtnet_receive()) then the
> > > worker might run on CPU1.  
> > 
> > On modern CPU's increment of native types is atomic but not locked.
> > u64_stats_update_begin is a no-op on UP and also if BIT_PER_LONG != 32  
> 
> On ARM64 you have load, inc, store. So if two CPUs increment the counter
> simultaneously we might lose one increment. That is why I asked if we
> care or not.
> 
> Sebastian

The point is that kicks is just a counter, not important as part of the
device operation. The point of the u64_stats is to avoid problems with
high/low 32 bit wrap on increment. So this is ok on ARM64.

Re: [RFC] virtio_net: add local_bh_disable() around u64_stats_update_begin

[Stephen Hemminger]

On Tue, 16 Oct 2018 20:42:07 +0200
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> On 2018-10-16 11:01:14 [-0700], Stephen Hemminger wrote:
> > On Tue, 16 Oct 2018 18:55:45 +0200
> > Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> >   
> > > Also, ptr->var++ is not an atomic operation even on 64bit CPUs. Which
> > > means if try_fill_recv() runs on CPU0 (via virtnet_receive()) then the
> > > worker might run on CPU1.  
> > 
> > On modern CPU's increment of native types is atomic but not locked.
> > u64_stats_update_begin is a no-op on UP and also if BIT_PER_LONG != 32  
> 
> On ARM64 you have load, inc, store. So if two CPUs increment the counter
> simultaneously we might lose one increment. That is why I asked if we
> care or not.
> 
> Sebastian

The point is that kicks is just a counter, not important as part of the
device operation. The point of the u64_stats is to avoid problems with
high/low 32 bit wrap on increment. So this is ok on ARM64.

Re: [RFC] virtio_net: add local_bh_disable() around u64_stats_update_begin

[Toshiaki Makita]

On 2018/10/17 1:55, Sebastian Andrzej Siewior wrote:
> on 32bit, lockdep notices:
> | ================================
> | WARNING: inconsistent lock state
> | 4.19.0-rc8+ #9 Tainted: G        W
> | --------------------------------
> | inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage.
> | ip/1106 [HC0[0]:SC1[1]:HE1:SE0] takes:
> | (ptrval) (&syncp->seq#2){+.?.}, at: net_rx_action+0xc8/0x380
> | {SOFTIRQ-ON-W} state was registered at:
> |   lock_acquire+0x7e/0x170
> |   try_fill_recv+0x5fa/0x700
> |   virtnet_open+0xe0/0x180
> |   __dev_open+0xae/0x130
> |   __dev_change_flags+0x17f/0x200
> |   dev_change_flags+0x23/0x60
> |   do_setlink+0x2bb/0xa20
> |   rtnl_newlink+0x523/0x830
> |   rtnetlink_rcv_msg+0x14b/0x470
> |   netlink_rcv_skb+0x6e/0xf0
> |   rtnetlink_rcv+0xd/0x10
> |   netlink_unicast+0x16e/0x1f0
> |   netlink_sendmsg+0x1af/0x3a0
> |   ___sys_sendmsg+0x20f/0x240
> |   __sys_sendmsg+0x39/0x80
> |   sys_socketcall+0x13a/0x2a0
> |   do_int80_syscall_32+0x50/0x180
> |   restore_all+0x0/0xb2
> | irq event stamp: 3326
> | hardirqs last  enabled at (3326): [<c159e6d0>] net_rx_action+0x80/0x380
> | hardirqs last disabled at (3325): [<c159e6aa>] net_rx_action+0x5a/0x380
> | softirqs last  enabled at (3322): [<c14b440d>] virtnet_napi_enable+0xd/0x60
> | softirqs last disabled at (3323): [<c101d63d>] call_on_stack+0xd/0x50
> |
> | other info that might help us debug this:
> |  Possible unsafe locking scenario:
> |
> |        CPU0
> |        ----
> |   lock(&syncp->seq#2);
> |   <Interrupt>
> |     lock(&syncp->seq#2);
> |
> |  *** DEADLOCK ***

IIUC try_fill_recv is called only when NAPI is disabled from process
context, so there should be no point to race with virtnet_receive which
is called from NAPI handler.

I'm not sure what condition triggered this warning.


Toshiaki Makita


> 
> This is the "up" path which is not a hotpath. There is also
> refill_work().
> It might be unwise to add the local_bh_disable() to try_fill_recv()
> because if it is used mostly in BH so that local_bh_en+dis might be a
> waste of cycles.
> 
> Adding local_bh_disable() around try_fill_recv() for the non-BH call
> sites would render GFP_KERNEL pointless.
> 
> Also, ptr->var++ is not an atomic operation even on 64bit CPUs. Which
> means if try_fill_recv() runs on CPU0 (via virtnet_receive()) then the
> worker might run on CPU1.
> 
> Do we care or is this just stupid stats?  Any suggestions?
> 
> This warning appears since commit 461f03dc99cf6 ("virtio_net: Add kick stats").
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/net/virtio_net.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index dab504ec5e502..d782160cfa882 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1206,9 +1206,11 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
>  			break;
>  	} while (rq->vq->num_free);
>  	if (virtqueue_kick_prepare(rq->vq) && virtqueue_notify(rq->vq)) {
> +		local_bh_disable();
>  		u64_stats_update_begin(&rq->stats.syncp);
>  		rq->stats.kicks++;
>  		u64_stats_update_end(&rq->stats.syncp);
> +		local_bh_enable();
>  	}
>  
>  	return !oom;
> 

Re: [RFC] virtio_net: add local_bh_disable() around u64_stats_update_begin

[Toshiaki Makita]

On 2018/10/17 1:55, Sebastian Andrzej Siewior wrote:
> on 32bit, lockdep notices:
> | ================================
> | WARNING: inconsistent lock state
> | 4.19.0-rc8+ #9 Tainted: G        W
> | --------------------------------
> | inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage.
> | ip/1106 [HC0[0]:SC1[1]:HE1:SE0] takes:
> | (ptrval) (&syncp->seq#2){+.?.}, at: net_rx_action+0xc8/0x380
> | {SOFTIRQ-ON-W} state was registered at:
> |   lock_acquire+0x7e/0x170
> |   try_fill_recv+0x5fa/0x700
> |   virtnet_open+0xe0/0x180
> |   __dev_open+0xae/0x130
> |   __dev_change_flags+0x17f/0x200
> |   dev_change_flags+0x23/0x60
> |   do_setlink+0x2bb/0xa20
> |   rtnl_newlink+0x523/0x830
> |   rtnetlink_rcv_msg+0x14b/0x470
> |   netlink_rcv_skb+0x6e/0xf0
> |   rtnetlink_rcv+0xd/0x10
> |   netlink_unicast+0x16e/0x1f0
> |   netlink_sendmsg+0x1af/0x3a0
> |   ___sys_sendmsg+0x20f/0x240
> |   __sys_sendmsg+0x39/0x80
> |   sys_socketcall+0x13a/0x2a0
> |   do_int80_syscall_32+0x50/0x180
> |   restore_all+0x0/0xb2
> | irq event stamp: 3326
> | hardirqs last  enabled at (3326): [<c159e6d0>] net_rx_action+0x80/0x380
> | hardirqs last disabled at (3325): [<c159e6aa>] net_rx_action+0x5a/0x380
> | softirqs last  enabled at (3322): [<c14b440d>] virtnet_napi_enable+0xd/0x60
> | softirqs last disabled at (3323): [<c101d63d>] call_on_stack+0xd/0x50
> |
> | other info that might help us debug this:
> |  Possible unsafe locking scenario:
> |
> |        CPU0
> |        ----
> |   lock(&syncp->seq#2);
> |   <Interrupt>
> |     lock(&syncp->seq#2);
> |
> |  *** DEADLOCK ***

IIUC try_fill_recv is called only when NAPI is disabled from process
context, so there should be no point to race with virtnet_receive which
is called from NAPI handler.

I'm not sure what condition triggered this warning.


Toshiaki Makita


> 
> This is the "up" path which is not a hotpath. There is also
> refill_work().
> It might be unwise to add the local_bh_disable() to try_fill_recv()
> because if it is used mostly in BH so that local_bh_en+dis might be a
> waste of cycles.
> 
> Adding local_bh_disable() around try_fill_recv() for the non-BH call
> sites would render GFP_KERNEL pointless.
> 
> Also, ptr->var++ is not an atomic operation even on 64bit CPUs. Which
> means if try_fill_recv() runs on CPU0 (via virtnet_receive()) then the
> worker might run on CPU1.
> 
> Do we care or is this just stupid stats?  Any suggestions?
> 
> This warning appears since commit 461f03dc99cf6 ("virtio_net: Add kick stats").
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/net/virtio_net.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index dab504ec5e502..d782160cfa882 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1206,9 +1206,11 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
>  			break;
>  	} while (rq->vq->num_free);
>  	if (virtqueue_kick_prepare(rq->vq) && virtqueue_notify(rq->vq)) {
> +		local_bh_disable();
>  		u64_stats_update_begin(&rq->stats.syncp);
>  		rq->stats.kicks++;
>  		u64_stats_update_end(&rq->stats.syncp);
> +		local_bh_enable();
>  	}
>  
>  	return !oom;
> 

Re: [RFC] virtio_net: add local_bh_disable() around u64_stats_update_begin

[Jason Wang]

On 2018/10/17 上午9:13, Toshiaki Makita wrote:
> On 2018/10/17 1:55, Sebastian Andrzej Siewior wrote:
>> on 32bit, lockdep notices:
>> | ================================
>> | WARNING: inconsistent lock state
>> | 4.19.0-rc8+ #9 Tainted: G        W
>> | --------------------------------
>> | inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage.
>> | ip/1106 [HC0[0]:SC1[1]:HE1:SE0] takes:
>> | (ptrval) (&syncp->seq#2){+.?.}, at: net_rx_action+0xc8/0x380
>> | {SOFTIRQ-ON-W} state was registered at:
>> |   lock_acquire+0x7e/0x170
>> |   try_fill_recv+0x5fa/0x700
>> |   virtnet_open+0xe0/0x180
>> |   __dev_open+0xae/0x130
>> |   __dev_change_flags+0x17f/0x200
>> |   dev_change_flags+0x23/0x60
>> |   do_setlink+0x2bb/0xa20
>> |   rtnl_newlink+0x523/0x830
>> |   rtnetlink_rcv_msg+0x14b/0x470
>> |   netlink_rcv_skb+0x6e/0xf0
>> |   rtnetlink_rcv+0xd/0x10
>> |   netlink_unicast+0x16e/0x1f0
>> |   netlink_sendmsg+0x1af/0x3a0
>> |   ___sys_sendmsg+0x20f/0x240
>> |   __sys_sendmsg+0x39/0x80
>> |   sys_socketcall+0x13a/0x2a0
>> |   do_int80_syscall_32+0x50/0x180
>> |   restore_all+0x0/0xb2
>> | irq event stamp: 3326
>> | hardirqs last  enabled at (3326): [<c159e6d0>] net_rx_action+0x80/0x380
>> | hardirqs last disabled at (3325): [<c159e6aa>] net_rx_action+0x5a/0x380
>> | softirqs last  enabled at (3322): [<c14b440d>] virtnet_napi_enable+0xd/0x60
>> | softirqs last disabled at (3323): [<c101d63d>] call_on_stack+0xd/0x50
>> |
>> | other info that might help us debug this:
>> |  Possible unsafe locking scenario:
>> |
>> |        CPU0
>> |        ----
>> |   lock(&syncp->seq#2);
>> |   <Interrupt>
>> |     lock(&syncp->seq#2);
>> |
>> |  *** DEADLOCK ***
> IIUC try_fill_recv is called only when NAPI is disabled from process
> context, so there should be no point to race with virtnet_receive which
> is called from NAPI handler.
>
> I'm not sure what condition triggered this warning.
>
>
> Toshiaki Makita


Or maybe NAPI is enabled unexpectedly somewhere?

Btw, the schedule_delayed_work() in virtnet_open() is also suspicious, 
if the work is executed before virtnet_napi_enable(), there will be a 
deadloop for napi_disable().

Thanks


>
>
>> This is the "up" path which is not a hotpath. There is also
>> refill_work().
>> It might be unwise to add the local_bh_disable() to try_fill_recv()
>> because if it is used mostly in BH so that local_bh_en+dis might be a
>> waste of cycles.
>>
>> Adding local_bh_disable() around try_fill_recv() for the non-BH call
>> sites would render GFP_KERNEL pointless.
>>
>> Also, ptr->var++ is not an atomic operation even on 64bit CPUs. Which
>> means if try_fill_recv() runs on CPU0 (via virtnet_receive()) then the
>> worker might run on CPU1.
>>
>> Do we care or is this just stupid stats?  Any suggestions?
>>
>> This warning appears since commit 461f03dc99cf6 ("virtio_net: Add kick stats").
>>
>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>> ---
>>   drivers/net/virtio_net.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index dab504ec5e502..d782160cfa882 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -1206,9 +1206,11 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
>>   			break;
>>   	} while (rq->vq->num_free);
>>   	if (virtqueue_kick_prepare(rq->vq) && virtqueue_notify(rq->vq)) {
>> +		local_bh_disable();
>>   		u64_stats_update_begin(&rq->stats.syncp);
>>   		rq->stats.kicks++;
>>   		u64_stats_update_end(&rq->stats.syncp);
>> +		local_bh_enable();
>>   	}
>>   
>>   	return !oom;
>>

Re: [RFC] virtio_net: add local_bh_disable() around u64_stats_update_begin

[Jason Wang]

On 2018/10/17 上午9:13, Toshiaki Makita wrote:
> On 2018/10/17 1:55, Sebastian Andrzej Siewior wrote:
>> on 32bit, lockdep notices:
>> | ================================
>> | WARNING: inconsistent lock state
>> | 4.19.0-rc8+ #9 Tainted: G        W
>> | --------------------------------
>> | inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage.
>> | ip/1106 [HC0[0]:SC1[1]:HE1:SE0] takes:
>> | (ptrval) (&syncp->seq#2){+.?.}, at: net_rx_action+0xc8/0x380
>> | {SOFTIRQ-ON-W} state was registered at:
>> |   lock_acquire+0x7e/0x170
>> |   try_fill_recv+0x5fa/0x700
>> |   virtnet_open+0xe0/0x180
>> |   __dev_open+0xae/0x130
>> |   __dev_change_flags+0x17f/0x200
>> |   dev_change_flags+0x23/0x60
>> |   do_setlink+0x2bb/0xa20
>> |   rtnl_newlink+0x523/0x830
>> |   rtnetlink_rcv_msg+0x14b/0x470
>> |   netlink_rcv_skb+0x6e/0xf0
>> |   rtnetlink_rcv+0xd/0x10
>> |   netlink_unicast+0x16e/0x1f0
>> |   netlink_sendmsg+0x1af/0x3a0
>> |   ___sys_sendmsg+0x20f/0x240
>> |   __sys_sendmsg+0x39/0x80
>> |   sys_socketcall+0x13a/0x2a0
>> |   do_int80_syscall_32+0x50/0x180
>> |   restore_all+0x0/0xb2
>> | irq event stamp: 3326
>> | hardirqs last  enabled at (3326): [<c159e6d0>] net_rx_action+0x80/0x380
>> | hardirqs last disabled at (3325): [<c159e6aa>] net_rx_action+0x5a/0x380
>> | softirqs last  enabled at (3322): [<c14b440d>] virtnet_napi_enable+0xd/0x60
>> | softirqs last disabled at (3323): [<c101d63d>] call_on_stack+0xd/0x50
>> |
>> | other info that might help us debug this:
>> |  Possible unsafe locking scenario:
>> |
>> |        CPU0
>> |        ----
>> |   lock(&syncp->seq#2);
>> |   <Interrupt>
>> |     lock(&syncp->seq#2);
>> |
>> |  *** DEADLOCK ***
> IIUC try_fill_recv is called only when NAPI is disabled from process
> context, so there should be no point to race with virtnet_receive which
> is called from NAPI handler.
>
> I'm not sure what condition triggered this warning.
>
>
> Toshiaki Makita


Or maybe NAPI is enabled unexpectedly somewhere?

Btw, the schedule_delayed_work() in virtnet_open() is also suspicious, 
if the work is executed before virtnet_napi_enable(), there will be a 
deadloop for napi_disable().

Thanks


>
>
>> This is the "up" path which is not a hotpath. There is also
>> refill_work().
>> It might be unwise to add the local_bh_disable() to try_fill_recv()
>> because if it is used mostly in BH so that local_bh_en+dis might be a
>> waste of cycles.
>>
>> Adding local_bh_disable() around try_fill_recv() for the non-BH call
>> sites would render GFP_KERNEL pointless.
>>
>> Also, ptr->var++ is not an atomic operation even on 64bit CPUs. Which
>> means if try_fill_recv() runs on CPU0 (via virtnet_receive()) then the
>> worker might run on CPU1.
>>
>> Do we care or is this just stupid stats?  Any suggestions?
>>
>> This warning appears since commit 461f03dc99cf6 ("virtio_net: Add kick stats").
>>
>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>> ---
>>   drivers/net/virtio_net.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index dab504ec5e502..d782160cfa882 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -1206,9 +1206,11 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
>>   			break;
>>   	} while (rq->vq->num_free);
>>   	if (virtqueue_kick_prepare(rq->vq) && virtqueue_notify(rq->vq)) {
>> +		local_bh_disable();
>>   		u64_stats_update_begin(&rq->stats.syncp);
>>   		rq->stats.kicks++;
>>   		u64_stats_update_end(&rq->stats.syncp);
>> +		local_bh_enable();
>>   	}
>>   
>>   	return !oom;
>>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH] virtio_net: add local_bh_disable() around u64_stats_update_begin

[Sebastian Andrzej Siewior]

on 32bit, lockdep notices that virtnet_open() and refill_work() invoke
try_fill_recv() from process context while virtnet_receive() invokes the
same function from BH context. The problem that the seqcounter within
u64_stats_update_begin() may deadlock if it is interrupted by BH and
then acquired again.

Introduce u64_stats_update_begin_bh() which disables BH on 32bit
architectures. Since the BH might interrupt the worker, this new
function should not limited to SMP like the others which are expected
to be used in softirq.

With this change we might lose increments but this is okay. The
important part that the two 32bit parts of the 64bit counter are not
corrupted.

Fixes: 461f03dc99cf6 ("virtio_net: Add kick stats").
Suggested-by: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/net/virtio_net.c       |  4 ++--
 include/linux/u64_stats_sync.h | 16 ++++++++++++++++
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index dab504ec5e502..fbcfb4d272336 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1206,9 +1206,9 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
 			break;
 	} while (rq->vq->num_free);
 	if (virtqueue_kick_prepare(rq->vq) && virtqueue_notify(rq->vq)) {
-		u64_stats_update_begin(&rq->stats.syncp);
+		u64_stats_update_begin_bh(&rq->stats.syncp);
 		rq->stats.kicks++;
-		u64_stats_update_end(&rq->stats.syncp);
+		u64_stats_update_end_bh(&rq->stats.syncp);
 	}
 
 	return !oom;
diff --git a/include/linux/u64_stats_sync.h b/include/linux/u64_stats_sync.h
index a27604f99ed04..46b6ad6175628 100644
--- a/include/linux/u64_stats_sync.h
+++ b/include/linux/u64_stats_sync.h
@@ -90,6 +90,22 @@ static inline void u64_stats_update_end(struct u64_stats_sync *syncp)
 #endif
 }
 
+static inline void u64_stats_update_begin_bh(struct u64_stats_sync *syncp)
+{
+#if BITS_PER_LONG==32
+	local_bh_disable();
+	write_seqcount_begin(&syncp->seq);
+#endif
+}
+
+static inline void u64_stats_update_end_bh(struct u64_stats_sync *syncp)
+{
+#if BITS_PER_LONG==32
+	write_seqcount_end(&syncp->seq);
+	local_bh_enable();
+#endif
+}
+
 static inline unsigned long
 u64_stats_update_begin_irqsave(struct u64_stats_sync *syncp)
 {
-- 
2.19.1

[PATCH] virtio_net: add local_bh_disable() around u64_stats_update_begin

[Sebastian Andrzej Siewior]

on 32bit, lockdep notices that virtnet_open() and refill_work() invoke
try_fill_recv() from process context while virtnet_receive() invokes the
same function from BH context. The problem that the seqcounter within
u64_stats_update_begin() may deadlock if it is interrupted by BH and
then acquired again.

Introduce u64_stats_update_begin_bh() which disables BH on 32bit
architectures. Since the BH might interrupt the worker, this new
function should not limited to SMP like the others which are expected
to be used in softirq.

With this change we might lose increments but this is okay. The
important part that the two 32bit parts of the 64bit counter are not
corrupted.

Fixes: 461f03dc99cf6 ("virtio_net: Add kick stats").
Suggested-by: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/net/virtio_net.c       |  4 ++--
 include/linux/u64_stats_sync.h | 16 ++++++++++++++++
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index dab504ec5e502..fbcfb4d272336 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1206,9 +1206,9 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
 			break;
 	} while (rq->vq->num_free);
 	if (virtqueue_kick_prepare(rq->vq) && virtqueue_notify(rq->vq)) {
-		u64_stats_update_begin(&rq->stats.syncp);
+		u64_stats_update_begin_bh(&rq->stats.syncp);
 		rq->stats.kicks++;
-		u64_stats_update_end(&rq->stats.syncp);
+		u64_stats_update_end_bh(&rq->stats.syncp);
 	}
 
 	return !oom;
diff --git a/include/linux/u64_stats_sync.h b/include/linux/u64_stats_sync.h
index a27604f99ed04..46b6ad6175628 100644
--- a/include/linux/u64_stats_sync.h
+++ b/include/linux/u64_stats_sync.h
@@ -90,6 +90,22 @@ static inline void u64_stats_update_end(struct u64_stats_sync *syncp)
 #endif
 }
 
+static inline void u64_stats_update_begin_bh(struct u64_stats_sync *syncp)
+{
+#if BITS_PER_LONG==32
+	local_bh_disable();
+	write_seqcount_begin(&syncp->seq);
+#endif
+}
+
+static inline void u64_stats_update_end_bh(struct u64_stats_sync *syncp)
+{
+#if BITS_PER_LONG==32
+	write_seqcount_end(&syncp->seq);
+	local_bh_enable();
+#endif
+}
+
 static inline unsigned long
 u64_stats_update_begin_irqsave(struct u64_stats_sync *syncp)
 {
-- 
2.19.1

Re: [RFC] virtio_net: add local_bh_disable() around u64_stats_update_begin

[Sebastian Andrzej Siewior]

On 2018-10-17 14:48:02 [+0800], Jason Wang wrote:
> 
> On 2018/10/17 上午9:13, Toshiaki Makita wrote:
> > I'm not sure what condition triggered this warning.

If the seqlock is acquired once in softirq and then in process context
again it is enough evidence for lockdep to trigger this warning.

> > Toshiaki Makita
> 
> 
> Or maybe NAPI is enabled unexpectedly somewhere?
> 
> Btw, the schedule_delayed_work() in virtnet_open() is also suspicious, if
> the work is executed before virtnet_napi_enable(), there will be a deadloop
> for napi_disable().

something like this? It is also likely if it runs OOM on queue 2, it
will run OOM again on queue 3.

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index fbcfb4d272336..87d6ec4765270 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1263,22 +1263,22 @@ static void refill_work(struct work_struct *work)
 {
 	struct virtnet_info *vi =
 		container_of(work, struct virtnet_info, refill.work);
-	bool still_empty;
+	int still_empty = 0;
 	int i;
 
 	for (i = 0; i < vi->curr_queue_pairs; i++) {
 		struct receive_queue *rq = &vi->rq[i];
 
 		napi_disable(&rq->napi);
-		still_empty = !try_fill_recv(vi, rq, GFP_KERNEL);
+		if (!try_fill_recv(vi, rq, GFP_KERNEL))
+		    still_empty++;
 		virtnet_napi_enable(rq->vq, &rq->napi);
-
-		/* In theory, this can happen: if we don't get any buffers in
-		 * we will *never* try to fill again.
-		 */
-		if (still_empty)
-			schedule_delayed_work(&vi->refill, HZ/2);
 	}
+	/* In theory, this can happen: if we don't get any buffers in
+	 * we will *never* try to fill again.
+	 */
+	if (still_empty)
+		schedule_delayed_work(&vi->refill, HZ/2);
 }
 
 static int virtnet_receive(struct receive_queue *rq, int budget,
@@ -1407,12 +1407,13 @@ static int virtnet_open(struct net_device *dev)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
 	int i, err;
+	int need_refill = 0;
 
 	for (i = 0; i < vi->max_queue_pairs; i++) {
 		if (i < vi->curr_queue_pairs)
 			/* Make sure we have some buffers: if oom use wq. */
 			if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL))
-				schedule_delayed_work(&vi->refill, 0);
+				need_refill++;
 
 		err = xdp_rxq_info_reg(&vi->rq[i].xdp_rxq, dev, i);
 		if (err < 0)
@@ -1428,6 +1429,8 @@ static int virtnet_open(struct net_device *dev)
 		virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
 		virtnet_napi_tx_enable(vi, vi->sq[i].vq, &vi->sq[i].napi);
 	}
+	if (need_refill)
+		schedule_delayed_work(&vi->refill, 0);
 
 	return 0;
 }
@@ -2236,6 +2239,7 @@ static int virtnet_restore_up(struct virtio_device *vdev)
 {
 	struct virtnet_info *vi = vdev->priv;
 	int err, i;
+	int need_refill = 0;
 
 	err = init_vqs(vi);
 	if (err)
@@ -2246,13 +2250,15 @@ static int virtnet_restore_up(struct virtio_device *vdev)
 	if (netif_running(vi->dev)) {
 		for (i = 0; i < vi->curr_queue_pairs; i++)
 			if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL))
-				schedule_delayed_work(&vi->refill, 0);
+				need_refill++;
 
 		for (i = 0; i < vi->max_queue_pairs; i++) {
 			virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
 			virtnet_napi_tx_enable(vi, vi->sq[i].vq,
 					       &vi->sq[i].napi);
 		}
+		if (need_refill)
+			schedule_delayed_work(&vi->refill, 0);
 	}
 
 	netif_device_attach(vi->dev);

> Thanks

Sebastian

Re: [RFC] virtio_net: add local_bh_disable() around u64_stats_update_begin

[Sebastian Andrzej Siewior]

On 2018-10-17 14:48:02 [+0800], Jason Wang wrote:
> 
> On 2018/10/17 上午9:13, Toshiaki Makita wrote:
> > I'm not sure what condition triggered this warning.

If the seqlock is acquired once in softirq and then in process context
again it is enough evidence for lockdep to trigger this warning.

> > Toshiaki Makita
> 
> 
> Or maybe NAPI is enabled unexpectedly somewhere?
> 
> Btw, the schedule_delayed_work() in virtnet_open() is also suspicious, if
> the work is executed before virtnet_napi_enable(), there will be a deadloop
> for napi_disable().

something like this? It is also likely if it runs OOM on queue 2, it
will run OOM again on queue 3.

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index fbcfb4d272336..87d6ec4765270 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1263,22 +1263,22 @@ static void refill_work(struct work_struct *work)
 {
 	struct virtnet_info *vi =
 		container_of(work, struct virtnet_info, refill.work);
-	bool still_empty;
+	int still_empty = 0;
 	int i;
 
 	for (i = 0; i < vi->curr_queue_pairs; i++) {
 		struct receive_queue *rq = &vi->rq[i];
 
 		napi_disable(&rq->napi);
-		still_empty = !try_fill_recv(vi, rq, GFP_KERNEL);
+		if (!try_fill_recv(vi, rq, GFP_KERNEL))
+		    still_empty++;
 		virtnet_napi_enable(rq->vq, &rq->napi);
-
-		/* In theory, this can happen: if we don't get any buffers in
-		 * we will *never* try to fill again.
-		 */
-		if (still_empty)
-			schedule_delayed_work(&vi->refill, HZ/2);
 	}
+	/* In theory, this can happen: if we don't get any buffers in
+	 * we will *never* try to fill again.
+	 */
+	if (still_empty)
+		schedule_delayed_work(&vi->refill, HZ/2);
 }
 
 static int virtnet_receive(struct receive_queue *rq, int budget,
@@ -1407,12 +1407,13 @@ static int virtnet_open(struct net_device *dev)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
 	int i, err;
+	int need_refill = 0;
 
 	for (i = 0; i < vi->max_queue_pairs; i++) {
 		if (i < vi->curr_queue_pairs)
 			/* Make sure we have some buffers: if oom use wq. */
 			if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL))
-				schedule_delayed_work(&vi->refill, 0);
+				need_refill++;
 
 		err = xdp_rxq_info_reg(&vi->rq[i].xdp_rxq, dev, i);
 		if (err < 0)
@@ -1428,6 +1429,8 @@ static int virtnet_open(struct net_device *dev)
 		virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
 		virtnet_napi_tx_enable(vi, vi->sq[i].vq, &vi->sq[i].napi);
 	}
+	if (need_refill)
+		schedule_delayed_work(&vi->refill, 0);
 
 	return 0;
 }
@@ -2236,6 +2239,7 @@ static int virtnet_restore_up(struct virtio_device *vdev)
 {
 	struct virtnet_info *vi = vdev->priv;
 	int err, i;
+	int need_refill = 0;
 
 	err = init_vqs(vi);
 	if (err)
@@ -2246,13 +2250,15 @@ static int virtnet_restore_up(struct virtio_device *vdev)
 	if (netif_running(vi->dev)) {
 		for (i = 0; i < vi->curr_queue_pairs; i++)
 			if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL))
-				schedule_delayed_work(&vi->refill, 0);
+				need_refill++;
 
 		for (i = 0; i < vi->max_queue_pairs; i++) {
 			virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
 			virtnet_napi_tx_enable(vi, vi->sq[i].vq,
 					       &vi->sq[i].napi);
 		}
+		if (need_refill)
+			schedule_delayed_work(&vi->refill, 0);
 	}
 
 	netif_device_attach(vi->dev);

> Thanks

Sebastian
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC] virtio_net: add local_bh_disable() around u64_stats_update_begin

[Toshiaki Makita]

On 2018/10/18 17:47, Sebastian Andrzej Siewior wrote:
> On 2018-10-17 14:48:02 [+0800], Jason Wang wrote:
>>
>> On 2018/10/17 上午9:13, Toshiaki Makita wrote:
>>> I'm not sure what condition triggered this warning.
> 
> If the seqlock is acquired once in softirq and then in process context
> again it is enough evidence for lockdep to trigger this warning.

No. As I said that should not happen because of NAPI guard.

-- 
Toshiaki Makita

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio_net: add local_bh_disable() around u64_stats_update_begin

[Toshiaki Makita]

On 2018/10/18 17:43, Sebastian Andrzej Siewior wrote:
> on 32bit, lockdep notices that virtnet_open() and refill_work() invoke
> try_fill_recv() from process context while virtnet_receive() invokes the
> same function from BH context. The problem that the seqcounter within
> u64_stats_update_begin() may deadlock if it is interrupted by BH and
> then acquired again.
> 
> Introduce u64_stats_update_begin_bh() which disables BH on 32bit
> architectures. Since the BH might interrupt the worker, this new
> function should not limited to SMP like the others which are expected
> to be used in softirq.
> 
> With this change we might lose increments but this is okay. The
> important part that the two 32bit parts of the 64bit counter are not
> corrupted.
> 
> Fixes: 461f03dc99cf6 ("virtio_net: Add kick stats").
> Suggested-by: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

NACK. Again, this race should not happen because of NAPI guard.
We need to investigate why this warning happened.

-- 
Toshiaki Makita

Re: [RFC] virtio_net: add local_bh_disable() around u64_stats_update_begin

[Sebastian Andrzej Siewior]

On 2018-10-18 18:00:05 [+0900], Toshiaki Makita wrote:
> On 2018/10/18 17:47, Sebastian Andrzej Siewior wrote:
> > On 2018-10-17 14:48:02 [+0800], Jason Wang wrote:
> >>
> >> On 2018/10/17 上午9:13, Toshiaki Makita wrote:
> >>> I'm not sure what condition triggered this warning.
> > 
> > If the seqlock is acquired once in softirq and then in process context
> > again it is enough evidence for lockdep to trigger this warning.
> 
> No. As I said that should not happen because of NAPI guard.
Again: lockdep saw the lock in softirq context once and in process
context once and this is what triggers the warning. It does not matter
if NAPI is enabled or not during the access in process context. If you
want to allow this you need further lockdep annotation…

… but: refill_work() disables NAPI for &vi->rq[1] and refills + updates
stats while NAPI is enabled for &vi->rq[0].

Sebastian

Re: [RFC] virtio_net: add local_bh_disable() around u64_stats_update_begin

[Sebastian Andrzej Siewior]

On 2018-10-18 18:00:05 [+0900], Toshiaki Makita wrote:
> On 2018/10/18 17:47, Sebastian Andrzej Siewior wrote:
> > On 2018-10-17 14:48:02 [+0800], Jason Wang wrote:
> >>
> >> On 2018/10/17 上午9:13, Toshiaki Makita wrote:
> >>> I'm not sure what condition triggered this warning.
> > 
> > If the seqlock is acquired once in softirq and then in process context
> > again it is enough evidence for lockdep to trigger this warning.
> 
> No. As I said that should not happen because of NAPI guard.
Again: lockdep saw the lock in softirq context once and in process
context once and this is what triggers the warning. It does not matter
if NAPI is enabled or not during the access in process context. If you
want to allow this you need further lockdep annotation…

… but: refill_work() disables NAPI for &vi->rq[1] and refills + updates
stats while NAPI is enabled for &vi->rq[0].

Sebastian
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio_net: add local_bh_disable() around u64_stats_update_begin

[Sebastian Andrzej Siewior]

On 2018-10-18 18:06:57 [+0900], Toshiaki Makita wrote:
> NACK. Again, this race should not happen because of NAPI guard.
> We need to investigate why this warning happened.

I tried to explain this. Please see
	20181018090812.rry5qgnqxxrjxaii@linutronix.de

Sebastian

Re: [PATCH] virtio_net: add local_bh_disable() around u64_stats_update_begin

[Sebastian Andrzej Siewior]

On 2018-10-18 18:06:57 [+0900], Toshiaki Makita wrote:
> NACK. Again, this race should not happen because of NAPI guard.
> We need to investigate why this warning happened.

I tried to explain this. Please see
	20181018090812.rry5qgnqxxrjxaii@linutronix.de

Sebastian

Re: [RFC] virtio_net: add local_bh_disable() around u64_stats_update_begin

[Toshiaki Makita]

On 2018/10/18 18:08, Sebastian Andrzej Siewior wrote:
> On 2018-10-18 18:00:05 [+0900], Toshiaki Makita wrote:
>> On 2018/10/18 17:47, Sebastian Andrzej Siewior wrote:
>>> On 2018-10-17 14:48:02 [+0800], Jason Wang wrote:
>>>>
>>>> On 2018/10/17 上午9:13, Toshiaki Makita wrote:
>>>>> I'm not sure what condition triggered this warning.
>>>
>>> If the seqlock is acquired once in softirq and then in process context
>>> again it is enough evidence for lockdep to trigger this warning.
>>
>> No. As I said that should not happen because of NAPI guard.
> Again: lockdep saw the lock in softirq context once and in process
> context once and this is what triggers the warning. It does not matter
> if NAPI is enabled or not during the access in process context. If you
> want to allow this you need further lockdep annotation…
> 
> … but: refill_work() disables NAPI for &vi->rq[1] and refills + updates
> stats while NAPI is enabled for &vi->rq[0].

Do you mean this is false positive? rq[0] and rq[1] never race with each
other...

-- 
Toshiaki Makita

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio_net: add local_bh_disable() around u64_stats_update_begin

[Toshiaki Makita]

On 2018/10/18 18:11, Sebastian Andrzej Siewior wrote:
> On 2018-10-18 18:06:57 [+0900], Toshiaki Makita wrote:
>> NACK. Again, this race should not happen because of NAPI guard.
>> We need to investigate why this warning happened.
> 
> I tried to explain this. Please see
> 	20181018090812.rry5qgnqxxrjxaii@linutronix.de
Anyway, if this really happens, then it means try_fill_recv() can be
called concurrently for the same rq, which looks like a far more severe
problem to me. If so, we need to fix it instead.

-- 
Toshiaki Makita

Re: [RFC] virtio_net: add local_bh_disable() around u64_stats_update_begin

[Sebastian Andrzej Siewior]

On 2018-10-18 18:19:21 [+0900], Toshiaki Makita wrote:
> On 2018/10/18 18:08, Sebastian Andrzej Siewior wrote:
> > Again: lockdep saw the lock in softirq context once and in process
> > context once and this is what triggers the warning. It does not matter
> > if NAPI is enabled or not during the access in process context. If you
> > want to allow this you need further lockdep annotation…
> > 
> > … but: refill_work() disables NAPI for &vi->rq[1] and refills + updates
> > stats while NAPI is enabled for &vi->rq[0].
> 
> Do you mean this is false positive? rq[0] and rq[1] never race with each
> other...

Why? So you can't refill rq[1] and then be interrupted and process NAPI
for rq[0]?

But as I said. If lockdep saw the lock in acquired in softirq (what it
did) _and_ in process context (what it did as well) _once_ then this is
enough evidence for the warning.
If you claim that this can not happen due to NAPI guard [0] then this is
something lockdep does not know about.

[0] which I currently don't understand and therefore sent the patch [1]
    as Jason pointed out that in the ->ndo_open case the work is
    scheduled and then NAPI is enabled (which means the worker could
    disable NAPI and refill but before it finishes, ->ndo_open would
    continue and enable NAPI)).
[1] 20181018084753.wefvsypdevbzoadg@linutronix.de

Sebastian

Re: [RFC] virtio_net: add local_bh_disable() around u64_stats_update_begin

[Sebastian Andrzej Siewior]

On 2018-10-18 18:19:21 [+0900], Toshiaki Makita wrote:
> On 2018/10/18 18:08, Sebastian Andrzej Siewior wrote:
> > Again: lockdep saw the lock in softirq context once and in process
> > context once and this is what triggers the warning. It does not matter
> > if NAPI is enabled or not during the access in process context. If you
> > want to allow this you need further lockdep annotation…
> > 
> > … but: refill_work() disables NAPI for &vi->rq[1] and refills + updates
> > stats while NAPI is enabled for &vi->rq[0].
> 
> Do you mean this is false positive? rq[0] and rq[1] never race with each
> other...

Why? So you can't refill rq[1] and then be interrupted and process NAPI
for rq[0]?

But as I said. If lockdep saw the lock in acquired in softirq (what it
did) _and_ in process context (what it did as well) _once_ then this is
enough evidence for the warning.
If you claim that this can not happen due to NAPI guard [0] then this is
something lockdep does not know about.

[0] which I currently don't understand and therefore sent the patch [1]
    as Jason pointed out that in the ->ndo_open case the work is
    scheduled and then NAPI is enabled (which means the worker could
    disable NAPI and refill but before it finishes, ->ndo_open would
    continue and enable NAPI)).
[1] 20181018084753.wefvsypdevbzoadg@linutronix.de

Sebastian
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC] virtio_net: add local_bh_disable() around u64_stats_update_begin

[Toshiaki Makita]

On 2018/10/18 18:30, Sebastian Andrzej Siewior wrote:
> On 2018-10-18 18:19:21 [+0900], Toshiaki Makita wrote:
>> On 2018/10/18 18:08, Sebastian Andrzej Siewior wrote:
>>> Again: lockdep saw the lock in softirq context once and in process
>>> context once and this is what triggers the warning. It does not matter
>>> if NAPI is enabled or not during the access in process context. If you
>>> want to allow this you need further lockdep annotation…
>>>
>>> … but: refill_work() disables NAPI for &vi->rq[1] and refills + updates
>>> stats while NAPI is enabled for &vi->rq[0].
>>
>> Do you mean this is false positive? rq[0] and rq[1] never race with each
>> other...
> 
> Why? So you can't refill rq[1] and then be interrupted and process NAPI
> for rq[0]?

Probably I don't understand what you are trying to say, but rq[0] and
rq[1] are different objects so they can be processed concurrently but
should not affect each other.

> 
> But as I said. If lockdep saw the lock in acquired in softirq (what it
> did) _and_ in process context (what it did as well) _once_ then this is
> enough evidence for the warning.
> If you claim that this can not happen due to NAPI guard [0] then this is
> something lockdep does not know about.

The point is that it is not the problem of stats. try_fill_recv() should
not be called for the same rq concurrently in the first place. This
requirement should be satisfied by NAPI guard, so if the NAPI guard
logic is buggy then we need to fix it.

> [0] which I currently don't understand and therefore sent the patch [1]
>     as Jason pointed out that in the ->ndo_open case the work is
>     scheduled and then NAPI is enabled (which means the worker could
>     disable NAPI and refill but before it finishes, ->ndo_open would
>     continue and enable NAPI)).

It may be related but I have not investigated it deeply. I'll check if
it can cause this problem later.

> [1] 20181018084753.wefvsypdevbzoadg@linutronix.de

-- 
Toshiaki Makita

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC] virtio_net: add local_bh_disable() around u64_stats_update_begin

[Rafael David Tinoco]

On Thu, Oct 18, 2018 at 6:19 AM, Toshiaki Makita
<makita.toshiaki@lab.ntt.co.jp> wrote:
> On 2018/10/18 18:08, Sebastian Andrzej Siewior wrote:
>> On 2018-10-18 18:00:05 [+0900], Toshiaki Makita wrote:
>>> On 2018/10/18 17:47, Sebastian Andrzej Siewior wrote:
>>>> On 2018-10-17 14:48:02 [+0800], Jason Wang wrote:
>>>>>
>>>>> On 2018/10/17 上午9:13, Toshiaki Makita wrote:
>>>>>> I'm not sure what condition triggered this warning.
>>>>
>>>> If the seqlock is acquired once in softirq and then in process context
>>>> again it is enough evidence for lockdep to trigger this warning.
>>>
>>> No. As I said that should not happen because of NAPI guard.
>> Again: lockdep saw the lock in softirq context once and in process
>> context once and this is what triggers the warning. It does not matter
>> if NAPI is enabled or not during the access in process context. If you
>> want to allow this you need further lockdep annotation…
>>
>> … but: refill_work() disables NAPI for &vi->rq[1] and refills + updates
>> stats while NAPI is enabled for &vi->rq[0].
>
> Do you mean this is false positive? rq[0] and rq[1] never race with each
> other...
>

I just came to this thread after having the same "false positive"
warning on an armhf kvm guest dmesg.

It appears to me that, at least for my case, the sequence:

u64_stats_update_begin() -> write_seqcount_begin() ->
write_seqcount_begin_nested() -> raw_write_seqcount_begin()

is only incrementing s->sequence++. With that, whenever we have:

CONFIG_TRACE_IRQ_FLAGS and CONFIG_DEBUG_LOCK_ALLOC enabled

we might face this false-positive warning since there are no locks,
but just a sequencer, right ? So, Having a barrier, after incrementing
the sequence, like I have now, won't block the other context to
"acquire" the "same lock" (not a lock for this particular case)
warning done in "seqcount_acquire()".

Hope this helps the discussion.

Link: https://bugs.linaro.org/show_bug.cgi?id=4027

Thank

Rafael Tinoco

Re: [PATCH] virtio_net: add local_bh_disable() around u64_stats_update_begin

[David Miller]

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Date: Thu, 18 Oct 2018 10:43:13 +0200

> on 32bit, lockdep notices that virtnet_open() and refill_work() invoke
> try_fill_recv() from process context while virtnet_receive() invokes the
> same function from BH context. The problem that the seqcounter within
> u64_stats_update_begin() may deadlock if it is interrupted by BH and
> then acquired again.
> 
> Introduce u64_stats_update_begin_bh() which disables BH on 32bit
> architectures. Since the BH might interrupt the worker, this new
> function should not limited to SMP like the others which are expected
> to be used in softirq.
> 
> With this change we might lose increments but this is okay. The
> important part that the two 32bit parts of the 64bit counter are not
> corrupted.
> 
> Fixes: 461f03dc99cf6 ("virtio_net: Add kick stats").
> Suggested-by: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Trying to get down to the bottom of this:

1) virtnet_receive() runs from softirq but only if NAPI is active and
   enabled.  It is in this context that it invokes try_fill_recv().

2) refill_work() runs from process context, but disables NAPI (and
   thus invocation of virtnet_receive()) before calling
   try_fill_recv().

3) virtnet_open() invokes from process context as well, but before the
   NAPI instances are enabled, it is same as case #2.

4) virtnet_restore_up() is the same situations as #3.

Therefore I agree that this is a false positive, and simply lockdep
cannot see the NAPI synchronization done by case #2.

I think we shouldn't add unnecessary BH disabling here, and instead
find some way to annotate this for lockdep's sake.

Thank you.

Re: [RFC] virtio_net: add local_bh_disable() around u64_stats_update_begin

[Jason Wang]

On 2018/10/18 下午4:47, Sebastian Andrzej Siewior wrote:
> On 2018-10-17 14:48:02 [+0800], Jason Wang wrote:
>> On 2018/10/17 上午9:13, Toshiaki Makita wrote:
>>> I'm not sure what condition triggered this warning.
> If the seqlock is acquired once in softirq and then in process context
> again it is enough evidence for lockdep to trigger this warning.
>
>>> Toshiaki Makita
>>
>> Or maybe NAPI is enabled unexpectedly somewhere?
>>
>> Btw, the schedule_delayed_work() in virtnet_open() is also suspicious, if
>> the work is executed before virtnet_napi_enable(), there will be a deadloop
>> for napi_disable().
> something like this? It is also likely if it runs OOM on queue 2, it
> will run OOM again on queue 3.
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index fbcfb4d272336..87d6ec4765270 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1263,22 +1263,22 @@ static void refill_work(struct work_struct *work)
>   {
>   	struct virtnet_info *vi =
>   		container_of(work, struct virtnet_info, refill.work);
> -	bool still_empty;
> +	int still_empty = 0;
>   	int i;
>   
>   	for (i = 0; i < vi->curr_queue_pairs; i++) {
>   		struct receive_queue *rq = &vi->rq[i];
>   
>   		napi_disable(&rq->napi);
> -		still_empty = !try_fill_recv(vi, rq, GFP_KERNEL);
> +		if (!try_fill_recv(vi, rq, GFP_KERNEL))
> +		    still_empty++;
>   		virtnet_napi_enable(rq->vq, &rq->napi);
> -
> -		/* In theory, this can happen: if we don't get any buffers in
> -		 * we will *never* try to fill again.
> -		 */
> -		if (still_empty)
> -			schedule_delayed_work(&vi->refill, HZ/2);
>   	}
> +	/* In theory, this can happen: if we don't get any buffers in
> +	 * we will *never* try to fill again.
> +	 */
> +	if (still_empty)
> +		schedule_delayed_work(&vi->refill, HZ/2);
>   }


I think this part is not a must or an independent optimization?


Thanks


>   
>   static int virtnet_receive(struct receive_queue *rq, int budget,
> @@ -1407,12 +1407,13 @@ static int virtnet_open(struct net_device *dev)
>   {
>   	struct virtnet_info *vi = netdev_priv(dev);
>   	int i, err;
> +	int need_refill = 0;
>   
>   	for (i = 0; i < vi->max_queue_pairs; i++) {
>   		if (i < vi->curr_queue_pairs)
>   			/* Make sure we have some buffers: if oom use wq. */
>   			if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL))
> -				schedule_delayed_work(&vi->refill, 0);
> +				need_refill++;
>   
>   		err = xdp_rxq_info_reg(&vi->rq[i].xdp_rxq, dev, i);
>   		if (err < 0)
> @@ -1428,6 +1429,8 @@ static int virtnet_open(struct net_device *dev)
>   		virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
>   		virtnet_napi_tx_enable(vi, vi->sq[i].vq, &vi->sq[i].napi);
>   	}
> +	if (need_refill)
> +		schedule_delayed_work(&vi->refill, 0);
>   
>   	return 0;
>   }
> @@ -2236,6 +2239,7 @@ static int virtnet_restore_up(struct virtio_device *vdev)
>   {
>   	struct virtnet_info *vi = vdev->priv;
>   	int err, i;
> +	int need_refill = 0;
>   
>   	err = init_vqs(vi);
>   	if (err)
> @@ -2246,13 +2250,15 @@ static int virtnet_restore_up(struct virtio_device *vdev)
>   	if (netif_running(vi->dev)) {
>   		for (i = 0; i < vi->curr_queue_pairs; i++)
>   			if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL))
> -				schedule_delayed_work(&vi->refill, 0);
> +				need_refill++;
>   
>   		for (i = 0; i < vi->max_queue_pairs; i++) {
>   			virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
>   			virtnet_napi_tx_enable(vi, vi->sq[i].vq,
>   					       &vi->sq[i].napi);
>   		}
> +		if (need_refill)
> +			schedule_delayed_work(&vi->refill, 0);
>   	}
>   
>   	netif_device_attach(vi->dev);
>
>> Thanks
> Sebastian

Re: [RFC] virtio_net: add local_bh_disable() around u64_stats_update_begin

[Jason Wang]

On 2018/10/18 下午4:47, Sebastian Andrzej Siewior wrote:
> On 2018-10-17 14:48:02 [+0800], Jason Wang wrote:
>> On 2018/10/17 上午9:13, Toshiaki Makita wrote:
>>> I'm not sure what condition triggered this warning.
> If the seqlock is acquired once in softirq and then in process context
> again it is enough evidence for lockdep to trigger this warning.
>
>>> Toshiaki Makita
>>
>> Or maybe NAPI is enabled unexpectedly somewhere?
>>
>> Btw, the schedule_delayed_work() in virtnet_open() is also suspicious, if
>> the work is executed before virtnet_napi_enable(), there will be a deadloop
>> for napi_disable().
> something like this? It is also likely if it runs OOM on queue 2, it
> will run OOM again on queue 3.
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index fbcfb4d272336..87d6ec4765270 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1263,22 +1263,22 @@ static void refill_work(struct work_struct *work)
>   {
>   	struct virtnet_info *vi =
>   		container_of(work, struct virtnet_info, refill.work);
> -	bool still_empty;
> +	int still_empty = 0;
>   	int i;
>   
>   	for (i = 0; i < vi->curr_queue_pairs; i++) {
>   		struct receive_queue *rq = &vi->rq[i];
>   
>   		napi_disable(&rq->napi);
> -		still_empty = !try_fill_recv(vi, rq, GFP_KERNEL);
> +		if (!try_fill_recv(vi, rq, GFP_KERNEL))
> +		    still_empty++;
>   		virtnet_napi_enable(rq->vq, &rq->napi);
> -
> -		/* In theory, this can happen: if we don't get any buffers in
> -		 * we will *never* try to fill again.
> -		 */
> -		if (still_empty)
> -			schedule_delayed_work(&vi->refill, HZ/2);
>   	}
> +	/* In theory, this can happen: if we don't get any buffers in
> +	 * we will *never* try to fill again.
> +	 */
> +	if (still_empty)
> +		schedule_delayed_work(&vi->refill, HZ/2);
>   }


I think this part is not a must or an independent optimization?


Thanks


>   
>   static int virtnet_receive(struct receive_queue *rq, int budget,
> @@ -1407,12 +1407,13 @@ static int virtnet_open(struct net_device *dev)
>   {
>   	struct virtnet_info *vi = netdev_priv(dev);
>   	int i, err;
> +	int need_refill = 0;
>   
>   	for (i = 0; i < vi->max_queue_pairs; i++) {
>   		if (i < vi->curr_queue_pairs)
>   			/* Make sure we have some buffers: if oom use wq. */
>   			if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL))
> -				schedule_delayed_work(&vi->refill, 0);
> +				need_refill++;
>   
>   		err = xdp_rxq_info_reg(&vi->rq[i].xdp_rxq, dev, i);
>   		if (err < 0)
> @@ -1428,6 +1429,8 @@ static int virtnet_open(struct net_device *dev)
>   		virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
>   		virtnet_napi_tx_enable(vi, vi->sq[i].vq, &vi->sq[i].napi);
>   	}
> +	if (need_refill)
> +		schedule_delayed_work(&vi->refill, 0);
>   
>   	return 0;
>   }
> @@ -2236,6 +2239,7 @@ static int virtnet_restore_up(struct virtio_device *vdev)
>   {
>   	struct virtnet_info *vi = vdev->priv;
>   	int err, i;
> +	int need_refill = 0;
>   
>   	err = init_vqs(vi);
>   	if (err)
> @@ -2246,13 +2250,15 @@ static int virtnet_restore_up(struct virtio_device *vdev)
>   	if (netif_running(vi->dev)) {
>   		for (i = 0; i < vi->curr_queue_pairs; i++)
>   			if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL))
> -				schedule_delayed_work(&vi->refill, 0);
> +				need_refill++;
>   
>   		for (i = 0; i < vi->max_queue_pairs; i++) {
>   			virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
>   			virtnet_napi_tx_enable(vi, vi->sq[i].vq,
>   					       &vi->sq[i].napi);
>   		}
> +		if (need_refill)
> +			schedule_delayed_work(&vi->refill, 0);
>   	}
>   
>   	netif_device_attach(vi->dev);
>
>> Thanks
> Sebastian
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio_net: add local_bh_disable() around u64_stats_update_begin

[Jason Wang]

On 2018/10/19 上午7:23, David Miller wrote:
> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Date: Thu, 18 Oct 2018 10:43:13 +0200
>
>> on 32bit, lockdep notices that virtnet_open() and refill_work() invoke
>> try_fill_recv() from process context while virtnet_receive() invokes the
>> same function from BH context. The problem that the seqcounter within
>> u64_stats_update_begin() may deadlock if it is interrupted by BH and
>> then acquired again.
>>
>> Introduce u64_stats_update_begin_bh() which disables BH on 32bit
>> architectures. Since the BH might interrupt the worker, this new
>> function should not limited to SMP like the others which are expected
>> to be used in softirq.
>>
>> With this change we might lose increments but this is okay. The
>> important part that the two 32bit parts of the 64bit counter are not
>> corrupted.
>>
>> Fixes: 461f03dc99cf6 ("virtio_net: Add kick stats").
>> Suggested-by: Stephen Hemminger <stephen@networkplumber.org>
>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Trying to get down to the bottom of this:
>
> 1) virtnet_receive() runs from softirq but only if NAPI is active and
>     enabled.  It is in this context that it invokes try_fill_recv().
>
> 2) refill_work() runs from process context, but disables NAPI (and
>     thus invocation of virtnet_receive()) before calling
>     try_fill_recv().
>
> 3) virtnet_open() invokes from process context as well, but before the
>     NAPI instances are enabled, it is same as case #2.
>
> 4) virtnet_restore_up() is the same situations as #3.
>
> Therefore I agree that this is a false positive, and simply lockdep
> cannot see the NAPI synchronization done by case #2.
>
> I think we shouldn't add unnecessary BH disabling here, and instead
> find some way to annotate this for lockdep's sake.
>
> Thank you.
>

+1

Thanks

Re: [PATCH] virtio_net: add local_bh_disable() around u64_stats_update_begin

[Jason Wang]

On 2018/10/19 上午7:23, David Miller wrote:
> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Date: Thu, 18 Oct 2018 10:43:13 +0200
>
>> on 32bit, lockdep notices that virtnet_open() and refill_work() invoke
>> try_fill_recv() from process context while virtnet_receive() invokes the
>> same function from BH context. The problem that the seqcounter within
>> u64_stats_update_begin() may deadlock if it is interrupted by BH and
>> then acquired again.
>>
>> Introduce u64_stats_update_begin_bh() which disables BH on 32bit
>> architectures. Since the BH might interrupt the worker, this new
>> function should not limited to SMP like the others which are expected
>> to be used in softirq.
>>
>> With this change we might lose increments but this is okay. The
>> important part that the two 32bit parts of the 64bit counter are not
>> corrupted.
>>
>> Fixes: 461f03dc99cf6 ("virtio_net: Add kick stats").
>> Suggested-by: Stephen Hemminger <stephen@networkplumber.org>
>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Trying to get down to the bottom of this:
>
> 1) virtnet_receive() runs from softirq but only if NAPI is active and
>     enabled.  It is in this context that it invokes try_fill_recv().
>
> 2) refill_work() runs from process context, but disables NAPI (and
>     thus invocation of virtnet_receive()) before calling
>     try_fill_recv().
>
> 3) virtnet_open() invokes from process context as well, but before the
>     NAPI instances are enabled, it is same as case #2.
>
> 4) virtnet_restore_up() is the same situations as #3.
>
> Therefore I agree that this is a false positive, and simply lockdep
> cannot see the NAPI synchronization done by case #2.
>
> I think we shouldn't add unnecessary BH disabling here, and instead
> find some way to annotate this for lockdep's sake.
>
> Thank you.
>

+1

Thanks

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[RFC] virtio_net: add local_bh_disable() around u64_stats_update_begin

[Sebastian Andrzej Siewior]

on 32bit, lockdep notices:
| ================================
| WARNING: inconsistent lock state
| 4.19.0-rc8+ #9 Tainted: G        W
| --------------------------------
| inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage.
| ip/1106 [HC0[0]:SC1[1]:HE1:SE0] takes:
| (ptrval) (&syncp->seq#2){+.?.}, at: net_rx_action+0xc8/0x380
| {SOFTIRQ-ON-W} state was registered at:
|   lock_acquire+0x7e/0x170
|   try_fill_recv+0x5fa/0x700
|   virtnet_open+0xe0/0x180
|   __dev_open+0xae/0x130
|   __dev_change_flags+0x17f/0x200
|   dev_change_flags+0x23/0x60
|   do_setlink+0x2bb/0xa20
|   rtnl_newlink+0x523/0x830
|   rtnetlink_rcv_msg+0x14b/0x470
|   netlink_rcv_skb+0x6e/0xf0
|   rtnetlink_rcv+0xd/0x10
|   netlink_unicast+0x16e/0x1f0
|   netlink_sendmsg+0x1af/0x3a0
|   ___sys_sendmsg+0x20f/0x240
|   __sys_sendmsg+0x39/0x80
|   sys_socketcall+0x13a/0x2a0
|   do_int80_syscall_32+0x50/0x180
|   restore_all+0x0/0xb2
| irq event stamp: 3326
| hardirqs last  enabled at (3326): [<c159e6d0>] net_rx_action+0x80/0x380
| hardirqs last disabled at (3325): [<c159e6aa>] net_rx_action+0x5a/0x380
| softirqs last  enabled at (3322): [<c14b440d>] virtnet_napi_enable+0xd/0x60
| softirqs last disabled at (3323): [<c101d63d>] call_on_stack+0xd/0x50
|
| other info that might help us debug this:
|  Possible unsafe locking scenario:
|
|        CPU0
|        ----
|   lock(&syncp->seq#2);
|   <Interrupt>
|     lock(&syncp->seq#2);
|
|  *** DEADLOCK ***

This is the "up" path which is not a hotpath. There is also
refill_work().
It might be unwise to add the local_bh_disable() to try_fill_recv()
because if it is used mostly in BH so that local_bh_en+dis might be a
waste of cycles.

Adding local_bh_disable() around try_fill_recv() for the non-BH call
sites would render GFP_KERNEL pointless.

Also, ptr->var++ is not an atomic operation even on 64bit CPUs. Which
means if try_fill_recv() runs on CPU0 (via virtnet_receive()) then the
worker might run on CPU1.

Do we care or is this just stupid stats?  Any suggestions?

This warning appears since commit 461f03dc99cf6 ("virtio_net: Add kick stats").

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/net/virtio_net.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index dab504ec5e502..d782160cfa882 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1206,9 +1206,11 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
 			break;
 	} while (rq->vq->num_free);
 	if (virtqueue_kick_prepare(rq->vq) && virtqueue_notify(rq->vq)) {
+		local_bh_disable();
 		u64_stats_update_begin(&rq->stats.syncp);
 		rq->stats.kicks++;
 		u64_stats_update_end(&rq->stats.syncp);
+		local_bh_enable();
 	}
 
 	return !oom;
-- 
2.19.1