DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH v1] devtools: update abi ignore for cryptodev
@ 2021-01-20 14:25 Ray Kinsella
  2021-01-20 15:41 ` Thomas Monjalon
  2021-01-26 11:55 ` Thomas Monjalon
  0 siblings, 2 replies; 9+ messages in thread
From: Ray Kinsella @ 2021-01-20 14:25 UTC (permalink / raw)
  To: Ray Kinsella, Neil Horman, Akhil Goyal, Konstantin Ananyev,
	Abhinandan Gujjar
  Cc: thomas, david.marchand, dev

Update the ignore entry for crytodev to use named fields instead of
bit positions.

Fixes: 1c3ffb9559

Signed-off-by: Ray Kinsella <mdr@ashroe.eu>
---
 devtools/libabigail.abignore | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
index 1dc84fa74b..1f17fbed58 100644
--- a/devtools/libabigail.abignore
+++ b/devtools/libabigail.abignore
@@ -15,4 +15,4 @@
 ; Ignore fields inserted in cacheline boundary of rte_cryptodev
 [suppress_type]
         name = rte_cryptodev
-        has_data_member_inserted_between = {0, 1023}
+        has_data_member_inserted_between = {offset_after(attached), end}
\ No newline at end of file
-- 
2.26.2


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

* Re: [dpdk-dev] [PATCH v1] devtools: update abi ignore for cryptodev
  2021-01-20 14:25 [dpdk-dev] [PATCH v1] devtools: update abi ignore for cryptodev Ray Kinsella
@ 2021-01-20 15:41 ` Thomas Monjalon
  2021-01-21 15:15   ` Dodji Seketeli
  2021-01-26 11:55 ` Thomas Monjalon
  1 sibling, 1 reply; 9+ messages in thread
From: Thomas Monjalon @ 2021-01-20 15:41 UTC (permalink / raw)
  To: dodji
  Cc: Ray Kinsella, Neil Horman, Akhil Goyal, Konstantin Ananyev,
	Abhinandan Gujjar, dev, david.marchand

Question to an expert, Dodji,

We have this structure:

struct rte_cryptodev {
	lot of fields...
	uint8_t attached : 1;
} __rte_cache_aligned;

Because of the cache alignment, there is enough padding in the struct
(no matter the size of the cache line) for adding two more pointers:

struct rte_cryptodev {
	lot of fields...
	uint8_t attached : 1;
	struct rte_cryptodev_cb_rcu *enq_cbs;
	struct rte_cryptodev_cb_rcu *deq_cbs;
} __rte_cache_aligned;

We checked manually that the ABI is still compatible.
Then I've added (quickly) a libabigail exception rule:

[suppress_type]
	name = rte_cryptodev
	has_data_member_inserted_between = {0, 1023}

Now we want to improve this rule to restrict the offsets
to the padding at the end of the struct only,
so we keep forbidding changes in existing fields,
and forbidding additions further the current struct size.
Is this new rule good?

	has_data_member_inserted_between = {offset_after(attached), end}

Do you confirm that the keyword "end" means the old reference size?

What else do we need to check for adding a new field in a padding?

Thank you


20/01/2021 15:25, Ray Kinsella:
> Update the ignore entry for crytodev to use named fields instead of
> bit positions.
> 
> Fixes: 1c3ffb9559
> 
> Signed-off-by: Ray Kinsella <mdr@ashroe.eu>
> ---
>  devtools/libabigail.abignore | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
> index 1dc84fa74b..1f17fbed58 100644
> --- a/devtools/libabigail.abignore
> +++ b/devtools/libabigail.abignore
> @@ -15,4 +15,4 @@
>  ; Ignore fields inserted in cacheline boundary of rte_cryptodev
>  [suppress_type]
>          name = rte_cryptodev
> -        has_data_member_inserted_between = {0, 1023}
> +        has_data_member_inserted_between = {offset_after(attached), end}





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

* Re: [dpdk-dev] [PATCH v1] devtools: update abi ignore for cryptodev
  2021-01-20 15:41 ` Thomas Monjalon
