DPDK patches and discussions
 help / color / mirror / Atom feed
* [PATCH] eventdev: fix alignment padding
@ 2023-04-18 10:45 Sivaprasad Tummala
  2023-04-18 11:06 ` Morten Brørup
  2023-04-18 12:30 ` Mattias Rönnblom
  0 siblings, 2 replies; 13+ messages in thread
From: Sivaprasad Tummala @ 2023-04-18 10:45 UTC (permalink / raw)
  To: jerinj; +Cc: dev, mattias.ronnblom

fixed the padding required to align to cacheline size.

Fixes: 54f17843a887 ("eventdev: add port maintenance API")
Cc: mattias.ronnblom@ericsson.com

Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com>
---
 lib/eventdev/rte_eventdev_core.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/eventdev/rte_eventdev_core.h b/lib/eventdev/rte_eventdev_core.h
index c328bdbc82..c27a52ccc0 100644
--- a/lib/eventdev/rte_eventdev_core.h
+++ b/lib/eventdev/rte_eventdev_core.h
@@ -65,7 +65,7 @@ struct rte_event_fp_ops {
 	/**< PMD Tx adapter enqueue same destination function. */
 	event_crypto_adapter_enqueue_t ca_enqueue;
 	/**< PMD Crypto adapter enqueue function. */
-	uintptr_t reserved[6];
+	uintptr_t reserved[5];
 } __rte_cache_aligned;
 
 extern struct rte_event_fp_ops rte_event_fp_ops[RTE_EVENT_MAX_DEVS];
-- 
2.34.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH] eventdev: fix alignment padding
  2023-04-18 10:45 [PATCH] eventdev: fix alignment padding Sivaprasad Tummala
@ 2023-04-18 11:06 ` Morten Brørup
  2023-04-18 12:40   ` Mattias Rönnblom
  2023-04-18 12:30 ` Mattias Rönnblom
  1 sibling, 1 reply; 13+ messages in thread
From: Morten Brørup @ 2023-04-18 11:06 UTC (permalink / raw)
  To: Sivaprasad Tummala, jerinj; +Cc: dev, mattias.ronnblom

> From: Sivaprasad Tummala [mailto:sivaprasad.tummala@amd.com]
> Sent: Tuesday, 18 April 2023 12.46
> 
> fixed the padding required to align to cacheline size.
> 
> Fixes: 54f17843a887 ("eventdev: add port maintenance API")
> Cc: mattias.ronnblom@ericsson.com
> 
> Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com>
> ---
>  lib/eventdev/rte_eventdev_core.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/eventdev/rte_eventdev_core.h
> b/lib/eventdev/rte_eventdev_core.h
> index c328bdbc82..c27a52ccc0 100644
> --- a/lib/eventdev/rte_eventdev_core.h
> +++ b/lib/eventdev/rte_eventdev_core.h
> @@ -65,7 +65,7 @@ struct rte_event_fp_ops {
>  	/**< PMD Tx adapter enqueue same destination function. */
>  	event_crypto_adapter_enqueue_t ca_enqueue;
>  	/**< PMD Crypto adapter enqueue function. */
> -	uintptr_t reserved[6];
> +	uintptr_t reserved[5];
>  } __rte_cache_aligned;

This fix changes the size (reduces it by one cache line) of the elements in the public rte_event_fp_ops array, and thus breaks the ABI.

BTW, the patch it fixes, which was dated November 2021, also broke the ABI.

> 
>  extern struct rte_event_fp_ops rte_event_fp_ops[RTE_EVENT_MAX_DEVS];
> --
> 2.34.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] eventdev: fix alignment padding
  2023-04-18 10:45 [PATCH] eventdev: fix alignment padding Sivaprasad Tummala
  2023-04-18 11:06 ` Morten Brørup
@ 2023-04-18 12:30 ` Mattias Rönnblom
  2023-04-18 14:07   ` Morten Brørup
  1 sibling, 1 reply; 13+ messages in thread
From: Mattias Rönnblom @ 2023-04-18 12:30 UTC (permalink / raw)
  To: Sivaprasad Tummala, jerinj; +Cc: dev

On 2023-04-18 12:45, Sivaprasad Tummala wrote:
> fixed the padding required to align to cacheline size.
> 

What's the point in having this structure cache-line aligned? False 
sharing is a non-issue, since this is more or less a read only struct.

This is not so much a comment on your patch, but the __rte_cache_aligned 
attribute.

