* [PATCH] virtio_net: fix lockdep warning on 32 bit
@ 2020-05-06 0:01 ` Michael S. Tsirkin
0 siblings, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2020-05-06 0:01 UTC (permalink / raw)
To: linux-kernel
Cc: Thomas Gleixner, Eric Dumazet, Jason Wang, David S. Miller,
virtualization, netdev
When we fill up a receive VQ, try_fill_recv currently tries to count
kicks using a 64 bit stats counter. Turns out, on a 32 bit kernel that
uses a seqcount. sequence counts are "lock" constructs where you need to
make sure that writers are serialized.
In turn, this means that we mustn't run two try_fill_recv concurrently.
Which of course we don't. We do run try_fill_recv sometimes from a fully
preemptible context and sometimes from a softirq (napi) context.
However, when it comes to the seqcount, lockdep is trying to enforce
the rule that the same lock isn't accessed from preemptible
and softirq context. This causes a false-positive warning:
WARNING: inconsistent lock state
...
inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage.
As a work around, shut down the warning by switching
to u64_stats_update_begin_irqsave - that works by disabling
interrupts on 32 bit only, is a NOP on 64 bit.
Reported-by: Thomas Gleixner <tglx@linutronix.de>
Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
I'm not thrilled about this but this seems the best we can do for now.
Completely untested.
Thomas, can you pls let me know the config I need to trigger the warning
in question?
drivers/net/virtio_net.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 6594aab4910e..95393b61187f 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1243,9 +1243,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)) {
- u64_stats_update_begin(&rq->stats.syncp);
+ unsigned long flags;
+
+ flags = u64_stats_update_begin_irqsave(&rq->stats.syncp);
rq->stats.kicks++;
- u64_stats_update_end(&rq->stats.syncp);
+ u64_stats_update_end_irqrestore(&rq->stats.syncp);
}
return !oom;
--
MST
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] virtio_net: fix lockdep warning on 32 bit
@ 2020-05-06 0:01 ` Michael S. Tsirkin
0 siblings, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2020-05-06 0:01 UTC (permalink / raw)
To: linux-kernel
Cc: Eric Dumazet, netdev, virtualization, Thomas Gleixner, David S. Miller
When we fill up a receive VQ, try_fill_recv currently tries to count
kicks using a 64 bit stats counter. Turns out, on a 32 bit kernel that
uses a seqcount. sequence counts are "lock" constructs where you need to
make sure that writers are serialized.
In turn, this means that we mustn't run two try_fill_recv concurrently.
Which of course we don't. We do run try_fill_recv sometimes from a fully
preemptible context and sometimes from a softirq (napi) context.
However, when it comes to the seqcount, lockdep is trying to enforce
the rule that the same lock isn't accessed from preemptible
and softirq context. This causes a false-positive warning:
WARNING: inconsistent lock state
...
inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage.
As a work around, shut down the warning by switching
to u64_stats_update_begin_irqsave - that works by disabling
interrupts on 32 bit only, is a NOP on 64 bit.
Reported-by: Thomas Gleixner <tglx@linutronix.de>
Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
I'm not thrilled about this but this seems the best we can do for now.
Completely untested.
Thomas, can you pls let me know the config I need to trigger the warning
in question?
drivers/net/virtio_net.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 6594aab4910e..95393b61187f 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1243,9 +1243,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)) {
- u64_stats_update_begin(&rq->stats.syncp);
+ unsigned long flags;
+
+ flags = u64_stats_update_begin_irqsave(&rq->stats.syncp);
rq->stats.kicks++;
- u64_stats_update_end(&rq->stats.syncp);
+ u64_stats_update_end_irqrestore(&rq->stats.syncp);
}
return !oom;
--
MST
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] virtio_net: fix lockdep warning on 32 bit
2020-05-06 0:01 ` Michael S. Tsirkin
@ 2020-05-06 5:28 ` Jason Wang
-1 siblings, 0 replies; 6+ messages in thread
From: Jason Wang @ 2020-05-06 5:28 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel
Cc: Thomas Gleixner, Eric Dumazet, David S. Miller, virtualization, netdev
On 2020/5/6 上午8:01, Michael S. Tsirkin wrote:
> When we fill up a receive VQ, try_fill_recv currently tries to count
> kicks using a 64 bit stats counter. Turns out, on a 32 bit kernel that
> uses a seqcount. sequence counts are "lock" constructs where you need to
> make sure that writers are serialized.
>
> In turn, this means that we mustn't run two try_fill_recv concurrently.
> Which of course we don't. We do run try_fill_recv sometimes from a fully
> preemptible context and sometimes from a softirq (napi) context.
>
> However, when it comes to the seqcount, lockdep is trying to enforce
> the rule that the same lock isn't accessed from preemptible
> and softirq context. This causes a false-positive warning:
>
> WARNING: inconsistent lock state
> ...
> inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage.
>
> As a work around, shut down the warning by switching
> to u64_stats_update_begin_irqsave - that works by disabling
> interrupts on 32 bit only, is a NOP on 64 bit.
>
> Reported-by: Thomas Gleixner <tglx@linutronix.de>
> Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> I'm not thrilled about this but this seems the best we can do for now.
>
> Completely untested.
>
>
> Thomas, can you pls let me know the config I need to trigger the warning
> in question?
>
>
> drivers/net/virtio_net.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 6594aab4910e..95393b61187f 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1243,9 +1243,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)) {
> - u64_stats_update_begin(&rq->stats.syncp);
> + unsigned long flags;
> +
> + flags = u64_stats_update_begin_irqsave(&rq->stats.syncp);
> rq->stats.kicks++;
> - u64_stats_update_end(&rq->stats.syncp);
> + u64_stats_update_end_irqrestore(&rq->stats.syncp);
> }
>
> return !oom;
Acked-by: Jason Wang <jasowang@redhat.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] virtio_net: fix lockdep warning on 32 bit
@ 2020-05-06 5:28 ` Jason Wang
0 siblings, 0 replies; 6+ messages in thread
From: Jason Wang @ 2020-05-06 5:28 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel
Cc: netdev, Thomas Gleixner, David S. Miller, Eric Dumazet, virtualization
On 2020/5/6 上午8:01, Michael S. Tsirkin wrote:
> When we fill up a receive VQ, try_fill_recv currently tries to count
> kicks using a 64 bit stats counter. Turns out, on a 32 bit kernel that
> uses a seqcount. sequence counts are "lock" constructs where you need to
> make sure that writers are serialized.
>
> In turn, this means that we mustn't run two try_fill_recv concurrently.
> Which of course we don't. We do run try_fill_recv sometimes from a fully
> preemptible context and sometimes from a softirq (napi) context.
>
> However, when it comes to the seqcount, lockdep is trying to enforce
> the rule that the same lock isn't accessed from preemptible
> and softirq context. This causes a false-positive warning:
>
> WARNING: inconsistent lock state
> ...
> inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage.
>
> As a work around, shut down the warning by switching
> to u64_stats_update_begin_irqsave - that works by disabling
> interrupts on 32 bit only, is a NOP on 64 bit.
>
> Reported-by: Thomas Gleixner <tglx@linutronix.de>
> Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> I'm not thrilled about this but this seems the best we can do for now.
>
> Completely untested.
>
>
> Thomas, can you pls let me know the config I need to trigger the warning
> in question?
>
>
> drivers/net/virtio_net.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 6594aab4910e..95393b61187f 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1243,9 +1243,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)) {
> - u64_stats_update_begin(&rq->stats.syncp);
> + unsigned long flags;
> +
> + flags = u64_stats_update_begin_irqsave(&rq->stats.syncp);
> rq->stats.kicks++;
> - u64_stats_update_end(&rq->stats.syncp);
> + u64_stats_update_end_irqrestore(&rq->stats.syncp);
> }
>
> return !oom;
Acked-by: Jason Wang <jasowang@redhat.com>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] virtio_net: fix lockdep warning on 32 bit
2020-05-06 0:01 ` Michael S. Tsirkin
(?)
(?)
@ 2020-05-06 20:42 ` David Miller
-1 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2020-05-06 20:42 UTC (permalink / raw)
To: mst; +Cc: linux-kernel, tglx, eric.dumazet, jasowang, virtualization, netdev
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Tue, 5 May 2020 20:01:31 -0400
> - u64_stats_update_end(&rq->stats.syncp);
> + u64_stats_update_end_irqrestore(&rq->stats.syncp);
Need to pass flags to this function.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] virtio_net: fix lockdep warning on 32 bit
2020-05-06 0:01 ` Michael S. Tsirkin
` (2 preceding siblings ...)
(?)
@ 2020-05-06 21:54 ` kbuild test robot
-1 siblings, 0 replies; 6+ messages in thread
From: kbuild test robot @ 2020-05-06 21:54 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 3307 bytes --]
Hi "Michael,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on linux/master sparc-next/master v5.7-rc4 next-20200505]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Michael-S-Tsirkin/virtio_net-fix-lockdep-warning-on-32-bit/20200506-113333
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git dc56c5acd8505e1e7f776d62650f3850ad2ce8e7
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=sh
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/net/virtio_net.c: In function 'try_fill_recv':
>> drivers/net/virtio_net.c:1250:3: error: too few arguments to function 'u64_stats_update_end_irqrestore'
1250 | u64_stats_update_end_irqrestore(&rq->stats.syncp);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from include/net/snmp.h:47,
from include/net/netns/mib.h:5,
from include/net/net_namespace.h:17,
from include/linux/netdevice.h:38,
from drivers/net/virtio_net.c:7:
include/linux/u64_stats_sync.h:149:1: note: declared here
149 | u64_stats_update_end_irqrestore(struct u64_stats_sync *syncp,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/u64_stats_update_end_irqrestore +1250 drivers/net/virtio_net.c
1219
1220 /*
1221 * Returns false if we couldn't fill entirely (OOM).
1222 *
1223 * Normally run in the receive path, but can also be run from ndo_open
1224 * before we're receiving packets, or from refill_work which is
1225 * careful to disable receiving (using napi_disable).
1226 */
1227 static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
1228 gfp_t gfp)
1229 {
1230 int err;
1231 bool oom;
1232
1233 do {
1234 if (vi->mergeable_rx_bufs)
1235 err = add_recvbuf_mergeable(vi, rq, gfp);
1236 else if (vi->big_packets)
1237 err = add_recvbuf_big(vi, rq, gfp);
1238 else
1239 err = add_recvbuf_small(vi, rq, gfp);
1240
1241 oom = err == -ENOMEM;
1242 if (err)
1243 break;
1244 } while (rq->vq->num_free);
1245 if (virtqueue_kick_prepare(rq->vq) && virtqueue_notify(rq->vq)) {
1246 unsigned long flags;
1247
1248 flags = u64_stats_update_begin_irqsave(&rq->stats.syncp);
1249 rq->stats.kicks++;
> 1250 u64_stats_update_end_irqrestore(&rq->stats.syncp);
1251 }
1252
1253 return !oom;
1254 }
1255
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 54673 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-05-06 21:54 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-06 0:01 [PATCH] virtio_net: fix lockdep warning on 32 bit Michael S. Tsirkin
2020-05-06 0:01 ` Michael S. Tsirkin
2020-05-06 5:28 ` Jason Wang
2020-05-06 5:28 ` Jason Wang
2020-05-06 20:42 ` David Miller
2020-05-06 21:54 ` kbuild test robot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.