@ 2021-01-21 15:15   ` Dodji Seketeli
  2021-01-21 15:58     ` Thomas Monjalon
  0 siblings, 1 reply; 9+ messages in thread
From: Dodji Seketeli @ 2021-01-21 15:15 UTC (permalink / raw)
  To: Thomas Monjalon
  Cc: Ray Kinsella, Neil Horman, Akhil Goyal, Konstantin Ananyev,
	Abhinandan Gujjar, dev, david.marchand

Hello Thomas and others,

Thomas Monjalon <thomas@monjalon.net> writes:

> Question to an expert, Dodji,

Thanks for the kind words, but I am not an expert in anything, sadly.  I
am just trying to keep learning about these things ;-)

> We have this structure:
>
> struct rte_cryptodev {
> 	lot of fields...
> 	uint8_t attached : 1;
> } __rte_cache_aligned;
>
> Because of the cache alignment, there is enough padding in the struct
> (no matter the size of the cache line) for adding two more pointers:
>
> struct rte_cryptodev {
> 	lot of fields...
> 	uint8_t attached : 1;
> 	struct rte_cryptodev_cb_rcu *enq_cbs;
> 	struct rte_cryptodev_cb_rcu *deq_cbs;
> } __rte_cache_aligned;
>
> We checked manually that the ABI is still compatible.

Right.

I am curious, but normally, libabigail should raise the addition of
structures, but then it'll tell you that there was no size or offset
change between the two structures.  If it doesn't, then that's a bug.  I
hope it does :-)


> Then I've added (quickly) a libabigail exception rule:
>
> [suppress_type]
> 	name = rte_cryptodev
> 	has_data_member_inserted_between = {0, 1023}
>
> Now we want to improve this rule to restrict the offsets
> to the padding at the end of the struct only,
> so we keep forbidding changes in existing fields,
> and forbidding additions further the current struct size.
> Is this new rule good?
>
> 	has_data_member_inserted_between = {offset_after(attached), end}


Yes, this rule should do what you think it says.

> Do you confirm that the keyword "end" means the old reference size?

Yes I do.


> What else do we need to check for adding a new field in a padding?

Actually, that rule will work independantly of it there is enough
padding or not.  It'll shut down the change report, even if the added
data exceeds the padding.

You just made me think of an idea of a new feature there.

Maybe we'd need a new property for the [suppress_type] directive that
would suppress changes only if said changes don't modify the size of the
type or any offset of any member of the type?

Maybe something like:

    [suppress_type]
       ; lots of properties can go here.

       ; ...

       ; If the type has any size or offset change
       ; then this suppression directive will fail
       ; and the change report will be emitted
       has_no_size_or_offset_change

Would that be useful to you in this case,

Cheers,

-- 
		Dodji


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

* Re: [dpdk-dev] [PATCH v1] devtools: update abi ignore for cryptodev
  2021-01-21 15:15   ` Dodji Seketeli
@ 2021-01-21 15:58     ` Thomas Monjalon
  2021-01-22 12:11       ` Kinsella, Ray
  2021-01-22 13:09       ` Dodji Seketeli
  0 siblings, 2 replies; 9+ messages in thread
From: Thomas Monjalon @ 2021-01-21 15:58 UTC (permalink / raw)
  To: Dodji Seketeli
  Cc: Ray Kinsella, Neil Horman, Akhil Goyal, Konstantin Ananyev,
	Abhinandan Gujjar, dev, david.marchand

