diff mbox series

[RFC,11/15] eventdev: reserve fields in timer object

Message ID 20210823194020.1229-11-pbhagavatula@marvell.com (mailing list archive)
State Superseded, archived
Delegated to: Jerin Jacob
Headers
Series [RFC,01/15] eventdev: make driver interface as internal |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Pavan Nikhilesh Bhagavatula Aug. 23, 2021, 7:40 p.m. UTC 
  From: Pavan Nikhilesh <pbhagavatula@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@marvell.com>
---
 lib/eventdev/rte_event_timer_adapter.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Carrillo, Erik G Aug. 23, 2021, 8:42 p.m. UTC | #1 
Hi Pavan,

One comment in-line:

> -----Original Message-----
> From: pbhagavatula@marvell.com <pbhagavatula@marvell.com>
> Sent: Monday, August 23, 2021 2:40 PM
> To: jerinj@marvell.com; Carrillo, Erik G <erik.g.carrillo@intel.com>
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;
> Pavan Nikhilesh <pbhagavatula@marvell.com>
> Subject: [dpdk-dev] [RFC 11/15] eventdev: reserve fields in timer object
> 
> From: Pavan Nikhilesh <pbhagavatula@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@marvell.com>
> ---
>  lib/eventdev/rte_event_timer_adapter.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/eventdev/rte_event_timer_adapter.h
> b/lib/eventdev/rte_event_timer_adapter.h
> index cad6d3b4c5..9499460a61 100644
> --- a/lib/eventdev/rte_event_timer_adapter.h
> +++ b/lib/eventdev/rte_event_timer_adapter.h
> @@ -475,7 +475,7 @@ struct rte_event_timer {
>  	 *  - op: RTE_EVENT_OP_NEW
>  	 *  - event_type: RTE_EVENT_TYPE_TIMER
>  	 */
> -	volatile enum rte_event_timer_state state;
> +	enum rte_event_timer_state state;
>  	/**< State of the event timer. */
>  	uint64_t timeout_ticks;
>  	/**< Expiry timer ticks expressed in number of *timer_ticks_ns*
> from @@ -492,6 +492,8 @@ struct rte_event_timer {
>  	/**< Memory to store user specific metadata.
>  	 * The event timer adapter implementation should not modify this
> area.
>  	 */
> +	uint64_t rsvd[2];
> +	/**< Reserved fields for future use. */

This placement puts rsvd after the user_meta field, which should be last since it is a zero-length array.  Am I missing something?

Thanks,
Erik

>  } __rte_cache_aligned;
> 
>  typedef uint16_t (*rte_event_timer_arm_burst_t)(
> --
> 2.17.1
Pavan Nikhilesh Bhagavatula Aug. 24, 2021, 5:16 a.m. UTC | #2 
Hi Erik,

>Hi Pavan,
>
>One comment in-line:
>
>> -----Original Message-----
>> From: pbhagavatula@marvell.com <pbhagavatula@marvell.com>
>> Sent: Monday, August 23, 2021 2:40 PM
>> To: jerinj@marvell.com; Carrillo, Erik G <erik.g.carrillo@intel.com>
>> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
>dev@dpdk.org;
>> Pavan Nikhilesh <pbhagavatula@marvell.com>
>> Subject: [dpdk-dev] [RFC 11/15] eventdev: reserve fields in timer
>object
>>
>> From: Pavan Nikhilesh <pbhagavatula@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@marvell.com>
>> ---
>>  lib/eventdev/rte_event_timer_adapter.h | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/eventdev/rte_event_timer_adapter.h
>> b/lib/eventdev/rte_event_timer_adapter.h
>> index cad6d3b4c5..9499460a61 100644
>> --- a/lib/eventdev/rte_event_timer_adapter.h
>> +++ b/lib/eventdev/rte_event_timer_adapter.h
>> @@ -475,7 +475,7 @@ struct rte_event_timer {
>>  	 *  - op: RTE_EVENT_OP_NEW
>>  	 *  - event_type: RTE_EVENT_TYPE_TIMER
>>  	 */
>> -	volatile enum rte_event_timer_state state;
>> +	enum rte_event_timer_state state;
>>  	/**< State of the event timer. */
>>  	uint64_t timeout_ticks;
>>  	/**< Expiry timer ticks expressed in number of
>*timer_ticks_ns*
>> from @@ -492,6 +492,8 @@ struct rte_event_timer {
>>  	/**< Memory to store user specific metadata.
>>  	 * The event timer adapter implementation should not modify
>this
>> area.
>>  	 */
>> +	uint64_t rsvd[2];
>> +	/**< Reserved fields for future use. */
>
>This placement puts rsvd after the user_meta field, which should be last
>since it is a zero-length array.  Am I missing something?

My bad, I will fix it in next version.

>
>Thanks,
>Erik

Thanks,
Pavan.

>
>>  } __rte_cache_aligned;
>>
>>  typedef uint16_t (*rte_event_timer_arm_burst_t)(
>> --
>> 2.17.1
Stephen Hemminger Aug. 24, 2021, 3:10 p.m. UTC | #3 
On Tue, 24 Aug 2021 01:10:15 +0530
<pbhagavatula@marvell.com> wrote:

> From: Pavan Nikhilesh <pbhagavatula@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@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.

A better way to make structures internal opaque objects that
can be resized.  Why is rte_event_timer_adapter exposed in API?
Pavan Nikhilesh Bhagavatula Sept. 1, 2021, 6:48 a.m. UTC | #4 
>On Tue, 24 Aug 2021 01:10:15 +0530
><pbhagavatula@marvell.com> wrote:
>
>> From: Pavan Nikhilesh <pbhagavatula@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@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?

Thanks,
Pavan.
Carrillo, Erik G Sept. 7, 2021, 9:02 p.m. UTC | #5 
> -----Original Message-----
> From: Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>
> Sent: Wednesday, September 1, 2021 1:48 AM
> To: Stephen Hemminger <stephen@networkplumber.org>; Carrillo, Erik G
> <erik.g.carrillo@intel.com>
> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Ananyev, Konstantin
> <konstantin.ananyev@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
> ><pbhagavatula@marvell.com> wrote:
> >
> >> From: Pavan Nikhilesh <pbhagavatula@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@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.
Stephen Hemminger Sept. 7, 2021, 9:31 p.m. UTC | #6 
On Tue, 24 Aug 2021 01:10:15 +0530
<pbhagavatula@marvell.com> wrote:

> +	uint64_t rsvd[2];
> +	/**< Reserved fields for future use. */

Did you check that these are 0 in current code?
No. then they can't be safely used in the future.
diff mbox series

Patch

diff --git a/lib/eventdev/rte_event_timer_adapter.h b/lib/eventdev/rte_event_timer_adapter.h
index cad6d3b4c5..9499460a61 100644
--- a/lib/eventdev/rte_event_timer_adapter.h
+++ b/lib/eventdev/rte_event_timer_adapter.h
@@ -475,7 +475,7 @@  struct rte_event_timer {
 	 *  - op: RTE_EVENT_OP_NEW
 	 *  - event_type: RTE_EVENT_TYPE_TIMER
 	 */
-	volatile enum rte_event_timer_state state;
+	enum rte_event_timer_state state;
 	/**< State of the event timer. */
 	uint64_t timeout_ticks;
 	/**< Expiry timer ticks expressed in number of *timer_ticks_ns* from
@@ -492,6 +492,8 @@  struct rte_event_timer {
 	/**< Memory to store user specific metadata.
 	 * The event timer adapter implementation should not modify this area.
 	 */
+	uint64_t rsvd[2];
+	/**< Reserved fields for future use. */
 } __rte_cache_aligned;
 
 typedef uint16_t (*rte_event_timer_arm_burst_t)(