diff mbox

[dpdk-dev,v2,02/14] ring: create common structure for prod and cons metadata

Message ID 20170307113217.11077-3-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 fail apply patch file failure

Commit Message

Bruce Richardson March 7, 2017, 11:32 a.m. UTC 
  create a common structure to hold the metadata for the producer and
the consumer, since both need essentially the same information - the
head and tail values, the ring size and mask.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>

---

V2: renamed the shared structure based on maintainer feedback.
---
 lib/librte_ring/rte_ring.h | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

Comments

Thomas Monjalon March 15, 2017, 2:01 p.m. UTC | #1 
clang error below:

2017-03-07 11:32, Bruce Richardson:
> +       union {
> +               uint32_t sp_enqueue; /**< True, if single producer. */
> +               uint32_t sc_dequeue; /**< True, if single consumer. */
> +       };

error: anonymous unions are a C11 extension
Bruce Richardson March 22, 2017, 4:38 p.m. UTC | #2 
On Wed, Mar 15, 2017 at 03:01:49PM +0100, Thomas Monjalon wrote:
> clang error below:
> 
> 2017-03-07 11:32, Bruce Richardson:
> > +       union {
> > +               uint32_t sp_enqueue; /**< True, if single producer. */
> > +               uint32_t sc_dequeue; /**< True, if single consumer. */
> > +       };
> 
> error: anonymous unions are a C11 extension

What clang version and other CFLAGS settings are you using? Clang
compilation runs fine for me with clang 3.9.1 on Fedora 25.

/Bruce
Bruce Richardson March 24, 2017, 2:55 p.m. UTC | #3 
On Wed, Mar 15, 2017 at 03:01:49PM +0100, Thomas Monjalon wrote:
> clang error below:
> 
> 2017-03-07 11:32, Bruce Richardson:
> > +       union {
> > +               uint32_t sp_enqueue; /**< True, if single producer. */
> > +               uint32_t sc_dequeue; /**< True, if single consumer. */
> > +       };
> 
> error: anonymous unions are a C11 extension

Olivier, Thomas, feedback on suggestions for fixing this? Note: I'm
still waiting to hear back on what compiler settings are needed to
trigger this error.

Two immediately obvious options:
* replace the union with a single variable called e.g. "single", i.e.
  prod.single indicates single producer, and cons.single indicates
  single consumer. The downside of this approach is that it makes the
  patch a little bigger - as other code needs to be modified to use the
  new name - and is not backward compatible for apps which
  may reference this public structure memeber.
* just remove the union without renaming anything, leaving two structure
  members called sp_enqueue and sc_dequeue. This uses a little more
  space in the structure, which is not a big deal since it needs to fill
  a cacheline anyway, but it is backward compatible in that no other
  code should need to be modified.

Other options? My preference is for the first one. Given we are breaking
the ring API anyway, I think we might as well use the shorter name and
eliminate the need for the union, or multiple variables.

/Bruce
Olivier Matz March 24, 2017, 4:41 p.m. UTC | #4 
Hi Bruce,

On Fri, 24 Mar 2017 14:55:36 +0000, Bruce Richardson <bruce.richardson@intel.com> wrote:
> On Wed, Mar 15, 2017 at 03:01:49PM +0100, Thomas Monjalon wrote:
> > clang error below:
> > 
> > 2017-03-07 11:32, Bruce Richardson:  
> > > +       union {
> > > +               uint32_t sp_enqueue; /**< True, if single producer. */
> > > +               uint32_t sc_dequeue; /**< True, if single consumer. */
> > > +       };  
> > 
> > error: anonymous unions are a C11 extension  
> 
> Olivier, Thomas, feedback on suggestions for fixing this? Note: I'm
> still waiting to hear back on what compiler settings are needed to
> trigger this error.
> 
> Two immediately obvious options:
> * replace the union with a single variable called e.g. "single", i.e.
>   prod.single indicates single producer, and cons.single indicates
>   single consumer. The downside of this approach is that it makes the
>   patch a little bigger - as other code needs to be modified to use the
>   new name - and is not backward compatible for apps which
>   may reference this public structure memeber.
> * just remove the union without renaming anything, leaving two structure
>   members called sp_enqueue and sc_dequeue. This uses a little more
>   space in the structure, which is not a big deal since it needs to fill
>   a cacheline anyway, but it is backward compatible in that no other
>   code should need to be modified.
> 
> Other options? My preference is for the first one. Given we are breaking
> the ring API anyway, I think we might as well use the shorter name and
> eliminate the need for the union, or multiple variables.

What about adding RTE_STD_C11 like it's done in rte_mbuf?

I didn't try, but since mbuf compiles, it should solve this issue in ring.