> Fixes: 54f17843a887 ("eventdev: add port maintenance API")
> Cc: mattias.ronnblom@ericsson.com
> 
> Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com>
> ---
>   lib/eventdev/rte_eventdev_core.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/eventdev/rte_eventdev_core.h b/lib/eventdev/rte_eventdev_core.h
> index c328bdbc82..c27a52ccc0 100644
> --- a/lib/eventdev/rte_eventdev_core.h
> +++ b/lib/eventdev/rte_eventdev_core.h
> @@ -65,7 +65,7 @@ struct rte_event_fp_ops {
>   	/**< PMD Tx adapter enqueue same destination function. */
>   	event_crypto_adapter_enqueue_t ca_enqueue;
>   	/**< PMD Crypto adapter enqueue function. */
> -	uintptr_t reserved[6];
> +	uintptr_t reserved[5];
>   } __rte_cache_aligned;
>   
>   extern struct rte_event_fp_ops rte_event_fp_ops[RTE_EVENT_MAX_DEVS];


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] eventdev: fix alignment padding
  2023-04-18 11:06 ` Morten Brørup
@ 2023-04-18 12:40   ` Mattias Rönnblom
  0 siblings, 0 replies; 13+ messages in thread
From: Mattias Rönnblom @ 2023-04-18 12:40 UTC (permalink / raw)
  To: Morten Brørup, Sivaprasad Tummala, jerinj; +Cc: dev

On 2023-04-18 13:06, Morten Brørup wrote:
>> From: Sivaprasad Tummala [mailto:sivaprasad.tummala@amd.com]
>> Sent: Tuesday, 18 April 2023 12.46
>>
>> fixed the padding required to align to cacheline size.
>>
>> Fixes: 54f17843a887 ("eventdev: add port maintenance API")
>> Cc: mattias.ronnblom@ericsson.com
>>
>> Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com>
>> ---
>>   lib/eventdev/rte_eventdev_core.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/eventdev/rte_eventdev_core.h
>> b/lib/eventdev/rte_eventdev_core.h
>> index c328bdbc82..c27a52ccc0 100644
>> --- a/lib/eventdev/rte_eventdev_core.h
>> +++ b/lib/eventdev/rte_eventdev_core.h
>> @@ -65,7 +65,7 @@ struct rte_event_fp_ops {
>>   	/**< PMD Tx adapter enqueue same destination function. */
>>   	event_crypto_adapter_enqueue_t ca_enqueue;
>>   	/**< PMD Crypto adapter enqueue function. */
>> -	uintptr_t reserved[6];
>> +	uintptr_t reserved[5];
>>   } __rte_cache_aligned;
> 
> This fix changes the size (reduces it by one cache line) of the elements in the public rte_event_fp_ops array, and thus breaks the ABI.
> 
> BTW, the patch it fixes, which was dated November 2021, also broke the ABI.

21.11 has a new ABI version, so that's not an issue.

> 
>>
>>   extern struct rte_event_fp_ops rte_event_fp_ops[RTE_EVENT_MAX_DEVS];
>> --
>> 2.34.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH] eventdev: fix alignment padding
  2023-04-18 12:30 ` Mattias Rönnblom
@ 2023-04-18 14:07   ` Morten Brørup
  2023-04-18 15:16     ` Mattias Rönnblom
  0 siblings, 1 reply; 13+ messages in thread
From: Morten Brørup @ 2023-04-18 14:07 UTC (permalink / raw)
  To: Mattias Rönnblom, Sivaprasad Tummala, jerinj; +Cc: dev

> From: Mattias Rönnblom [mailto:mattias.ronnblom@ericsson.com]
> Sent: Tuesday, 18 April 2023 14.31
> 
> On 2023-04-18 12:45, Sivaprasad Tummala wrote:
> > fixed the padding required to align to cacheline size.
> >
> 
> What's the point in having this structure cache-line aligned? False
> sharing is a non-issue, since this is more or less a read only struct.
> 
> This is not so much a comment on your patch, but the __rte_cache_aligned
> attribute.

When the structure is cache aligned, an individual entry in the array does not unnecessarily cross a cache line border. With 16 pointers and aligned, it uses exactly two cache lines. If unaligned, it may span three cache lines.

> 
> > Fixes: 54f17843a887 ("eventdev: add port maintenance API")
> > Cc: mattias.ronnblom@ericsson.com
> >
> > Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com>
> > ---
> >   lib/eventdev/rte_eventdev_core.h | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/eventdev/rte_eventdev_core.h
> b/lib/eventdev/rte_eventdev_core.h
> > index c328bdbc82..c27a52ccc0 100644
> > --- a/lib/eventdev/rte_eventdev_core.h
> > +++ b/lib/eventdev/rte_eventdev_core.h
> > @@ -65,7 +65,7 @@ struct rte_event_fp_ops {
> >   	/**< PMD Tx adapter enqueue same destination function. */
> >   	event_crypto_adapter_enqueue_t ca_enqueue;
> >   	/**< PMD Crypto adapter enqueue function. */
> > -	uintptr_t reserved[6];
> > +	uintptr_t reserved[5];
> >   } __rte_cache_aligned;
> >
> >   extern struct rte_event_fp_ops rte_event_fp_ops[RTE_EVENT_MAX_DEVS];


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] eventdev: fix alignment padding
  2023-04-18 14:07   ` Morten Brørup
