Re: [dpdk-dev] [PATCH v8 2/3] build: use Python pmdinfogen

From: "Kinsella, Ray" <mdr@ashroe.eu>
To: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
Cc: Thomas Monjalon <thomas@monjalon.net>,
	dev@dpdk.org, Stephen Hemminger <stephen@networkplumber.org>,
	David Marchand <david.marchand@redhat.com>,
	Maxime Coquelin <maxime.coquelin@redhat.com>,
	Aaron Conole <aconole@redhat.com>,
	Bruce Richardson <bruce.richardson@intel.com>,
	ferruh.yigit@intel.com, ray.kinsella@intel.com
Subject: Re: [dpdk-dev] [PATCH v8 2/3] build: use Python pmdinfogen
Date: Mon, 25 Jan 2021 10:11:07 +0000	[thread overview]
Message-ID: <c814573b-1759-138d-32e8-640769e988d0@ashroe.eu> (raw)
In-Reply-To: <20210125130517.470ae277@sovereign>

On 25/01/2021 10:05, Dmitry Kozlyuk wrote:
> On Mon, 25 Jan 2021 09:25:51 +0000, Kinsella, Ray wrote:
>> On 23/01/2021 11:38, Thomas Monjalon wrote:
>>> 22/01/2021 23:24, Dmitry Kozlyuk:  
>>>> On Fri, 22 Jan 2021 21:57:15 +0100, Thomas Monjalon wrote:  
>>>>> 22/01/2021 21:31, Dmitry Kozlyuk:  
>>>>>> On Wed, 20 Jan 2021 11:24:21 +0100, Thomas Monjalon wrote:    
>>>>>>> 20/01/2021 08:23, Dmitry Kozlyuk:    
>>>>>>>> On Wed, 20 Jan 2021 01:05:59 +0100, Thomas Monjalon wrote:      
>>>>>>>>> This is now the right timeframe to introduce this change
>>>>>>>>> with the new Python module dependency.
>>>>>>>>> Unfortunately, the ABI check is returning an issue:
>>>>>>>>>
>>>>>>>>> 'const char mlx5_common_pci_pmd_info[62]' was changed
>>>>>>>>> to 'const char mlx5_common_pci_pmd_info[60]' at rte_common_mlx5.pmd.c      
>>>>>>>>
>>>>>>>> Will investigate and fix ASAP.    
>>>>>>
>>>>>> Now that I think of it: strings like this change every time new PCI IDs are
>>>>>> added to a PMD, but AFAIK adding PCI IDs is not considered an ABI breakage,
>>>>>> is it? One example is 28c9a7d7b48e ("net/mlx5: add ConnectX-6 Lx device ID")
>>>>>> added 2020-07-08, i.e. clearly outside of ABI change window.    
>>>>>
>>>>> You're right.
>>>>>  
>>>>>> "xxx_pmd_info" changes are due to JSON formatting (new is more canonical),
>>>>>> which can be worked around easily, if the above is wrong.    
>>>>>
>>>>> If the new format is better, please keep it.
>>>>> What we need is an exception for the pmdinfo symbols
>>>>> in the file devtools/libabigail.abignore.
>>>>> You can probably use a regex for these symbols.  
>>>>
>>>> This would allow real breakages to pass ABI check, abidiff doesn't analyze
>>>> variable content and it's not easy to compare. Maybe later a script can be
>>>> added that checks lines with RTE_DEVICE_IN in patches. There are at most 32 of
>>>> 5494 relevant commits between 19.11 and 20.11, though.
>>>>
>>>> To verify there are no meaningful changes I ensured empty diff between
>>>> results of the following command for "main" and the branch:
>>>>
>>>> 	find build/drivers -name '*.so' -exec usertools/dpdk-pmdinfo.py  
>>>
>>> For now we cannot do such check as part of the ABI checker.
>>> And we cannot merge this patch if the ABI check fails.
>>> I think the only solution is to allow any change in the pmdinfo variables.
>>>   
>>
>> So my 2c on this is that this is an acceptable work-around for the v21 (DPDK v20.11) ABI.
>> However we are going to end up carrying this rule in libabigail.ignore indefinitely.
>>
>> Would it make sense to just fix the size of _pmd_info to some reasonably large value - 
>> say 128 bytes, to allow us to drop the rule in the DPDK 21.11 v22 release?
> 
> I don't think so. This is a JSON *string to be parsed;* considering its size
> as part of application *binary* interface is wrong in the first place.

Right - then is belongs in INTERNAL, I would say. 

> As for
> content, checking that no PCI IDs are removed is out of scope for libabigail
> anyway. 

Lets be clear PCI IDs - are _nothing_ to do with ABI.

> Technically we could fix _pmd_info size, but this still allows
> breaking changes to pass the check with no benefit.

ABI changes or other, please explain?