Re: [PATCH v2] graph: expose node context as pointers

["Robin Jarry" <rjarry@redhat.com>]

Hi Tyler,

Tyler Retzlaff, Mar 22, 2024 at 17:56:
> i can answer this!
>
> windows toolchains will only require __extension__ qualification on use
> of statement expressions, so msvc won't require any use of __extension__
> in this patch.
>
> as a general rule of thumb __extension__ is something you may choose to
> use for any gcc compiled code that is an extension to standard C and you
> intend to use the -pedantic flag (i.e. -std=c11 && -pedantic used together)

Got it, thanks!

> >  	/* Fast path area  */
> >  #define RTE_NODE_CTX_SZ 16
> > -	alignas(RTE_CACHE_LINE_SIZE) uint8_t ctx[RTE_NODE_CTX_SZ]; /**< Node Context. */
> > +	__extension__ alignas(RTE_CACHE_LINE_SIZE) union {
>
> __extension__ should not be on the anonymous union (since they are standard C11).
>
> anonymous union declaration is actually a type with no name and then a data
> field of that type so __rte_aligned is most likely what you want, since
> you're using RTE_CACHE_LINE_SIZE we can use __rte_cache_aligned.
>
> union __rte_cache_aligned {
>    ... your union fields ...
> };
>
> and i think checkpatches still gives a warning unrelated to alignment
> for this but it can be safely ignored. it's the warning about alignment
> that we care about and should be fixed.

This passes the C++ header check but it breaks the static_assert I just 
added. I believe the alignment is somehow transferred to all union 
fields. And since ctx is an array, it makes the whole union .

So before my patch:

  /* --- cacheline 3 boundary (192 bytes) --- */
  uint8_t  ctx[16] __attribute__((__aligned__(64))); /*   192    16 */
  uint16_t size;                                     /*   208     2 */

With the anonymous union aligned:

  /* --- cacheline 3 boundary (192 bytes) --- */
  union {
          uint8_t          ctx[16];                  /*   192    16 */
          struct {
                  void *   ctx_ptr;                  /*   192     8 */
                  void *   ctx_ptr2;                 /*   200     8 */
          };                                         /*   192    16 */
  } __attribute__((__aligned__(64)));                /*   192    64 */
  /* --- cacheline 4 boundary (256 bytes) --- */
  uint16_t                 size;                     /*   256     2 */

However, if I remove the explicit align, I get what I expect:

  /* --- cacheline 3 boundary (192 bytes) --- */
  union {
          uint8_t          ctx[16];                  /*   192    16 */
          struct {
                  void *   ctx_ptr;                  /*   192     8 */
                  void *   ctx_ptr2;                 /*   200     8 */
          };                                         /*   192    16 */
  };                                                 /*   192    16 */
  uint16_t                 size;                     /*   208     2 */

Is it OK to drop the explicit alignment? This is beyond my C skills :)

> > +		uint8_t ctx[RTE_NODE_CTX_SZ];
> > +		/* Convenience aliases to store pointers without complex casting. */
> > +		__extension__ struct {
>
> this is correct/recommended since anonymous structs aren't standard,
> with the __extension__ -pedantic won't emit a warning (our intention).

Ack.

> > +static_assert(offsetof(struct rte_node, size) - offsetof(struct rte_node, ctx) == RTE_NODE_CTX_SZ,
> > +	"The node context anonymous union cannot be larger than RTE_NODE_CTX_SZ");
> > +
>
> you should include directly include <stddef.h> in this file for use of offsetof.
> you should include directly include <assert.h> in this file for use of the static_assert.

Will do for v3.

Thanks!