@ 2023-04-18 15:16     ` Mattias Rönnblom
  2023-04-19  6:38       ` Morten Brørup
  2023-05-17 13:20       ` Jerin Jacob
  0 siblings, 2 replies; 13+ messages in thread
From: Mattias Rönnblom @ 2023-04-18 15:16 UTC (permalink / raw)
  To: Morten Brørup, Sivaprasad Tummala, jerinj; +Cc: dev

On 2023-04-18 16:07, Morten Brørup wrote:
>> From: Mattias Rönnblom [mailto:mattias.ronnblom@ericsson.com]
>> Sent: Tuesday, 18 April 2023 14.31
>>
>> On 2023-04-18 12:45, Sivaprasad Tummala wrote:
>>> fixed the padding required to align to cacheline size.
>>>
>>
>> What's the point in having this structure cache-line aligned? False
>> sharing is a non-issue, since this is more or less a read only struct.
>>
>> This is not so much a comment on your patch, but the __rte_cache_aligned
>> attribute.
> 
> When the structure is cache aligned, an individual entry in the array does not unnecessarily cross a cache line border. With 16 pointers and aligned, it uses exactly two cache lines. If unaligned, it may span three cache lines.
> 
An *element* in the reserved uint64_t array won't span across two cache 
lines, regardless if __rte_cache_aligned is specified or not. You would 
need a packed struct for that to occur, plus the reserved array field 
being preceded by some appropriately-sized fields.

The only effect __rte_cache_aligned has on this particular struct is 
that if you instantiate the struct on the stack, or as a static 
variable, it will be cache-line aligned. That effect you can get by 
specifying the attribute when you define the variable, and you will save 
some space (by having smaller elements). In this case it doesn't matter 
if the array is compact or not, since an application is likely to only 
use one of the members in the array.

It also doesn't matter of the struct is two or three cache lines, as 
long as only the first two are used.

>>
>>> Fixes: 54f17843a887 ("eventdev: add port maintenance API")
>>> Cc: mattias.ronnblom@ericsson.com
>>>
>>> Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com>
>>> ---
>>>    lib/eventdev/rte_eventdev_core.h | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/lib/eventdev/rte_eventdev_core.h
>> b/lib/eventdev/rte_eventdev_core.h
>>> index c328bdbc82..c27a52ccc0 100644
>>> --- a/lib/eventdev/rte_eventdev_core.h
>>> +++ b/lib/eventdev/rte_eventdev_core.h
>>> @@ -65,7 +65,7 @@ struct rte_event_fp_ops {
>>>    	/**< PMD Tx adapter enqueue same destination function. */
>>>    	event_crypto_adapter_enqueue_t ca_enqueue;
>>>    	/**< PMD Crypto adapter enqueue function. */
>>> -	uintptr_t reserved[6];
>>> +	uintptr_t reserved[5];
>>>    } __rte_cache_aligned;
>>>
>>>    extern struct rte_event_fp_ops rte_event_fp_ops[RTE_EVENT_MAX_DEVS];
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH] eventdev: fix alignment padding
  2023-04-18 15:16     ` Mattias Rönnblom
@ 2023-04-19  6:38       ` Morten Brørup
  2023-05-17 13:20       ` Jerin Jacob
  1 sibling, 0 replies; 13+ messages in thread
From: Morten Brørup @ 2023-04-19  6:38 UTC (permalink / raw)
  To: Mattias Rönnblom, Sivaprasad Tummala, jerinj; +Cc: dev

