[v2] graph: expose node context as pointers
Checks
Commit Message
In some cases, the node context data is used to store two pointers
because the data is larger than the reserved 16 bytes. Having to define
intermediate structures just to be able to cast is tedious. Add two
pointers that take the same space than ctx.
Signed-off-by: Robin Jarry <rjarry@redhat.com>
---
Notes:
v2:
* Added __extension__ (not sure where it is needed, I don't have access to windows).
* It still fails the header check for C++. It seems not possible to align an unnamed union...
Tyler, do you have an idea about how to fix that?
* Added static_assert to ensure the anonymous union is not larger than RTE_NODE_CTX_SZ.
lib/graph/rte_graph_worker_common.h | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
Comments
On Fri, Mar 22, 2024 at 05:31:31PM +0100, Robin Jarry wrote:
> In some cases, the node context data is used to store two pointers
> because the data is larger than the reserved 16 bytes. Having to define
> intermediate structures just to be able to cast is tedious. Add two
> pointers that take the same space than ctx.
>
> Signed-off-by: Robin Jarry <rjarry@redhat.com>
> ---
>
> Notes:
> v2:
>
> * Added __extension__ (not sure where it is needed, I don't have access to windows).
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)
i've commented inline below.
> * It still fails the header check for C++. It seems not possible to align an unnamed union...
> Tyler, do you have an idea about how to fix that?
yes, see below what i suspect you want.
> * Added static_assert to ensure the anonymous union is not larger than RTE_NODE_CTX_SZ.
>
> lib/graph/rte_graph_worker_common.h | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/lib/graph/rte_graph_worker_common.h b/lib/graph/rte_graph_worker_common.h
> index 36d864e2c14e..a60c2bc3f0c3 100644
> --- a/lib/graph/rte_graph_worker_common.h
> +++ b/lib/graph/rte_graph_worker_common.h
> @@ -112,7 +112,14 @@ struct __rte_cache_aligned rte_node {
> };
> /* 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.
> + 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).
> + void *ctx_ptr;
> + void *ctx_ptr2;
> + };
> + }; /**< Node Context. */
> uint16_t size; /**< Total number of objects available. */
> uint16_t idx; /**< Number of objects used. */
> rte_graph_off_t off; /**< Offset of node in the graph reel. */
> @@ -130,6 +137,9 @@ struct __rte_cache_aligned rte_node {
> alignas(RTE_CACHE_LINE_MIN_SIZE) struct rte_node *nodes[]; /**< Next nodes. */
> };
>
> +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.
hope this helps!
ty
> /**
> * @internal
> *
> --
> 2.44.0
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!
@@ -112,7 +112,14 @@ struct __rte_cache_aligned rte_node {
};
/* 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 {
+ uint8_t ctx[RTE_NODE_CTX_SZ];
+ /* Convenience aliases to store pointers without complex casting. */
+ __extension__ struct {
+ void *ctx_ptr;
+ void *ctx_ptr2;
+ };
+ }; /**< Node Context. */
uint16_t size; /**< Total number of objects available. */
uint16_t idx; /**< Number of objects used. */
rte_graph_off_t off; /**< Offset of node in the graph reel. */
@@ -130,6 +137,9 @@ struct __rte_cache_aligned rte_node {
alignas(RTE_CACHE_LINE_MIN_SIZE) struct rte_node *nodes[]; /**< Next nodes. */
};
+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");
+
/**
* @internal
*