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

On Wed, Jun 06, 2012 at 10:45:41AM +0200, Eric Dumazet wrote:

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.

We currently do all stats either on napi callback or from
start_xmit callback.
This makes them safe, yes?

[1] If done with one instruction, we still have a race, since a reader
might see an even sequence and conclude no writer is inside the critical
section. So read values could be wrong.