> From: Mattias Rönnblom [mailto:mattias.ronnblom@ericsson.com]
> Sent: Tuesday, 18 April 2023 17.17
> 
> On 2023-04-18 16:07, Morten Brørup wrote:
> >> From: Mattias Rönnblom [mailto:mattias.ronnblom@ericsson.com]
> >> Sent: Tuesday, 18 April 2023 14.31
> >>
> >> On 2023-04-18 12:45, Sivaprasad Tummala wrote:
> >>> fixed the padding required to align to cacheline size.
> >>>
> >>
> >> What's the point in having this structure cache-line aligned? False
> >> sharing is a non-issue, since this is more or less a read only struct.
> >>
> >> This is not so much a comment on your patch, but the __rte_cache_aligned
> >> attribute.
> >
> > When the structure is cache aligned, an individual entry in the array does
> not unnecessarily cross a cache line border. With 16 pointers and aligned, it
> uses exactly two cache lines. If unaligned, it may span three cache lines.
> >
> An *element* in the reserved uint64_t array won't span across two cache
> lines, regardless if __rte_cache_aligned is specified or not. You would
> need a packed struct for that to occur, plus the reserved array field
> being preceded by some appropriately-sized fields.

What I wrote above made no sense, and I agree with everything in your response.

However, I meant to write "an individual entry in the rte_event_fp_ops[RTE_EVENT_MAX_DEVS] array", so please re-read my comment with that in mind.

There are similar arrays, e.g. for Ethdev, where the same alignment goal applies.

> 
> The only effect __rte_cache_aligned has on this particular struct is
> that if you instantiate the struct on the stack, or as a static
> variable, it will be cache-line aligned.

Or as elements in an array, such as rte_event_fp_ops[RTE_EVENT_MAX_DEVS].

> That effect you can get by
> specifying the attribute when you define the variable, and you will save
> some space (by having smaller elements). In this case it doesn't matter
> if the array is compact or not, since an application is likely to only
> use one of the members in the array.
> 
> It also doesn't matter of the struct is two or three cache lines, as
> long as only the first two are used.

I think the drivers are likely to use only one function pointer at a time, but they are likely to use the data pointer at the same time. So, when the struct is used in an array, cache aligning the struct prevents the data pointer from ending up as the last pointer in a preceding cache line.

> 
> >>
> >>> Fixes: 54f17843a887 ("eventdev: add port maintenance API")
> >>> Cc: mattias.ronnblom@ericsson.com
> >>>
> >>> Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com>
> >>> ---
> >>>    lib/eventdev/rte_eventdev_core.h | 2 +-
> >>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/lib/eventdev/rte_eventdev_core.h
> >> b/lib/eventdev/rte_eventdev_core.h
> >>> index c328bdbc82..c27a52ccc0 100644
> >>> --- a/lib/eventdev/rte_eventdev_core.h
> >>> +++ b/lib/eventdev/rte_eventdev_core.h
> >>> @@ -65,7 +65,7 @@ struct rte_event_fp_ops {
> >>>    	/**< PMD Tx adapter enqueue same destination function. */
> >>>    	event_crypto_adapter_enqueue_t ca_enqueue;
> >>>    	/**< PMD Crypto adapter enqueue function. */
> >>> -	uintptr_t reserved[6];
> >>> +	uintptr_t reserved[5];
> >>>    } __rte_cache_aligned;
> >>>
> >>>    extern struct rte_event_fp_ops rte_event_fp_ops[RTE_EVENT_MAX_DEVS];
> >


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] eventdev: fix alignment padding
  2023-04-18 15:16     ` Mattias Rönnblom
  2023-04-19  6:38       ` Morten Brørup
@ 2023-05-17 13:20       ` Jerin Jacob
  2023-05-17 13:35         ` Morten Brørup
  1 sibling, 1 reply; 13+ messages in thread
From: Jerin Jacob @ 2023-05-17 13:20 UTC (permalink / raw)
  To: Mattias Rönnblom; +Cc: Morten Brørup, Sivaprasad Tummala, jerinj, dev