Regards,
Olivier
Bruce Richardson March 24, 2017, 4:57 p.m. UTC | #5 
On Fri, Mar 24, 2017 at 05:41:34PM +0100, Olivier Matz wrote:
> Hi Bruce,
> 
> On Fri, 24 Mar 2017 14:55:36 +0000, Bruce Richardson <bruce.richardson@intel.com> wrote:
> > On Wed, Mar 15, 2017 at 03:01:49PM +0100, Thomas Monjalon wrote:
> > > clang error below:
> > > 
> > > 2017-03-07 11:32, Bruce Richardson:  
> > > > +       union {
> > > > +               uint32_t sp_enqueue; /**< True, if single producer. */
> > > > +               uint32_t sc_dequeue; /**< True, if single consumer. */
> > > > +       };  
> > > 
> > > error: anonymous unions are a C11 extension  
> > 
> > Olivier, Thomas, feedback on suggestions for fixing this? Note: I'm
> > still waiting to hear back on what compiler settings are needed to
> > trigger this error.
> > 
> > Two immediately obvious options:
> > * replace the union with a single variable called e.g. "single", i.e.
> >   prod.single indicates single producer, and cons.single indicates
> >   single consumer. The downside of this approach is that it makes the
> >   patch a little bigger - as other code needs to be modified to use the
> >   new name - and is not backward compatible for apps which
> >   may reference this public structure memeber.
> > * just remove the union without renaming anything, leaving two structure
> >   members called sp_enqueue and sc_dequeue. This uses a little more
> >   space in the structure, which is not a big deal since it needs to fill
> >   a cacheline anyway, but it is backward compatible in that no other
> >   code should need to be modified.
> > 
> > Other options? My preference is for the first one. Given we are breaking
> > the ring API anyway, I think we might as well use the shorter name and
> > eliminate the need for the union, or multiple variables.
> 
> What about adding RTE_STD_C11 like it's done in rte_mbuf?
> 
> I didn't try, but since mbuf compiles, it should solve this issue in ring.
> 
Yes, it might well. However, looking at the resulting code, I actually
think it's cleaner to have just one variable called "single" in the
structure. The union is really for backward compatibility, and there is
little point in doing so since we are changing the rest of the structure
in other ways.

Struct now looks like:
 /* structure to hold a pair of head/tail values and other metadata */
 struct rte_ring_headtail {
        volatile uint32_t head;  /**< Prod/consumer head. */
        volatile uint32_t tail;  /**< Prod/consumer tail. */
        uint32_t single;         /**< True if single prod/cons */
 };

And the code checks read e.g. for single producer:

       if (r->prod.single)

/Bruce
diff mbox

Patch

diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h
index 399ae3b..659c6d0 100644
--- a/lib/librte_ring/rte_ring.h
+++ b/lib/librte_ring/rte_ring.h
@@ -1,7 +1,7 @@ 
 /*-
  *   BSD LICENSE
  *
- *   Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
+ *   Copyright(c) 2010-2017 Intel Corporation. All rights reserved.
  *   All rights reserved.
  *
  *   Redistribution and use in source and binary forms, with or without
@@ -147,6 +147,19 @@  struct rte_memzone; /* forward declaration, so as not to require memzone.h */
 #define CONS_ALIGN RTE_CACHE_LINE_SIZE
 #endif
 
+/* structure to hold a pair of head/tail values and other metadata */
+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. */
+	union {
+		uint32_t sp_enqueue; /**< True, if single producer. */
+		uint32_t sc_dequeue; /**< True, if single consumer. */
+	};
+	uint32_t watermark;      /**< Max items before EDQUOT in producer. */
+};
+
 /**
  * An RTE ring structure.
  *
@@ -169,23 +182,10 @@  struct rte_ring {
 			/**< Memzone, if any, containing the rte_ring */
 
 	/** Ring producer status. */
-	struct prod {
-		uint32_t watermark;      /**< Maximum items before EDQUOT. */
-		uint32_t sp_enqueue;     /**< True, if single producer. */
-		uint32_t size;           /**< Size of ring. */
-		uint32_t mask;           /**< Mask (size-1) of ring. */
-		volatile uint32_t head;  /**< Producer head. */
-		volatile uint32_t tail;  /**< Producer tail. */
-	} prod __rte_aligned(PROD_ALIGN);
+	struct rte_ring_headtail prod __rte_aligned(PROD_ALIGN);
 
 	/** Ring consumer status. */
-	struct cons {
-		uint32_t sc_dequeue;     /**< True, if single consumer. */
-		uint32_t size;           /**< Size of the ring. */
-		uint32_t mask;           /**< Mask (size-1) of ring. */
-		volatile uint32_t head;  /**< Consumer head. */
-		volatile uint32_t tail;  /**< Consumer tail. */
-	} cons __rte_aligned(CONS_ALIGN);
+	struct rte_ring_headtail cons __rte_aligned(CONS_ALIGN);
 
 #ifdef RTE_LIBRTE_RING_DEBUG
 	struct rte_ring_debug_stats stats[RTE_MAX_LCORE];