Re: [PATCH] virtio-net: fix a race on 32bit arches

On Sun, Jun 10, 2012 at 04:06:34PM +0930, Rusty Russell wrote:

On Wed, 06 Jun 2012 10:45:41 +0200, Eric Dumazet [off-list ref] wrote:

quoted

On Wed, 2012-06-06 at 10:35 +0200, Eric Dumazet wrote:

quoted

From: Eric Dumazet <edumazet@google.com>

commit 3fa2a1df909 (virtio-net: per cpu 64 bit stats (v2)) added a race
on 32bit arches.

We must use separate syncp for rx and tx path as they can be run at the
same time on different cpus. Thus one sequence increment can be lost and
readers spin forever.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Stephen Hemminger <redacted>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Jason Wang <redacted>
---

Just to make clear : even using percpu stats/syncp, we have no guarantee
that write_seqcount_begin() is done with one instruction. [1]

It is OK on x86 if "incl" instruction is generated by the compiler, but
on a RISC cpu, the "load memory,%reg ; inc %reg ; store %reg,memory" can
be interrupted.

So if you are 100% sure all paths are safe against preemption/BH, then
this patch is not needed, but a big comment in the code would avoid
adding possible races in the future.

Too fragile; let's keep them separate as per this patch.

Acked-by: Rusty Russell <redacted>

Thanks,
Rusty.

One question though: do we want to lay the structure
out so that the rx sync structure precedes the rx counters?

-- 
MST