Bug: loop counter in ringbuf.h function __store_ring() and __load_ring() is wrong

[liuqun]

Both __store_ring() and __load_ring() have a same bug.

function __store_ring:

portable-lib/inc/fast/ringbuf.h

Lines 237 to 277 in 10c3286

	__store_ring(struct rte_ring *r, uint_fast32_t head, void * const* obj, unsigned n)
	{
	unsigned i;
	unsigned loops = n & 0x3;
	unsigned idx = head & r->mask;
	// If we know we won't wrap around, we unroll 4 times
	if (likely((idx + n) <= r->mask)) {
	for (i = 0; i < loops; i += 4, idx += 4) {
	r->ring[idx+0] = obj[i+0];
	r->ring[idx+1] = obj[i+1];
	r->ring[idx+2] = obj[i+2];
	r->ring[idx+3] = obj[i+3];
	}
	// mop up remainder
	switch (n & 0x3) {
	case 3:
	r->ring[idx+0] = obj[i+0];
	r->ring[idx+1] = obj[i+1];
	r->ring[idx+2] = obj[i+2];
	break;
	case 2:
	r->ring[idx+0] = obj[i+0];
	r->ring[idx+1] = obj[i+1];
	break;
	case 1:
	r->ring[idx+0] = obj[i+0];
	break;
	}
	} else {
	const uint32_t mask = r->mask;
	for (i = 0; i < n; i++, idx++) {
	r->ring[idx & mask] = obj[i];
	}
	}
	}

• Bug: When n=1, function __store_ring(ring, head, elements, n) actually writes 4 elements into the ring.

• Reason: loop should = n>>2 not n&0x03
  Buggy code is at line 240:
  

  portable-lib/inc/fast/ringbuf.h

  Line 240 in 10c3286

  unsigned loops = n & 0x3;

• Solution:
  The correct logic should be:

unsigned loop = n & ((unsigned) ~0x3U);

see also https://github.com/DPDK/dpdk/blob/07f08a96f5255a0375dce2683db4fb489bbaa8a0/lib/librte_ring/rte_ring.h#L240-L250

[opencoff]

Thank you for finding this issue. I patched it - but slightly different; I moved the unrolled-loop-count to inside the "am I wrapped or not" if () block. Please take a look at the latest commit. Best, -- 刘群 wrote on 8/10/19 8:19 AM:

…

https://github.com/opencoff/portable-lib/blob/10c32868e57c65b314e24834be6c62b1e856e74e/inc/fast/ringbuf.h#L237-L277 * Bug: When |n=1|, function |__store_ring(ring, head, elements, n)| actually writes 4 elements into the ring. * Reason: |loop| should = |n>>2| not |n&0x03| Buggy code is at line 240: https://github.com/opencoff/portable-lib/blob/10c32868e57c65b314e24834be6c62b1e856e74e/inc/fast/ringbuf.h#L240 * Solution: The correct logic should let |loop = n % 4| or using bit-shift operator '>>': |unsigned loop = n >> 2; | — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1?email_source=notifications&email_token=ACB7AW3MNRZXRQNKEDMMRSLQDZTWPA5CNFSM4IKYZZ4KYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HEQTEKQ>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACB7AWZXAEEPPYRV43EEAO3QDZTWPANCNFSM4IKYZZ4A>.

[liuqun]

Fixed by 9b47975