[v3] eventdev: ensure 16-byte alignment for events
Checks
Commit Message
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>
---
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.h | 6 +++++-
2 files changed, 9 insertions(+), 1 deletion(-)
Comments
> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Friday, 6 October 2023 11.45
>
> 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>
>
> ---
[...]
> +_Static_assert(sizeof(struct rte_event) == 16, "Event structure size is not 16-bytes in size");
Thank you for adding this extra check. We should have more of these.
NB: _Static_assert is deprecated in C23 [1], so for forward compatibility, you could use static_assert (which is available in <assert.h>) instead. Nice to have; feel free to ignore this comment.
[1]: https://en.cppreference.com/w/c/language/_Static_assert
On Fri, Oct 6, 2023 at 3:44 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > Sent: Friday, 6 October 2023 11.45
> >
> > 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>
> >
> > ---
>
> [...]
>
> > +_Static_assert(sizeof(struct rte_event) == 16, "Event structure size is not 16-bytes in size");
>
> Thank you for adding this extra check. We should have more of these.
Use existing RTE_BUILD_BUG_ON this on .c file instead of header file.
>
> NB: _Static_assert is deprecated in C23 [1], so for forward compatibility, you could use static_assert (which is available in <assert.h>) instead. Nice to have; feel free to ignore this comment.
>
> [1]: https://en.cppreference.com/w/c/language/_Static_assert
>
On Fri, Oct 06, 2023 at 12:13:54PM +0200, Morten Brørup wrote:
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > Sent: Friday, 6 October 2023 11.45
> >
> > 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>
> >
> > ---
>
> [...]
>
> > +_Static_assert(sizeof(struct rte_event) == 16, "Event structure size is not 16-bytes in size");
>
> Thank you for adding this extra check. We should have more of these.
>
> NB: _Static_assert is deprecated in C23 [1], so for forward compatibility, you could use static_assert (which is available in <assert.h>) instead. Nice to have; feel free to ignore this comment.
>
Is the availability in assert.h backward compatible with C11, since the
link you posted seems to imply that "static_assert" is only from C23
onwards?
On Fri, Oct 06, 2023 at 03:46:21PM +0530, Jerin Jacob wrote:
> On Fri, Oct 6, 2023 at 3:44 PM Morten Brørup <mb@smartsharesystems.com> wrote:
> >
> > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > Sent: Friday, 6 October 2023 11.45
> > >
> > > 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>
> > >
> > > ---
> >
> > [...]
> >
> > > +_Static_assert(sizeof(struct rte_event) == 16, "Event structure size is not 16-bytes in size");
> >
> > Thank you for adding this extra check. We should have more of these.
>
> Use existing RTE_BUILD_BUG_ON this on .c file instead of header file.
>
Ok to move to a C file, but I think using static_asserts are better than
using the old macro tricks of negatively sized arrays.
> >
> > NB: _Static_assert is deprecated in C23 [1], so for forward compatibility, you could use static_assert (which is available in <assert.h>) instead. Nice to have; feel free to ignore this comment.
> >
> > [1]: https://en.cppreference.com/w/c/language/_Static_assert
> >
On Fri, Oct 6, 2023 at 3:49 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Fri, Oct 06, 2023 at 03:46:21PM +0530, Jerin Jacob wrote:
> > On Fri, Oct 6, 2023 at 3:44 PM Morten Brørup <mb@smartsharesystems.com> wrote:
> > >
> > > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > > Sent: Friday, 6 October 2023 11.45
> > > >
> > > > 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>
> > > >
> > > > ---
> > >
> > > [...]
> > >
> > > > +_Static_assert(sizeof(struct rte_event) == 16, "Event structure size is not 16-bytes in size");
> > >
> > > Thank you for adding this extra check. We should have more of these.
> >
> > Use existing RTE_BUILD_BUG_ON this on .c file instead of header file.
> >
>
> Ok to move to a C file, but I think using static_asserts are better than
> using the old macro tricks of negatively sized arrays.
No strong opinion on the API. I just told because compatibility
discussions came in.
Key is to move to .c file.
>
> > >
> > > NB: _Static_assert is deprecated in C23 [1], so for forward compatibility, you could use static_assert (which is available in <assert.h>) instead. Nice to have; feel free to ignore this comment.
> > >
> > > [1]: https://en.cppreference.com/w/c/language/_Static_assert
> > >
On Fri, Oct 06, 2023 at 03:54:26PM +0530, Jerin Jacob wrote:
> On Fri, Oct 6, 2023 at 3:49 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Fri, Oct 06, 2023 at 03:46:21PM +0530, Jerin Jacob wrote:
> > > On Fri, Oct 6, 2023 at 3:44 PM Morten Brørup <mb@smartsharesystems.com> wrote:
> > > >
> > > > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > > > Sent: Friday, 6 October 2023 11.45
> > > > >
> > > > > 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>
> > > > >
> > > > > ---
> > > >
> > > > [...]
> > > >
> > > > > +_Static_assert(sizeof(struct rte_event) == 16, "Event structure size is not 16-bytes in size");
> > > >
> > > > Thank you for adding this extra check. We should have more of these.
> > >
> > > Use existing RTE_BUILD_BUG_ON this on .c file instead of header file.
> > >
> >
> > Ok to move to a C file, but I think using static_asserts are better than
> > using the old macro tricks of negatively sized arrays.
>
> No strong opinion on the API. I just told because compatibility
> discussions came in.
> Key is to move to .c file.
>
Thanks. V4 coming up....
> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Friday, 6 October 2023 12.17
ev: ensure 16-byte alignment for events
>
> On Fri, Oct 06, 2023 at 12:13:54PM +0200, Morten Brørup wrote:
> > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > Sent: Friday, 6 October 2023 11.45
> > >
> > > 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>
> > >
> > > ---
> >
> > [...]
> >
> > > +_Static_assert(sizeof(struct rte_event) == 16, "Event structure size is
> not 16-bytes in size");
> >
> > Thank you for adding this extra check. We should have more of these.
> >
> > NB: _Static_assert is deprecated in C23 [1], so for forward compatibility,
> you could use static_assert (which is available in <assert.h>) instead. Nice
> to have; feel free to ignore this comment.
> >
> Is the availability in assert.h backward compatible with C11, since the
> link you posted seems to imply that "static_assert" is only from C23
> onwards?
Yes, the link mentions "static_assert" being available for C11 as a convenience macro in assert.h.
I had to read the link very carefully to get this. I guess I'm not the only one. :-)
I don't object to moving it to the .c file. However, I think it's convenient for readability to have the static_assert close to the thing it checks, and/or close to any code that relies on the assumption it checks.
On Fri, Oct 06, 2023 at 12:35:26PM +0200, Morten Brørup wrote:
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > Sent: Friday, 6 October 2023 12.17
> ev: ensure 16-byte alignment for events
> >
> > On Fri, Oct 06, 2023 at 12:13:54PM +0200, Morten Brørup wrote:
> > > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > > Sent: Friday, 6 October 2023 11.45
> > > >
> > > > 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>
> > > >
> > > > ---
> > >
> > > [...]
> > >
> > > > +_Static_assert(sizeof(struct rte_event) == 16, "Event structure size is
> > not 16-bytes in size");
> > >
> > > Thank you for adding this extra check. We should have more of these.
> > >
> > > NB: _Static_assert is deprecated in C23 [1], so for forward compatibility,
> > you could use static_assert (which is available in <assert.h>) instead. Nice
> > to have; feel free to ignore this comment.
> > >
> > Is the availability in assert.h backward compatible with C11, since the
> > link you posted seems to imply that "static_assert" is only from C23
> > onwards?
>
> Yes, the link mentions "static_assert" being available for C11 as a convenience macro in assert.h.
>
> I had to read the link very carefully to get this. I guess I'm not the only one. :-)
>
I missed that in the link, but I did have a read of the header file itself
to see that there was a macro there with C11 guards. :-)
> I don't object to moving it to the .c file. However, I think it's convenient for readability to have the static_assert close to the thing it checks, and/or close to any code that relies on the assumption it checks.
>
I'm ok either way, and happy enough to have the check in the C file. I
suppose it saves cluttering up the public header file with checks that
should not be relevant to the end-user, and are only for DPDK developers.
Anyway, v4 sent, hopefully with all concerns addressed.
/Bruce
@@ -209,6 +209,10 @@ ABI Changes
fields, to move ``rxq`` and ``txq`` fields, to change the size of
``reserved1`` and ``reserved2`` fields.
+* The ``rte_event`` structure, used by eventdev library and DPDK "event" class drivers,
+ is now 16-byte aligned, as well as being 16-bytes in size.
+ In previous releases, the structure only required 8-byte alignment.
+
Known Issues
------------
@@ -1284,6 +1284,8 @@ struct rte_event_vector {
/**
* The generic *rte_event* structure to hold the event attributes
* for dequeue and enqueue operation
+ *
+ * This structure must be kept at 16-bytes in size, and has 16-byte alignment.
*/
struct rte_event {
/** WORD0 */
@@ -1356,7 +1358,9 @@ struct rte_event {
struct rte_event_vector *vec;
/**< Event vector pointer. */
};
-};
+} __rte_aligned(16);
+
+_Static_assert(sizeof(struct rte_event) == 16, "Event structure size is not 16-bytes in size");
/* Ethdev Rx adapter capability bitmap flags */
#define RTE_EVENT_ETH_RX_ADAPTER_CAP_INTERNAL_PORT 0x1