diff mbox

[dpdk-dev,v3,03/14] ring: eliminate duplication of size and mask fields

Message ID 20170324171008.29355-4-bruce.richardson@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Bruce Richardson March 24, 2017, 5:09 p.m. UTC 
  The size and mask fields are duplicated in both the producer and
consumer data structures. Move them out of that into the top level
structure so they are not duplicated.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Olivier Matz <olivier.matz@6wind.com>
---
 lib/librte_ring/rte_ring.c | 20 ++++++++++----------
 lib/librte_ring/rte_ring.h | 32 ++++++++++++++++----------------
 test/test/test_ring.c      |  6 +++---
 3 files changed, 29 insertions(+), 29 deletions(-)

Comments

Thomas Monjalon March 27, 2017, 9:52 a.m. UTC | #1 
2017-03-24 17:09, Bruce Richardson:
> The size and mask fields are duplicated in both the producer and
> consumer data structures. Move them out of that into the top level
> structure so they are not duplicated.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> Acked-by: Olivier Matz <olivier.matz@6wind.com>

Sorry Bruce, I encounter this error:

fatal error: no member named 'size' in 'struct rte_ring_headtail'
                if (r->prod.size >= ring_size) {
                    ~~~~~~~ ^
Bruce Richardson March 27, 2017, 10:13 a.m. UTC | #2 
On Mon, Mar 27, 2017 at 11:52:58AM +0200, Thomas Monjalon wrote:
> 2017-03-24 17:09, Bruce Richardson:
> > The size and mask fields are duplicated in both the producer and
> > consumer data structures. Move them out of that into the top level
> > structure so they are not duplicated.
> > 
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > Acked-by: Olivier Matz <olivier.matz@6wind.com>
> 
> Sorry Bruce, I encounter this error:
> 
> fatal error: no member named 'size' in 'struct rte_ring_headtail'
>                 if (r->prod.size >= ring_size) {
>                     ~~~~~~~ ^
>
Hi Thomas,

again I need more information here. I've just revalidated these first
three patches doing 7 builds with each one (gcc, clang, debug, shared 
library, old ABI, default-machine and 32-bit), as well as compiling the
apps for gcc and clang, and I see no errors.

/Bruce
Bruce Richardson March 27, 2017, 10:15 a.m. UTC | #3 
On Mon, Mar 27, 2017 at 11:52:58AM +0200, Thomas Monjalon wrote:
> 2017-03-24 17:09, Bruce Richardson:
> > The size and mask fields are duplicated in both the producer and
> > consumer data structures. Move them out of that into the top level
> > structure so they are not duplicated.
> > 
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > Acked-by: Olivier Matz <olivier.matz@6wind.com>
> 
> Sorry Bruce, I encounter this error:
> 
> fatal error: no member named 'size' in 'struct rte_ring_headtail'
>                 if (r->prod.size >= ring_size) {
>                     ~~~~~~~ ^
> 
Ok, I think I've found it now using git grep. I assume this is in the
crypto code which is disabled by default, right?

/Bruce
Thomas Monjalon March 27, 2017, 1:13 p.m. UTC | #4 
2017-03-27 11:15, Bruce Richardson:
> On Mon, Mar 27, 2017 at 11:52:58AM +0200, Thomas Monjalon wrote:
> > 2017-03-24 17:09, Bruce Richardson:
> > > The size and mask fields are duplicated in both the producer and
> > > consumer data structures. Move them out of that into the top level
> > > structure so they are not duplicated.
> > > 
> > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > > Acked-by: Olivier Matz <olivier.matz@6wind.com>
> > 
> > Sorry Bruce, I encounter this error:
> > 
> > fatal error: no member named 'size' in 'struct rte_ring_headtail'
> >                 if (r->prod.size >= ring_size) {
> >                     ~~~~~~~ ^
> > 
> Ok, I think I've found it now using git grep. I assume this is in the
> crypto code which is disabled by default, right?

Right, sorry for forgetting the context.
Bruce Richardson March 27, 2017, 2:57 p.m. UTC | #5 
On Mon, Mar 27, 2017 at 03:13:04PM +0200, Thomas Monjalon wrote:
> 2017-03-27 11:15, Bruce Richardson:
> > On Mon, Mar 27, 2017 at 11:52:58AM +0200, Thomas Monjalon wrote:
> > > 2017-03-24 17:09, Bruce Richardson:
> > > > The size and mask fields are duplicated in both the producer and
> > > > consumer data structures. Move them out of that into the top level
> > > > structure so they are not duplicated.
> > > > 
> > > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > > > Acked-by: Olivier Matz <olivier.matz@6wind.com>
> > > 
> > > Sorry Bruce, I encounter this error:
> > > 
> > > fatal error: no member named 'size' in 'struct rte_ring_headtail'
> > >                 if (r->prod.size >= ring_size) {
> > >                     ~~~~~~~ ^
> > > 
> > Ok, I think I've found it now using git grep. I assume this is in the
> > crypto code which is disabled by default, right?
> 
> Right, sorry for forgetting the context.

Submitted a new patch for the crypto drivers to fix this issue. The
previous patch I did to fix this as a pre-requisite didn't catch the
drivers that were disabled by default.

http://dpdk.org/dev/patchwork/patch/22445/

I've verified this now compiles with the aesni-mb crypto PMD enabled at
least. I'll see about enabling a few more crypto drivers on my system
just in case there are other things I missed in them for this set.

/Bruce
diff mbox

Patch

diff --git a/lib/librte_ring/rte_ring.c b/lib/librte_ring/rte_ring.c
index 93a8692..93485d4 100644
--- a/lib/librte_ring/rte_ring.c
+++ b/lib/librte_ring/rte_ring.c
@@ -144,11 +144,11 @@  rte_ring_init(struct rte_ring *r, const char *name, unsigned count,
 	if (ret < 0 || ret >= (int)sizeof(r->name))
 		return -ENAMETOOLONG;
 	r->flags = flags;
-	r->prod.watermark = count;
+	r->watermark = count;
 	r->prod.single = !!(flags & RING_F_SP_ENQ);
 	r->cons.single = !!(flags & RING_F_SC_DEQ);
-	r->prod.size = r->cons.size = count;
-	r->prod.mask = r->cons.mask = count-1;
+	r->size = count;
+	r->mask = count - 1;
 	r->prod.head = r->cons.head = 0;
 	r->prod.tail = r->cons.tail = 0;
 
@@ -269,14 +269,14 @@  rte_ring_free(struct rte_ring *r)
 int
 rte_ring_set_water_mark(struct rte_ring *r, unsigned count)
 {
-	if (count >= r->prod.size)
+	if (count >= r->size)
 		return -EINVAL;
 
 	/* if count is 0, disable the watermarking */
 	if (count == 0)
-		count = r->prod.size;
+		count = r->size;
 
-	r->prod.watermark = count;
+	r->watermark = count;
 	return 0;
 }
 
@@ -291,17 +291,17 @@  rte_ring_dump(FILE *f, const struct rte_ring *r)
 
 	fprintf(f, "ring <%s>@%p\n", r->name, r);
 	fprintf(f, "  flags=%x\n", r->flags);
-	fprintf(f, "  size=%"PRIu32"\n", r->prod.size);
+	fprintf(f, "  size=%"PRIu32"\n", r->size);
 	fprintf(f, "  ct=%"PRIu32"\n", r->cons.tail);
 	fprintf(f, "  ch=%"PRIu32"\n", r->cons.head);
 	fprintf(f, "  pt=%"PRIu32"\n", r->prod.tail);
 	fprintf(f, "  ph=%"PRIu32"\n", r->prod.head);
 	fprintf(f, "  used=%u\n", rte_ring_count(r));
 	fprintf(f, "  avail=%u\n", rte_ring_free_count(r));
-	if (r->prod.watermark == r->prod.size)
+	if (r->watermark == r->size)
 		fprintf(f, "  watermark=0\n");
 	else
-		fprintf(f, "  watermark=%"PRIu32"\n", r->prod.watermark);
+		fprintf(f, "  watermark=%"PRIu32"\n", r->watermark);
 
 	/* sum and dump statistics */
 #ifdef RTE_LIBRTE_RING_DEBUG
@@ -318,7 +318,7 @@  rte_ring_dump(FILE *f, const struct rte_ring *r)
 		sum.deq_fail_bulk += r->stats[lcore_id].deq_fail_bulk;
 		sum.deq_fail_objs += r->stats[lcore_id].deq_fail_objs;
 	}
-	fprintf(f, "  size=%"PRIu32"\n", r->prod.size);
+	fprintf(f, "  size=%"PRIu32"\n", r->size);
 	fprintf(f, "  enq_success_bulk=%"PRIu64"\n", sum.enq_success_bulk);
 	fprintf(f, "  enq_success_objs=%"PRIu64"\n", sum.enq_success_objs);
 	fprintf(f, "  enq_quota_bulk=%"PRIu64"\n", sum.enq_quota_bulk);
diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h
index 331c94f..d650215 100644
--- a/lib/librte_ring/rte_ring.h
+++ b/lib/librte_ring/rte_ring.h
@@ -151,10 +151,7 @@  struct rte_memzone; /* forward declaration, so as not to require memzone.h */
 struct rte_ring_headtail {
 	volatile uint32_t head;  /**< Prod/consumer head. */
 	volatile uint32_t tail;  /**< Prod/consumer tail. */
-	uint32_t size;           /**< Size of ring. */
-	uint32_t mask;           /**< Mask (size-1) of ring. */
 	uint32_t single;         /**< True if single prod/cons */
-	uint32_t watermark;      /**< Max items before EDQUOT in producer. */
 };
 
 /**
@@ -174,9 +171,12 @@  struct rte_ring {
 	 * next time the ABI changes
 	 */
 	char name[RTE_MEMZONE_NAMESIZE];    /**< Name of the ring. */
-	int flags;                       /**< Flags supplied at creation. */
+	int flags;               /**< Flags supplied at creation. */
 	const struct rte_memzone *memzone;
 			/**< Memzone, if any, containing the rte_ring */
+	uint32_t size;           /**< Size of ring. */
+	uint32_t mask;           /**< Mask (size-1) of ring. */
+	uint32_t watermark;      /**< Max items before EDQUOT in producer. */
 
 	/** Ring producer status. */
 	struct rte_ring_headtail prod __rte_aligned(PROD_ALIGN);
@@ -355,7 +355,7 @@  void rte_ring_dump(FILE *f, const struct rte_ring *r);
  * Placed here since identical code needed in both
  * single and multi producer enqueue functions */
 #define ENQUEUE_PTRS() do { \
-	const uint32_t size = r->prod.size; \
+	const uint32_t size = r->size; \
 	uint32_t idx = prod_head & mask; \
 	if (likely(idx + n < size)) { \
 		for (i = 0; i < (n & ((~(unsigned)0x3))); i+=4, idx+=4) { \
@@ -382,7 +382,7 @@  void rte_ring_dump(FILE *f, const struct rte_ring *r);
  * single and multi consumer dequeue functions */
 #define DEQUEUE_PTRS() do { \
 	uint32_t idx = cons_head & mask; \
-	const uint32_t size = r->cons.size; \
+	const uint32_t size = r->size; \
 	if (likely(idx + n < size)) { \
 		for (i = 0; i < (n & (~(unsigned)0x3)); i+=4, idx+=4) {\
 			obj_table[i] = r->ring[idx]; \
@@ -437,7 +437,7 @@  __rte_ring_mp_do_enqueue(struct rte_ring *r, void * const *obj_table,
 	const unsigned max = n;
 	int success;
 	unsigned i, rep = 0;
-	uint32_t mask = r->prod.mask;
+	uint32_t mask = r->mask;
 	int ret;
 
 	/* Avoid the unnecessary cmpset operation below, which is also
@@ -485,7 +485,7 @@  __rte_ring_mp_do_enqueue(struct rte_ring *r, void * const *obj_table,
 	rte_smp_wmb();
 
 	/* if we exceed the watermark */
-	if (unlikely(((mask + 1) - free_entries + n) > r->prod.watermark)) {
+	if (unlikely(((mask + 1) - free_entries + n) > r->watermark)) {
 		ret = (behavior == RTE_RING_QUEUE_FIXED) ? -EDQUOT :
 				(int)(n | RTE_RING_QUOT_EXCEED);
 		__RING_STAT_ADD(r, enq_quota, n);
@@ -544,7 +544,7 @@  __rte_ring_sp_do_enqueue(struct rte_ring *r, void * const *obj_table,
 	uint32_t prod_head, cons_tail;
 	uint32_t prod_next, free_entries;
 	unsigned i;
-	uint32_t mask = r->prod.mask;
+	uint32_t mask = r->mask;
 	int ret;
 
 	prod_head = r->prod.head;
@@ -580,7 +580,7 @@  __rte_ring_sp_do_enqueue(struct rte_ring *r, void * const *obj_table,
 	rte_smp_wmb();
 
 	/* if we exceed the watermark */
-	if (unlikely(((mask + 1) - free_entries + n) > r->prod.watermark)) {
+	if (unlikely(((mask + 1) - free_entries + n) > r->watermark)) {
 		ret = (behavior == RTE_RING_QUEUE_FIXED) ? -EDQUOT :
 			(int)(n | RTE_RING_QUOT_EXCEED);
 		__RING_STAT_ADD(r, enq_quota, n);
@@ -630,7 +630,7 @@  __rte_ring_mc_do_dequeue(struct rte_ring *r, void **obj_table,
 	const unsigned max = n;
 	int success;
 	unsigned i, rep = 0;
-	uint32_t mask = r->prod.mask;
+	uint32_t mask = r->mask;
 
 	/* Avoid the unnecessary cmpset operation below, which is also
 	 * potentially harmful when n equals 0. */
@@ -727,7 +727,7 @@  __rte_ring_sc_do_dequeue(struct rte_ring *r, void **obj_table,
 	uint32_t cons_head, prod_tail;
 	uint32_t cons_next, entries;
 	unsigned i;
-	uint32_t mask = r->prod.mask;
+	uint32_t mask = r->mask;
 
 	cons_head = r->cons.head;
 	prod_tail = r->prod.tail;
@@ -1056,7 +1056,7 @@  rte_ring_full(const struct rte_ring *r)
 {
 	uint32_t prod_tail = r->prod.tail;
 	uint32_t cons_tail = r->cons.tail;
-	return ((cons_tail - prod_tail - 1) & r->prod.mask) == 0;
+	return ((cons_tail - prod_tail - 1) & r->mask) == 0;
 }
 
 /**
@@ -1089,7 +1089,7 @@  rte_ring_count(const struct rte_ring *r)
 {
 	uint32_t prod_tail = r->prod.tail;
 	uint32_t cons_tail = r->cons.tail;
-	return (prod_tail - cons_tail) & r->prod.mask;
+	return (prod_tail - cons_tail) & r->mask;
 }
 
 /**
@@ -1105,7 +1105,7 @@  rte_ring_free_count(const struct rte_ring *r)
 {
 	uint32_t prod_tail = r->prod.tail;
 	uint32_t cons_tail = r->cons.tail;
-	return (cons_tail - prod_tail - 1) & r->prod.mask;
+	return (cons_tail - prod_tail - 1) & r->mask;
 }
 
 /**
@@ -1119,7 +1119,7 @@  rte_ring_free_count(const struct rte_ring *r)
 static inline unsigned int
 rte_ring_get_size(const struct rte_ring *r)
 {
-	return r->prod.size;
+	return r->size;
 }
 
 /**
diff --git a/test/test/test_ring.c b/test/test/test_ring.c
index ebcb896..5f09097 100644
--- a/test/test/test_ring.c
+++ b/test/test/test_ring.c
@@ -148,7 +148,7 @@  check_live_watermark_change(__attribute__((unused)) void *dummy)
 		}
 
 		/* read watermark, the only change allowed is from 16 to 32 */
-		watermark = r->prod.watermark;
+		watermark = r->watermark;
 		if (watermark != watermark_old &&
 		    (watermark_old != 16 || watermark != 32)) {
 			printf("Bad watermark change %u -> %u\n", watermark_old,
@@ -213,7 +213,7 @@  test_set_watermark( void ){
 		printf( " ring lookup failed\n" );
 		goto error;
 	}
-	count = r->prod.size*2;
+	count = r->size * 2;
 	setwm = rte_ring_set_water_mark(r, count);
 	if (setwm != -EINVAL){
 		printf("Test failed to detect invalid watermark count value\n");
@@ -222,7 +222,7 @@  test_set_watermark( void ){
 
 	count = 0;
 	rte_ring_set_water_mark(r, count);
-	if (r->prod.watermark != r->prod.size) {
+	if (r->watermark != r->size) {
 		printf("Test failed to detect invalid watermark count value\n");
 		goto error;
 	}