21/01/2021 16:15, Dodji Seketeli:
> Hello Thomas and others,
> 
> Thomas Monjalon <thomas@monjalon.net> writes:
> 
> > Question to an expert, Dodji,
> 
> Thanks for the kind words, but I am not an expert in anything, sadly.  I
> am just trying to keep learning about these things ;-)
> 
> > We have this structure:
> >
> > struct rte_cryptodev {
> > 	lot of fields...
> > 	uint8_t attached : 1;
> > } __rte_cache_aligned;
> >
> > Because of the cache alignment, there is enough padding in the struct
> > (no matter the size of the cache line) for adding two more pointers:
> >
> > struct rte_cryptodev {
> > 	lot of fields...
> > 	uint8_t attached : 1;
> > 	struct rte_cryptodev_cb_rcu *enq_cbs;
> > 	struct rte_cryptodev_cb_rcu *deq_cbs;
> > } __rte_cache_aligned;
> >
> > We checked manually that the ABI is still compatible.
> 
> Right.
> 
> I am curious, but normally, libabigail should raise the addition of
> structures, but then it'll tell you that there was no size or offset
> change between the two structures.  If it doesn't, then that's a bug.  I
> hope it does :-)

Yes it was raising a problem, that's why we are adding a rule.


> > Then I've added (quickly) a libabigail exception rule:
> >
> > [suppress_type]
> > 	name = rte_cryptodev
> > 	has_data_member_inserted_between = {0, 1023}
> >
> > Now we want to improve this rule to restrict the offsets
> > to the padding at the end of the struct only,
> > so we keep forbidding changes in existing fields,
> > and forbidding additions further the current struct size.
> > Is this new rule good?
> >
> > 	has_data_member_inserted_between = {offset_after(attached), end}
> 
> 
> Yes, this rule should do what you think it says.
> 
> > Do you confirm that the keyword "end" means the old reference size?
> 
> Yes I do.
> 
> 
> > What else do we need to check for adding a new field in a padding?
> 
> Actually, that rule will work independantly of it there is enough
> padding or not.  It'll shut down the change report, even if the added
> data exceeds the padding.

I don't understand why.
If "end" means the old reference size, then addition after the old size
should be reported, isn't it?


> You just made me think of an idea of a new feature there.
> 
> Maybe we'd need a new property for the [suppress_type] directive that
> would suppress changes only if said changes don't modify the size of the
> type or any offset of any member of the type?
> 
> Maybe something like:
> 
>     [suppress_type]
>        ; lots of properties can go here.
> 
>        ; ...
> 
>        ; If the type has any size or offset change
>        ; then this suppression directive will fail
>        ; and the change report will be emitted
>        has_no_size_or_offset_change
> 
> Would that be useful to you in this case,
> 
> Cheers,




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