On Tue, Apr 18, 2023 at 8:46 PM Mattias Rönnblom
<mattias.ronnblom@ericsson.com> wrote:
>
> On 2023-04-18 16:07, Morten Brørup wrote:
> >> From: Mattias Rönnblom [mailto:mattias.ronnblom@ericsson.com]
> >> Sent: Tuesday, 18 April 2023 14.31
> >>
> >> On 2023-04-18 12:45, Sivaprasad Tummala wrote:
> >>> fixed the padding required to align to cacheline size.
> >>>
> >>
> >> What's the point in having this structure cache-line aligned? False
> >> sharing is a non-issue, since this is more or less a read only struct.
> >>
> >> This is not so much a comment on your patch, but the __rte_cache_aligned
> >> attribute.
> >
> > When the structure is cache aligned, an individual entry in the array does not unnecessarily cross a cache line border. With 16 pointers and aligned, it uses exactly two cache lines. If unaligned, it may span three cache lines.
> >
> An *element* in the reserved uint64_t array won't span across two cache
> lines, regardless if __rte_cache_aligned is specified or not. You would
> need a packed struct for that to occur, plus the reserved array field
> being preceded by some appropriately-sized fields.
>
> The only effect __rte_cache_aligned has on this particular struct is
> that if you instantiate the struct on the stack, or as a static
> variable, it will be cache-line aligned. That effect you can get by
> specifying the attribute when you define the variable, and you will save
> some space (by having smaller elements). In this case it doesn't matter
> if the array is compact or not, since an application is likely to only
> use one of the members in the array.
>
> It also doesn't matter of the struct is two or three cache lines, as
> long as only the first two are used.


Discussions stalled at this point.

Hi Shiva,

Marking this patch as rejected. If you think the other way, Please
change patchwork status and let's discuss more here.



>
> >>
> >>> Fixes: 54f17843a887 ("eventdev: add port maintenance API")
> >>> Cc: mattias.ronnblom@ericsson.com
> >>>
> >>> Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com>
> >>> ---
> >>>    lib/eventdev/rte_eventdev_core.h | 2 +-
> >>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/lib/eventdev/rte_eventdev_core.h
> >> b/lib/eventdev/rte_eventdev_core.h
> >>> index c328bdbc82..c27a52ccc0 100644
> >>> --- a/lib/eventdev/rte_eventdev_core.h
> >>> +++ b/lib/eventdev/rte_eventdev_core.h
> >>> @@ -65,7 +65,7 @@ struct rte_event_fp_ops {
> >>>     /**< PMD Tx adapter enqueue same destination function. */
> >>>     event_crypto_adapter_enqueue_t ca_enqueue;
> >>>     /**< PMD Crypto adapter enqueue function. */
> >>> -   uintptr_t reserved[6];
> >>> +   uintptr_t reserved[5];
> >>>    } __rte_cache_aligned;
> >>>
> >>>    extern struct rte_event_fp_ops rte_event_fp_ops[RTE_EVENT_MAX_DEVS];
> >
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH] eventdev: fix alignment padding
  2023-05-17 13:20       ` Jerin Jacob
@ 2023-05-17 13:35         ` Morten Brørup
  2023-05-23 15:15           ` Jerin Jacob
  0 siblings, 1 reply; 13+ messages in thread
From: Morten Brørup @ 2023-05-17 13:35 UTC (permalink / raw)
  To: Jerin Jacob, Mattias Rönnblom; +Cc: Sivaprasad Tummala, jerinj, dev

> From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> Sent: Wednesday, 17 May 2023 15.20
> 
> On Tue, Apr 18, 2023 at 8:46 PM Mattias Rönnblom
> <mattias.ronnblom@ericsson.com> wrote:
> >
> > On 2023-04-18 16:07, Morten Brørup wrote:
> > >> From: Mattias Rönnblom [mailto:mattias.ronnblom@ericsson.com]
> > >> Sent: Tuesday, 18 April 2023 14.31
> > >>
> > >> On 2023-04-18 12:45, Sivaprasad Tummala wrote:
> > >>> fixed the padding required to align to cacheline size.
> > >>>
> > >>
> > >> What's the point in having this structure cache-line aligned? False
> > >> sharing is a non-issue, since this is more or less a read only struct.
> > >>
> > >> This is not so much a comment on your patch, but the __rte_cache_aligned
> > >> attribute.
> > >
> > > When the structure is cache aligned, an individual entry in the array does
> not unnecessarily cross a cache line border. With 16 pointers and aligned, it
> uses exactly two cache lines. If unaligned, it may span three cache lines.
> > >
> > An *element* in the reserved uint64_t array won't span across two cache
> > lines, regardless if __rte_cache_aligned is specified or not. You would
> > need a packed struct for that to occur, plus the reserved array field
> > being preceded by some appropriately-sized fields.
> >
> > The only effect __rte_cache_aligned has on this particular struct is
> > that if you instantiate the struct on the stack, or as a static
> > variable, it will be cache-line aligned. That effect you can get by
> > specifying the attribute when you define the variable, and you will save
> > some space (by having smaller elements). In this case it doesn't matter
> > if the array is compact or not, since an application is likely to only
> > use one of the members in the array.
> >
> > It also doesn't matter of the struct is two or three cache lines, as
> > long as only the first two are used.
> 
> 
> Discussions stalled at this point.

Not stalled at this point. You seem to have missed my follow-up email clarifying why cache aligning is relevant:
http://inbox.dpdk.org/dev/98CBD80474FA8B44BF855DF32C47DC35D87897@smartserver.smartshare.dk/

But the patch still breaks the ABI, and thus should be postponed to 23.11.

> 
> Hi Shiva,
> 
> Marking this patch as rejected. If you think the other way, Please
> change patchwork status and let's discuss more here.

I am not taking any action regarding the status of this patch. I will leave that decision to Jerin and Shiva.

> 
> 
> 
> >
> > >>
> > >>> Fixes: 54f17843a887 ("eventdev: add port maintenance API")
> > >>> Cc: mattias.ronnblom@ericsson.com
> > >>>
> > >>> Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com>
> > >>> ---
> > >>>    lib/eventdev/rte_eventdev_core.h | 2 +-
> > >>>    1 file changed, 1 insertion(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/lib/eventdev/rte_eventdev_core.h
> > >> b/lib/eventdev/rte_eventdev_core.h
> > >>> index c328bdbc82..c27a52ccc0 100644
> > >>> --- a/lib/eventdev/rte_eventdev_core.h
> > >>> +++ b/lib/eventdev/rte_eventdev_core.h
> > >>> @@ -65,7 +65,7 @@ struct rte_event_fp_ops {
> > >>>     /**< PMD Tx adapter enqueue same destination function. */
> > >>>     event_crypto_adapter_enqueue_t ca_enqueue;
> > >>>     /**< PMD Crypto adapter enqueue function. */
> > >>> -   uintptr_t reserved[6];
> > >>> +   uintptr_t reserved[5];
> > >>>    } __rte_cache_aligned;
> > >>>
> > >>>    extern struct rte_event_fp_ops rte_event_fp_ops[RTE_EVENT_MAX_DEVS];
> > >
> >

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] eventdev: fix alignment padding
  2023-05-17 13:35         ` Morten Brørup
