> -----Original Message-----
> From: Pavan Nikhilesh Bhagavatula <pbhagavat...@marvell.com>
> Sent: Wednesday, September 1, 2021 1:48 AM
> To: Stephen Hemminger <step...@networkplumber.org>; Carrillo, Erik G
> <erik.g.carri...@intel.com>
> Cc: Jerin Jacob Kollanukkaran <jer...@marvell.com>; Ananyev, Konstantin
> <konstantin.anan...@intel.com>; dev@dpdk.org
> Subject: RE: [EXT] Re: [dpdk-dev] [RFC 11/15] eventdev: reserve fields in
> timer object
>
> >On Tue, 24 Aug 2021 01:10:15 +0530
> ><pbhagavat...@marvell.com> wrote:
> >
> >> From: Pavan Nikhilesh <pbhagavat...@marvell.com>
> >>
> >> Reserve fields in rte_event_timer data structure to address future
> >> use cases.
> >> Also, remove volatile from rte_event_timer.
> >>
> >> Signed-off-by: Pavan Nikhilesh <pbhagavat...@marvell.com>
> >
> >Reserve fields are not a good idea. They don't solve future API/ABI
> >problems.
> >
> >The issue is that you need to zero them and check they are zero
> >otherwise they can't safely be used later. This happened with the
> >Linux kernel system calls where in several cases a flag field was added
> >for future.
> >The problem is that old programs would work with any garbage in the
> >flag field, and therefore the flag could not be extended.
>
> The change is in rte_event_timer which is a fastpath object similar to
> rte_mbuf.
> I think fast path objects don't have the above mentioned caveat.
>
> >
> >A better way to make structures internal opaque objects that can be
> >resized. Why is rte_event_timer_adapter exposed in API?
>
> rte_event_timer_adapter has similar API semantics of rte_mempool, the
> objects of the adapter are rte_event_timer objects which have information
> of the timer state.
>
> Erik,
>
> I think we should move the fields in current rte_event_timer structure
> around, currently we have
>
> struct rte_event_timer {
> struct rte_event ev;
> volatile enum rte_event_timer_state state;
> x-------x 4 byte hole x---------x
> uint64_t timeout_ticks;
> uint64_t impl_opaque[2];
> uint8_t user_meta[0];
> } __rte_cache_aligned;
>
> Move to
>
> struct rte_event_timer {
> struct rte_event ev;
> uint64_t timeout_ticks;
> uint64_t impl_opaque[2];
> uint64_t rsvd;
> enum rte_event_timer_state state;
> uint8_t user_meta[0];
> } __rte_cache_aligned;
>
> Since its cache aligned, the size doesn't change.
>
> Thoughts?
>
I'm not seeing any problem with rearranging the members. However, you
originally had " uint64_t rsvd[2];" and above, it's just one variable. Did
you mean to make it an array?
The following also appears to have no holes:
$ pahole -C rte_event_timer eventdev_rte_event_timer_adapter.c.o
struct rte_event_timer {
struct rte_event ev; /* 0 16 */
uint64_t timeout_ticks; /* 16 8 */
uint64_t impl_opaque[2]; /* 24 16 */
uint64_t rsvd[2]; /* 40 16 */
enum rte_event_timer_state state; /* 56 4 */
uint8_t user_meta[]; /* 60 0 */
/* size: 64, cachelines: 1, members: 6 */
/* padding: 4 */
} __attribute__((__aligned__(64)));
Thanks,
Erik
> Thanks,
> Pavan.
