[v4] eventdev: ensure 16-byte alignment for events

  The event structure in DPDK is 16-bytes in size, and events are
regularly passed as parameters directly rather than being passed as
pointers. To help compiler optimize correctly, we can explicitly request
16-byte alignment for events, which means that we should be able
to do aligned vector loads/stores (e.g. with SSE or Neon) when working
with those events.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Jerin Jacob <jerinj@marvell.com>

---
v4: change _Static_assert to static_assert for fwd compatibility
    moved size check on event structure to C file from header file

v3: Fix spelling mistake in RN entry
    rebase on latest eventdev tree HEAD

v2: added release note entry for ABI change
    added structure comment on 16-byte size and alignment
    added static assert for structure size
---
 doc/guides/rel_notes/release_23_11.rst | 4 ++++
 lib/eventdev/rte_eventdev.c            | 3 +++
 lib/eventdev/rte_eventdev.h            | 4 +++-
 3 files changed, 10 insertions(+), 1 deletion(-)
  

On Fri,  6 Oct 2023 11:29:32 +0100
Bruce Richardson <bruce.richardson@intel.com> wrote:

> diff --git a/lib/eventdev/rte_eventdev.c b/lib/eventdev/rte_eventdev.c
> index 95373bbaad..adc9751cef 100644
> --- a/lib/eventdev/rte_eventdev.c
> +++ b/lib/eventdev/rte_eventdev.c
> @@ -9,6 +9,7 @@
>  #include <errno.h>
>  #include <stdint.h>
>  #include <inttypes.h>
> +#include <assert.h>
>  
>  #include <rte_string_fns.h>
>  #include <rte_log.h>
> @@ -28,6 +29,8 @@
>  #include "eventdev_pmd.h"
>  #include "eventdev_trace.h"
>  
> +static_assert(sizeof(struct rte_event) == 16, "Event structure size is not 16-bytes in size");
> +
>  static struct rte_eventdev rte_event_devices[RTE_EVENT_MAX_DEVS];