@ 2023-05-23 15:15           ` Jerin Jacob
  2023-08-02 16:19             ` Jerin Jacob
  0 siblings, 1 reply; 13+ messages in thread
From: Jerin Jacob @ 2023-05-23 15:15 UTC (permalink / raw)
  To: Morten Brørup; +Cc: Mattias Rönnblom, Sivaprasad Tummala, jerinj, dev

On Wed, May 17, 2023 at 7:05 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> > Sent: Wednesday, 17 May 2023 15.20
> >
> > On Tue, Apr 18, 2023 at 8:46 PM Mattias Rönnblom
> > <mattias.ronnblom@ericsson.com> wrote:
> > >
> > > On 2023-04-18 16:07, Morten Brørup wrote:
> > > >> From: Mattias Rönnblom [mailto:mattias.ronnblom@ericsson.com]
> > > >> Sent: Tuesday, 18 April 2023 14.31
> > > >>
> > > >> On 2023-04-18 12:45, Sivaprasad Tummala wrote:
> > > >>> fixed the padding required to align to cacheline size.
> > > >>>
> > > >>
> > > >> What's the point in having this structure cache-line aligned? False
> > > >> sharing is a non-issue, since this is more or less a read only struct.
> > > >>
> > > >> This is not so much a comment on your patch, but the __rte_cache_aligned
> > > >> attribute.
> > > >
> > > > When the structure is cache aligned, an individual entry in the array does
> > not unnecessarily cross a cache line border. With 16 pointers and aligned, it
> > uses exactly two cache lines. If unaligned, it may span three cache lines.
> > > >
> > > An *element* in the reserved uint64_t array won't span across two cache
> > > lines, regardless if __rte_cache_aligned is specified or not. You would
> > > need a packed struct for that to occur, plus the reserved array field
> > > being preceded by some appropriately-sized fields.
> > >
> > > The only effect __rte_cache_aligned has on this particular struct is
> > > that if you instantiate the struct on the stack, or as a static
> > > variable, it will be cache-line aligned. That effect you can get by
> > > specifying the attribute when you define the variable, and you will save
> > > some space (by having smaller elements). In this case it doesn't matter
> > > if the array is compact or not, since an application is likely to only
> > > use one of the members in the array.
> > >
> > > It also doesn't matter of the struct is two or three cache lines, as
> > > long as only the first two are used.
> >
> >
> > Discussions stalled at this point.
>
> Not stalled at this point. You seem to have missed my follow-up email clarifying why cache aligning is relevant:
> http://inbox.dpdk.org/dev/98CBD80474FA8B44BF855DF32C47DC35D87897@smartserver.smartshare.dk/
>
> But the patch still breaks the ABI, and thus should be postponed to 23.11.