* Re: [dpdk-dev] [PATCH v1] devtools: update abi ignore for cryptodev
  2021-01-21 15:58     ` Thomas Monjalon
@ 2021-01-22 12:11       ` Kinsella, Ray
  2021-01-22 13:09       ` Dodji Seketeli
  1 sibling, 0 replies; 9+ messages in thread
From: Kinsella, Ray @ 2021-01-22 12:11 UTC (permalink / raw)
  To: Thomas Monjalon, Dodji Seketeli
  Cc: Neil Horman, Akhil Goyal, Konstantin Ananyev, Abhinandan Gujjar,
	dev, david.marchand



On 21/01/2021 15:58, Thomas Monjalon wrote:
> 21/01/2021 16:15, Dodji Seketeli:
>> Hello Thomas and others,
>>
>> Thomas Monjalon <thomas@monjalon.net> writes:
>>
>>> Question to an expert, Dodji,
>>
>> Thanks for the kind words, but I am not an expert in anything, sadly.  I
>> am just trying to keep learning about these things ;-)
>>
>>> We have this structure:
>>>
>>> struct rte_cryptodev {
>>> 	lot of fields...
>>> 	uint8_t attached : 1;
>>> } __rte_cache_aligned;
>>>
>>> Because of the cache alignment, there is enough padding in the struct
>>> (no matter the size of the cache line) for adding two more pointers:
>>>
>>> struct rte_cryptodev {
>>> 	lot of fields...
>>> 	uint8_t attached : 1;
>>> 	struct rte_cryptodev_cb_rcu *enq_cbs;
>>> 	struct rte_cryptodev_cb_rcu *deq_cbs;
>>> } __rte_cache_aligned;
>>>
>>> We checked manually that the ABI is still compatible.
>>
>> Right.
>>
>> I am curious, but normally, libabigail should raise the addition of
>> structures, but then it'll tell you that there was no size or offset
>> change between the two structures.  If it doesn't, then that's a bug.  I
>> hope it does :-)
> 
> Yes it was raising a problem, that's why we are adding a rule.
> 
> 
>>> Then I've added (quickly) a libabigail exception rule:
>>>
>>> [suppress_type]
>>> 	name = rte_cryptodev
>>> 	has_data_member_inserted_between = {0, 1023}
>>>
>>> Now we want to improve this rule to restrict the offsets
>>> to the padding at the end of the struct only,
>>> so we keep forbidding changes in existing fields,
>>> and forbidding additions further the current struct size.
>>> Is this new rule good?
>>>
>>> 	has_data_member_inserted_between = {offset_after(attached), end}
>>
>>
>> Yes, this rule should do what you think it says.
>>
>>> Do you confirm that the keyword "end" means the old reference size?
>>
>> Yes I do.
>>
>>
>>> What else do we need to check for adding a new field in a padding?
>>
>> Actually, that rule will work independantly of it there is enough
>> padding or not.  It'll shut down the change report, even if the added
>> data exceeds the padding.
> 
> I don't understand why.
> If "end" means the old reference size, then addition after the old size
> should be reported, isn't it?

yes - this comment confuses me also.

If "end" refers to the size original data-structure (position of the end), 
which in this case had some padding. If the additions fall fully within the 
padding I would expect this rule to work - as long as the data-structure size
is still the same. 

However if the additions fall beyond the size of the original data-structure,
the data-structure's size will have changed, I would not expect this rule to 
condone a change in the size of the data-structure.  

> 
> 
>> You just made me think of an idea of a new feature there.
>>
>> Maybe we'd need a new property for the [suppress_type] directive that
>> would suppress changes only if said changes don't modify the size of the
>> type or any offset of any member of the type?
>>
>> Maybe something like:
>>
>>     [suppress_type]
>>        ; lots of properties can go here.
>>
>>        ; ...
>>
>>        ; If the type has any size or offset change
>>        ; then this suppression directive will fail
>>        ; and the change report will be emitted
>>        has_no_size_or_offset_change
>>
>> Would that be useful to you in this case,
>>
>> Cheers,
> 
> 
> 

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

* Re: [dpdk-dev] [PATCH v1] devtools: update abi ignore for cryptodev
  2021-01-21 15:58     ` Thomas Monjalon
  2021-01-22 12:11       ` Kinsella, Ray
@ 2021-01-22 13:09       ` Dodji Seketeli
  2021-01-22 13:12         ` Kinsella, Ray
  1 sibling, 1 reply; 9+ messages in thread
From: Dodji Seketeli @ 2021-01-22 13:09 UTC (permalink / raw)
  To: Thomas Monjalon
  Cc: Ray Kinsella, Neil Horman, Akhil Goyal, Konstantin Ananyev,
	Abhinandan Gujjar, dev, david.marchand

Thomas Monjalon <thomas@monjalon.net> writes:

[...]

>> > Then I've added (quickly) a libabigail exception rule:
>> >
>> > [suppress_type]
>> > 	name = rte_cryptodev
>> > 	has_data_member_inserted_between = {0, 1023}
>> >
>> > Now we want to improve this rule to restrict the offsets
>> > to the padding at the end of the struct only,
>> > so we keep forbidding changes in existing fields,
>> > and forbidding additions further the current struct size.
>> > Is this new rule good?
>> >
>> > 	has_data_member_inserted_between = {offset_after(attached), end}
>> 
>> 
>> Yes, this rule should do what you think it says.
>> 
>> > Do you confirm that the keyword "end" means the old reference size?
>> 
>> Yes I do.
>> 
>> 
>> > What else do we need to check for adding a new field in a padding?
>> 
>> Actually, that rule will work independantly of it there is enough
>> padding or not.  It'll shut down the change report, even if the added
>> data exceeds the padding.
>
> I don't understand why.
> If "end" means the old reference size, then addition after the old size
> should be reported, isn't it?

Yes, you are right.

What I meant is that even if (in an hypothetical case, not yours) the
padding was so "small" that it wasn't going up to the 'end' of the
struct, that rule would have still shut down the change report.

[...]

Cheers,

-- 
		Dodji


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

* Re: [dpdk-dev] [PATCH v1] devtools: update abi ignore for cryptodev
  2021-01-22 13:09       ` Dodji Seketeli