Please don't reinvent RTE_BUILD_BUG_ON().
Instead fix that to be a static_assert()
  

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Sunday, 12 November 2023 01.01
> 
> On Fri,  6 Oct 2023 11:29:32 +0100
> Bruce Richardson <bruce.richardson@intel.com> wrote:
> 
> > diff --git a/lib/eventdev/rte_eventdev.c
> b/lib/eventdev/rte_eventdev.c
> > index 95373bbaad..adc9751cef 100644
> > --- a/lib/eventdev/rte_eventdev.c
> > +++ b/lib/eventdev/rte_eventdev.c
> > @@ -9,6 +9,7 @@
> >  #include <errno.h>
> >  #include <stdint.h>
> >  #include <inttypes.h>
> > +#include <assert.h>
> >
> >  #include <rte_string_fns.h>
> >  #include <rte_log.h>
> > @@ -28,6 +29,8 @@
> >  #include "eventdev_pmd.h"
> >  #include "eventdev_trace.h"
> >
> > +static_assert(sizeof(struct rte_event) == 16, "Event structure size
> is not 16-bytes in size");
> > +
> >  static struct rte_eventdev rte_event_devices[RTE_EVENT_MAX_DEVS];
> 
> Please don't reinvent RTE_BUILD_BUG_ON().
> Instead fix that to be a static_assert()

I would say the opposite:
With our upgrade to the C11 standard, let's get rid of the RTE_BUILD_BUG_ON() workaround for the lack of static_assert() in older C standards.

Unfortunately, the static_assert(expression) variant without the "message" parameter, which would make our RTE_BUILD_BUG_ON() macro completely obsolete, requires C23. And I don't see how we can make this variant available with C11. So we probably have to wait until DPDK requires C23.

Until then, let's gradually phase out the DPDK-specific RTE_BUILD_BUG_ON() in favor of standard C's static_assert(), and live with the inconvenience of having to provide a message parameter for it.

Please also note that static_assert() can be used outside code blocks, which makes it handy for use in header files.
  

On Sun, 12 Nov 2023 09:30:24 +0100
Morten Brørup <mb@smartsharesystems.com> wrote:

> > > +static_assert(sizeof(struct rte_event) == 16, "Event structure size  
> > is not 16-bytes in size");  
> > > +
> > >  static struct rte_eventdev rte_event_devices[RTE_EVENT_MAX_DEVS];  
> > 
> > Please don't reinvent RTE_BUILD_BUG_ON().
> > Instead fix that to be a static_assert()  
> 
> I would say the opposite:
> With our upgrade to the C11 standard, let's get rid of the RTE_BUILD_BUG_ON() workaround for the lack of static_assert() in older C standards.
> 
> Unfortunately, the static_assert(expression) variant without the "message" parameter, which would make our RTE_BUILD_BUG_ON() macro completely obsolete, requires C23. And I don't see how we can make this variant available with C11. So we probably have to wait until DPDK requires C23.
> 
> Until then, let's gradually phase out the DPDK-specific RTE_BUILD_BUG_ON() in favor of standard C's static_assert(), and live with the inconvenience of having to provide a message parameter for it.
> 
> Please also note that static_assert() can be used outside code blocks, which makes it handy for use in header files.

If you look at my RFC, the message is just as good as the one in this code.
It ends up being stringified version of the expression. Which is more exact than the wording used in some other places.
  

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Monday, 13 November 2023 00.32
> 
> On Sun, 12 Nov 2023 09:30:24 +0100
> Morten Brørup <mb@smartsharesystems.com> wrote:
> 
> > > > +static_assert(sizeof(struct rte_event) == 16, "Event structure
> size
> > > is not 16-bytes in size");
> > > > +
> > > >  static struct rte_eventdev
> rte_event_devices[RTE_EVENT_MAX_DEVS];
> > >
> > > Please don't reinvent RTE_BUILD_BUG_ON().
> > > Instead fix that to be a static_assert()
> >
> > I would say the opposite:
> > With our upgrade to the C11 standard, let's get rid of the
> RTE_BUILD_BUG_ON() workaround for the lack of static_assert() in older
> C standards.
> >
> > Unfortunately, the static_assert(expression) variant without the
> "message" parameter, which would make our RTE_BUILD_BUG_ON() macro
> completely obsolete, requires C23. And I don't see how we can make this
> variant available with C11. So we probably have to wait until DPDK
> requires C23.
> >
> > Until then, let's gradually phase out the DPDK-specific
> RTE_BUILD_BUG_ON() in favor of standard C's static_assert(), and live
> with the inconvenience of having to provide a message parameter for it.
> >
> > Please also note that static_assert() can be used outside code
> blocks, which makes it handy for use in header files.
> 
> If you look at my RFC, the message is just as good as the one in this
> code.
> It ends up being stringified version of the expression. Which is more
> exact than the wording used in some other places.

I agree that the output of your RFC is better than that of static_assert(expr, msg).
I'm arguing that RTE_BUILD_BUG_ON() would be considered reinventing the wheel if introduced today, because the C11 standard already offers something similar. So we should prefer that over RTE_BUILD_BUG_ON(), not the other way around.

It's a difference in opinion. Both RTE_BUILD_BUG_ON() and static_assert() are viable; you seem to prefer the first, I prefer the latter.

Both occur in existing DPDK code, and Bruce happened to chose static_assert() for this patch.

The overall question is a choice between three options:
1. prefer using RTE_BUILD_BUG_ON() (using your RFC implementation),
2. phase out RTE_BUILD_BUG_ON() in favor of the C11 standard's static_assert(), or
3. continue using both?

I'm not strongly opposed to either of the three, as long as the community agrees.
  

On Mon, Nov 13, 2023 at 08:58:19AM +0100, Morten Brørup wrote:
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Monday, 13 November 2023 00.32
> > 
> > On Sun, 12 Nov 2023 09:30:24 +0100
> > Morten Brørup <mb@smartsharesystems.com> wrote:
> > 
> > > > > +static_assert(sizeof(struct rte_event) == 16, "Event structure
> > size
> > > > is not 16-bytes in size");
> > > > > +
> > > > >  static struct rte_eventdev
> > rte_event_devices[RTE_EVENT_MAX_DEVS];
> > > >
> > > > Please don't reinvent RTE_BUILD_BUG_ON().
> > > > Instead fix that to be a static_assert()
> > >
> > > I would say the opposite:
> > > With our upgrade to the C11 standard, let's get rid of the
> > RTE_BUILD_BUG_ON() workaround for the lack of static_assert() in older
> > C standards.
> > >
> > > Unfortunately, the static_assert(expression) variant without the
> > "message" parameter, which would make our RTE_BUILD_BUG_ON() macro
> > completely obsolete, requires C23. And I don't see how we can make this
> > variant available with C11. So we probably have to wait until DPDK
> > requires C23.
> > >
> > > Until then, let's gradually phase out the DPDK-specific
> > RTE_BUILD_BUG_ON() in favor of standard C's static_assert(), and live
> > with the inconvenience of having to provide a message parameter for it.
> > >
> > > Please also note that static_assert() can be used outside code
> > blocks, which makes it handy for use in header files.
> > 
> > If you look at my RFC, the message is just as good as the one in this
> > code.
> > It ends up being stringified version of the expression. Which is more
> > exact than the wording used in some other places.
> 
> I agree that the output of your RFC is better than that of static_assert(expr, msg).
> I'm arguing that RTE_BUILD_BUG_ON() would be considered reinventing the wheel if introduced today, because the C11 standard already offers something similar. So we should prefer that over RTE_BUILD_BUG_ON(), not the other way around.
> 
> It's a difference in opinion. Both RTE_BUILD_BUG_ON() and static_assert() are viable; you seem to prefer the first, I prefer the latter.
> 
> Both occur in existing DPDK code, and Bruce happened to chose static_assert() for this patch.
> 
> The overall question is a choice between three options:
> 1. prefer using RTE_BUILD_BUG_ON() (using your RFC implementation),
> 2. phase out RTE_BUILD_BUG_ON() in favor of the C11 standard's static_assert(), or
> 3. continue using both?
> 
> I'm not strongly opposed to either of the three, as long as the community agrees.
> 

not a strong preference, but i see no reason to maintain macros when the
standard offers nearly equivalent.

* keep RTE_BUILD_BUG_ON
* discourage new additions using RTE_BUILD_BUG_ON with checkpatches
* convert to static_assert gradually over time

glad to see the conversion picked up real problems though, thank you
stephen for doing this.