[dpdk-dev] [EXT] Re: [RFC 11/15] eventdev: reserve fields in timer object

[Carrillo, Erik G]

> -----Original Message-----
> From: Pavan Nikhilesh Bhagavatula <pbhagavatula at marvell.com>
> Sent: Wednesday, September 1, 2021 1:48 AM
> To: Stephen Hemminger <stephen at networkplumber.org>; Carrillo, Erik G
> <erik.g.carrillo at intel.com>
> Cc: Jerin Jacob Kollanukkaran <jerinj at marvell.com>; Ananyev, Konstantin
> <konstantin.ananyev at intel.com>; dev at 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
> ><pbhagavatula at marvell.com> wrote:
> >
> >> From: Pavan Nikhilesh <pbhagavatula at 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 <pbhagavatula at 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.