@ 2021-01-22 13:12         ` Kinsella, Ray
  2021-01-24 11:58           ` Dodji Seketeli
  0 siblings, 1 reply; 9+ messages in thread
From: Kinsella, Ray @ 2021-01-22 13:12 UTC (permalink / raw)
  To: Dodji Seketeli, Thomas Monjalon
  Cc: Neil Horman, Akhil Goyal, Konstantin Ananyev, Abhinandan Gujjar,
	dev, david.marchand



On 22/01/2021 13:09, Dodji Seketeli wrote:
> Thomas Monjalon <thomas@monjalon.net> writes:
> 
> [...]
> 
>>>> Then I've added (quickly) a libabigail exception rule:
>>>>
>>>> [suppress_type]
>>>> 	name = rte_cryptodev
>>>> 	has_data_member_inserted_between = {0, 1023}
>>>>
>>>> Now we want to improve this rule to restrict the offsets
>>>> to the padding at the end of the struct only,
>>>> so we keep forbidding changes in existing fields,
>>>> and forbidding additions further the current struct size.
>>>> Is this new rule good?
>>>>
>>>> 	has_data_member_inserted_between = {offset_after(attached), end}
>>>
>>>
>>> Yes, this rule should do what you think it says.
>>>
>>>> Do you confirm that the keyword "end" means the old reference size?
>>>
>>> Yes I do.
>>>
>>>
>>>> What else do we need to check for adding a new field in a padding?
>>>
>>> Actually, that rule will work independantly of it there is enough
>>> padding or not.  It'll shut down the change report, even if the added
>>> data exceeds the padding.
>>
>> I don't understand why.
>> If "end" means the old reference size, then addition after the old size
>> should be reported, isn't it?
> 
> Yes, you are right.
> 
> What I meant is that even if (in an hypothetical case, not yours) the
> padding was so "small" that it wasn't going up to the 'end' of the
> struct, that rule would have still shut down the change report.

Understood - you are talking about padding between members. 

> 
> [...]
> 
> Cheers,
> 

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

* Re: [dpdk-dev] [PATCH v1] devtools: update abi ignore for cryptodev
  2021-01-22 13:12         ` Kinsella, Ray
@ 2021-01-24 11:58           ` Dodji Seketeli
  0 siblings, 0 replies; 9+ messages in thread
From: Dodji Seketeli @ 2021-01-24 11:58 UTC (permalink / raw)
  To: Kinsella, Ray
  Cc: Dodji Seketeli, Thomas Monjalon, Neil Horman, Akhil Goyal,
	Konstantin Ananyev, Abhinandan Gujjar, dev, david.marchand

"Kinsella, Ray" <mdr@ashroe.eu> writes:

