RE: [PATCH net-next 3/3] packet: use percpu mmap tx frame pending refcount

From: Cong Wang

On Sun, Jan 12, 2014 at 8:22 AM, Daniel Borkmann [off-list ref] wrote:

quoted

+static void packet_inc_pending(struct packet_ring_buffer *rb)
+{
+       this_cpu_inc(*rb->pending_refcnt);
+}
+
+static void packet_dec_pending(struct packet_ring_buffer *rb)
+{
+       this_cpu_dec(*rb->pending_refcnt);
+}
+
+static int packet_read_pending(const struct packet_ring_buffer *rb)
+{
+       int i, refcnt = 0;
+
+       /* We don't use pending refcount in rx_ring. */
+       if (rb->pending_refcnt == NULL)
+               return 0;
+
+       for_each_possible_cpu(i)
+               refcnt += *per_cpu_ptr(rb->pending_refcnt, i);
+
+       return refcnt;
+}

How is this supposed to work? Since there is no lock,
you can't read accurate refcnt. Take a look at lib/percpu_counter.c.

I guess for some reason you don't care the accuracy?
Then at least you need to comment in the code.

Hmmm... did the code ever work?

The value looks like the number of active transmits (s/pending/tx_pending/)
The test of the number of pending tx is at the bottom of a code loop,
I'd expect that some action should be locked against this check - but even
the atomic read doesn't do this.

Check what happens if the count reads as 1 just before another cpu
decrements it to zero (or reads 0 just before being incremented).
In one of those cases something is likely to go wrong.

I'm actually surprised that there isn't a mutex covering the code path.

	David