Yes.

>
> >
> > Hi Shiva,
> >
> > Marking this patch as rejected. If you think the other way, Please
> > change patchwork status and let's discuss more here.
>
> I am not taking any action regarding the status of this patch. I will leave that decision to Jerin and Shiva.

It is good to merge.

Shiva,

Please send ABI change notice for this for 23.11 NOW.
Once it is Acked and merged. I will merge the patch for 23.11 release.

I am marking the patch as DEFERRED in patchwork and next release
window it will come as NEW in patchwork.

>
> >
> >
> >
> > >
> > > >>
> > > >>> Fixes: 54f17843a887 ("eventdev: add port maintenance API")
> > > >>> Cc: mattias.ronnblom@ericsson.com
> > > >>>
> > > >>> Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com>
> > > >>> ---
> > > >>>    lib/eventdev/rte_eventdev_core.h | 2 +-
> > > >>>    1 file changed, 1 insertion(+), 1 deletion(-)
> > > >>>
> > > >>> diff --git a/lib/eventdev/rte_eventdev_core.h
> > > >> b/lib/eventdev/rte_eventdev_core.h
> > > >>> index c328bdbc82..c27a52ccc0 100644
> > > >>> --- a/lib/eventdev/rte_eventdev_core.h
> > > >>> +++ b/lib/eventdev/rte_eventdev_core.h
> > > >>> @@ -65,7 +65,7 @@ struct rte_event_fp_ops {
> > > >>>     /**< PMD Tx adapter enqueue same destination function. */
> > > >>>     event_crypto_adapter_enqueue_t ca_enqueue;
> > > >>>     /**< PMD Crypto adapter enqueue function. */
> > > >>> -   uintptr_t reserved[6];
> > > >>> +   uintptr_t reserved[5];
> > > >>>    } __rte_cache_aligned;
> > > >>>
> > > >>>    extern struct rte_event_fp_ops rte_event_fp_ops[RTE_EVENT_MAX_DEVS];
> > > >
> > >

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] eventdev: fix alignment padding
  2023-05-23 15:15           ` Jerin Jacob
@ 2023-08-02 16:19             ` Jerin Jacob
  2023-08-08 10:24               ` Jerin Jacob
  0 siblings, 1 reply; 13+ messages in thread
From: Jerin Jacob @ 2023-08-02 16:19 UTC (permalink / raw)
  To: Morten Brørup; +Cc: Mattias Rönnblom, Sivaprasad Tummala, jerinj, dev

On Tue, May 23, 2023 at 8:45 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>
> On Wed, May 17, 2023 at 7:05 PM Morten Brørup <mb@smartsharesystems.com> wrote:
> >

> Shiva,
>
> Please send ABI change notice for this for 23.11 NOW.
> Once it is Acked and merged. I will merge the patch for 23.11 release.
>
> I am marking the patch as DEFERRED in patchwork and next release
> window it will come as NEW in patchwork.


Any objection to merge this?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] eventdev: fix alignment padding
  2023-08-02 16:19             ` Jerin Jacob
@ 2023-08-08 10:24               ` Jerin Jacob
  2023-08-08 10:25                 ` Jerin Jacob
  0 siblings, 1 reply; 13+ messages in thread
From: Jerin Jacob @ 2023-08-08 10:24 UTC (permalink / raw)
  To: Morten Brørup; +Cc: Mattias Rönnblom, Sivaprasad Tummala, jerinj, dev

On Wed, Aug 2, 2023 at 9:49 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>
> On Tue, May 23, 2023 at 8:45 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> >
> > On Wed, May 17, 2023 at 7:05 PM Morten Brørup <mb@smartsharesystems.com> wrote:
> > >
>
> > Shiva,
> >
> > Please send ABI change notice for this for 23.11 NOW.
> > Once it is Acked and merged. I will merge the patch for 23.11 release.
> >
> > I am marking the patch as DEFERRED in patchwork and next release
> > window it will come as NEW in patchwork.
>
>
> Any objection to merge this?


pahole output after the change,

