[dpdk-dev] [PATCH v1] devtools: update abi ignore for cryptodev

[dpdk-dev] [PATCH v1] devtools: update abi ignore for cryptodev

[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}
\ No newline at end of file
-- 
2.26.2

Re: [dpdk-dev] [PATCH v1] devtools: update abi ignore for cryptodev

[Thomas Monjalon]

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}

Re: [dpdk-dev] [PATCH v1] devtools: update abi ignore for cryptodev

[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 :-)


> 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

Re: [dpdk-dev] [PATCH v1] devtools: update abi ignore for cryptodev

[Thomas Monjalon]

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,

Re: [dpdk-dev] [PATCH v1] devtools: update abi ignore for cryptodev

[Kinsella, Ray]

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,
> 
> 
> 

Re: [dpdk-dev] [PATCH v1] devtools: update abi ignore for cryptodev

[Dodji Seketeli]

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

Re: [dpdk-dev] [PATCH v1] devtools: update abi ignore for cryptodev

[Kinsella, Ray]

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,
> 

Re: [dpdk-dev] [PATCH v1] devtools: update abi ignore for cryptodev

[Dodji Seketeli]

"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

Re: [dpdk-dev] [PATCH v1] devtools: update abi ignore for cryptodev

[Thomas Monjalon]

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.