> On 22/01/2021 13:09, Dodji Seketeli wrote:
>> Thomas Monjalon <thomas@monjalon.net> writes:
>> 
>> [...]
>> 
>>>>> Then I've added (quickly) a libabigail exception rule:
>>>>>
>>>>> [suppress_type]
>>>>> 	name = rte_cryptodev
>>>>> 	has_data_member_inserted_between = {0, 1023}
>>>>>
>>>>> Now we want to improve this rule to restrict the offsets
>>>>> to the padding at the end of the struct only,
>>>>> so we keep forbidding changes in existing fields,
>>>>> and forbidding additions further the current struct size.
>>>>> Is this new rule good?
>>>>>
>>>>> 	has_data_member_inserted_between = {offset_after(attached), end}
>>>>
>>>>
>>>> Yes, this rule should do what you think it says.
>>>>
>>>>> Do you confirm that the keyword "end" means the old reference size?
>>>>
>>>> Yes I do.
>>>>
>>>>
>>>>> What else do we need to check for adding a new field in a padding?
>>>>
>>>> Actually, that rule will work independantly of it there is enough
>>>> padding or not.  It'll shut down the change report, even if the added
>>>> data exceeds the padding.
>>>
>>> I don't understand why.
>>> If "end" means the old reference size, then addition after the old size
>>> should be reported, isn't it?
>> 
>> Yes, you are right.
>> 
>> What I meant is that even if (in an hypothetical case, not yours) the
>> padding was so "small" that it wasn't going up to the 'end' of the
>> struct, that rule would have still shut down the change report.
>
> Understood - you are talking about padding between members.

Exactly.

Cheers,

-- 
		Dodji


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

* Re: [dpdk-dev] [PATCH v1] devtools: update abi ignore for cryptodev
  2021-01-20 14:25 [dpdk-dev] [PATCH v1] devtools: update abi ignore for cryptodev Ray Kinsella
  2021-01-20 15:41 ` Thomas Monjalon
@ 2021-01-26 11:55 ` Thomas Monjalon
  1 sibling, 0 replies; 9+ messages in thread
From: Thomas Monjalon @ 2021-01-26 11:55 UTC (permalink / raw)
  To: Ray Kinsella
  Cc: Neil Horman, Akhil Goyal, Konstantin Ananyev, Abhinandan Gujjar,
	dev, david.marchand

20/01/2021 15:25, Ray Kinsella:
> Update the ignore entry for crytodev to use named fields instead of
> bit positions.
> 
> Fixes: 1c3ffb9559
> 
> Signed-off-by: Ray Kinsella <mdr@ashroe.eu>
> ---
> --- a/devtools/libabigail.abignore
> +++ b/devtools/libabigail.abignore
> @@ -15,4 +15,4 @@
>  ; Ignore fields inserted in cacheline boundary of rte_cryptodev
>  [suppress_type]
>          name = rte_cryptodev
> -        has_data_member_inserted_between = {0, 1023}
> +        has_data_member_inserted_between = {offset_after(attached), end}

Adding a bit more explanations in the commit message:

It is allowing changes between the last field (attached) in ABI 21.0,
and the end of the padded struct in ABI 21.

Fixes: 1c3ffb95595e ("cryptodev: add enqueue and dequeue callbacks")

Acked-by: Thomas Monjalon <thomas@monjalon.net>

Applied, thanks.



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

end of thread, other threads:[~2021-01-26 11:55 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-20 14:25 [dpdk-dev] [PATCH v1] devtools: update abi ignore for cryptodev Ray Kinsella
2021-01-20 15:41 ` Thomas Monjalon
2021-01-21 15:15   ` Dodji Seketeli
2021-01-21 15:58     ` Thomas Monjalon
2021-01-22 12:11       ` Kinsella, Ray
2021-01-22 13:09       ` Dodji Seketeli
2021-01-22 13:12         ` Kinsella, Ray
2021-01-24 11:58           ` Dodji Seketeli
2021-01-26 11:55 ` Thomas Monjalon

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).