[for-main]dell[dpdk-next-eventdev] $ pahole build/app/test/dpdk-test
-C rte_event_fp_ops
struct rte_event_fp_ops {
        void * *                   data;                 /*     0     8 */
        event_enqueue_t            enqueue;              /*     8     8 */
        event_enqueue_burst_t      enqueue_burst;        /*    16     8 */
        event_enqueue_burst_t      enqueue_new_burst;    /*    24     8 */
        event_enqueue_burst_t      enqueue_forward_burst; /*    32     8 */
        event_dequeue_t            dequeue;              /*    40     8 */
        event_dequeue_burst_t      dequeue_burst;        /*    48     8 */
        event_maintain_t           maintain;             /*    56     8 */
        /* --- cacheline 1 boundary (64 bytes) --- */
        event_tx_adapter_enqueue_t txa_enqueue;          /*    64     8 */
        event_tx_adapter_enqueue_t txa_enqueue_same_dest; /*    72     8 */
        event_crypto_adapter_enqueue_t ca_enqueue;       /*    80     8 */
        uintptr_t                  reserved[5];          /*    88    40 */

        /* size: 128, cachelines: 2, members: 12 */
} __attribute__((__aligned__(64)));

Acked-by: Jerin Jacob <jerinj@marvell.com>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] eventdev: fix alignment padding
  2023-08-08 10:24               ` Jerin Jacob
@ 2023-08-08 10:25                 ` Jerin Jacob
  0 siblings, 0 replies; 13+ messages in thread
From: Jerin Jacob @ 2023-08-08 10:25 UTC (permalink / raw)
  To: Morten Brørup; +Cc: Mattias Rönnblom, Sivaprasad Tummala, jerinj, dev

On Tue, Aug 8, 2023 at 3:54 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>
> On Wed, Aug 2, 2023 at 9:49 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> >
> > On Tue, May 23, 2023 at 8:45 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > >
> > > On Wed, May 17, 2023 at 7:05 PM Morten Brørup <mb@smartsharesystems.com> wrote:
> > > >
> >
> > > Shiva,
> > >
> > > Please send ABI change notice for this for 23.11 NOW.
> > > Once it is Acked and merged. I will merge the patch for 23.11 release.
> > >
> > > I am marking the patch as DEFERRED in patchwork and next release
> > > window it will come as NEW in patchwork.
> >
> >
> > Any objection to merge this?
>
>
> pahole output after the change,
>
> [for-main]dell[dpdk-next-eventdev] $ pahole build/app/test/dpdk-test
> -C rte_event_fp_ops
> struct rte_event_fp_ops {
>         void * *                   data;                 /*     0     8 */
>         event_enqueue_t            enqueue;              /*     8     8 */
>         event_enqueue_burst_t      enqueue_burst;        /*    16     8 */
>         event_enqueue_burst_t      enqueue_new_burst;    /*    24     8 */
>         event_enqueue_burst_t      enqueue_forward_burst; /*    32     8 */
>         event_dequeue_t            dequeue;              /*    40     8 */
>         event_dequeue_burst_t      dequeue_burst;        /*    48     8 */
>         event_maintain_t           maintain;             /*    56     8 */
>         /* --- cacheline 1 boundary (64 bytes) --- */
>         event_tx_adapter_enqueue_t txa_enqueue;          /*    64     8 */
>         event_tx_adapter_enqueue_t txa_enqueue_same_dest; /*    72     8 */
>         event_crypto_adapter_enqueue_t ca_enqueue;       /*    80     8 */
>         uintptr_t                  reserved[5];          /*    88    40 */
>
>         /* size: 128, cachelines: 2, members: 12 */
> } __attribute__((__aligned__(64)));
>
> Acked-by: Jerin Jacob <jerinj@marvell.com>


Applied to dpdk-next-net-eventdev/for-main. Thanks

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2023-08-08 10:25 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-18 10:45 [PATCH] eventdev: fix alignment padding Sivaprasad Tummala
2023-04-18 11:06 ` Morten Brørup
2023-04-18 12:40   ` Mattias Rönnblom
2023-04-18 12:30 ` Mattias Rönnblom
2023-04-18 14:07   ` Morten Brørup
2023-04-18 15:16     ` Mattias Rönnblom
2023-04-19  6:38       ` Morten Brørup
2023-05-17 13:20       ` Jerin Jacob
2023-05-17 13:35         ` Morten Brørup
2023-05-23 15:15           ` Jerin Jacob
2023-08-02 16:19             ` Jerin Jacob
2023-08-08 10:24               ` Jerin Jacob
2023-08-08 10:25                 ` Jerin Jacob

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).