Re: 21.11.1 patches review and test

[Kevin Traynor <ktraynor@redhat.com>]

On 14/04/2022 06:52, Christian Ehrhardt wrote:
> On Wed, Apr 13, 2022 at 12:06 PM Kevin Traynor <ktraynor@redhat.com> wrote:
>>
>> Hi Christian/Thomas/Ori,
>>
>> On 11/04/2022 07:58, Christian Ehrhardt wrote:
>>> On Fri, Apr 1, 2022 at 12:22 PM Kevin Traynor<ktraynor@redhat.com>  wrote:
>>>> Hi all,
>>>>
>>>> Here is a list of patches targeted for stable release 21.11.1.
>>> Hi Kevin,
>>> this breaks on me at build time due to symbol changes.
>>> It is a wild mix of Base->Internal/Experimental, a few new symbols,
>>> and even just removed ones (in gpu which is experimental, but still
>>> would that need a minor soname bump?).
>>> They might be intentional, but it felt too much to me without at least
>>> discussing it.
>>> Could you have a look if you think that they are all intentional, save
>>> and correct for an LTS release?
>>>
>>
>> Thanks for the report. I've looked through these. Comments below.
>>
>>> dpkg-gensymbols: warning: some new symbols appeared in the symbols
>>> file: see diff output below
>>> dpkg-gensymbols: warning: debian/librte-common-cnxk22/DEBIAN/symbols
>>> doesn't match completely debian/librte-common-cnxk22.symbols
>>> --- debian/librte-common-cnxk22.symbols
>>> (librte-common-cnxk22_21.11.1~rc1-0ubuntu1~jammyppa2_amd64)
>>> +++ dpkg-gensymbolsUuRb8d 2022-04-11 06:46:22.276766813 +0000
>>> @@ -197,6 +197,7 @@
>>>     roc_nix_ptp_clock_read@INTERNAL 21.08
>>>     roc_nix_ptp_info_cb_register@INTERNAL 21.08
>>>     roc_nix_ptp_info_cb_unregister@INTERNAL 21.08
>>> + roc_nix_ptp_is_enable@INTERNAL 21.11.1~rc1-0ubuntu1~jammyppa2
>>>     roc_nix_ptp_rx_ena_dis@INTERNAL 21.08
>>>     roc_nix_ptp_sync_time_adjust@INTERNAL 21.08
>>>     roc_nix_ptp_tx_ena_dis@INTERNAL 21.08
>>>
>>
>> This is a new internal from:
>> commit 28acfe550dcb12b0908df754a4307b8b4d1fe5b0
>> Author: Harman Kalra <hkalra@marvell.com>
>> Date:   Thu Mar 3 12:30:42 2022 +0530
>>
>>       common/cnxk: fix mbuf data offset for VF
>>
>>       [ upstream commit 8f98e3ecc55e02234f8bec7213b0b6a69c086949 ]
>>
>> Looks ok to me.
>>
>>> dpkg-gensymbols: warning: some new symbols appeared in the symbols
>>> file: see diff output below
>>> dpkg-gensymbols: warning: debian/librte-ethdev22/DEBIAN/symbols
>>> doesn't match completely debian/librte-ethdev22.symbols
>>> --- debian/librte-ethdev22.symbols
>>> (librte-ethdev22_21.11.1~rc1-0ubuntu1~jammyppa2_amd64)
>>> +++ dpkg-gensymbolskEnokB 2022-04-11 06:46:25.252795157 +0000
>>> @@ -37,6 +37,7 @@
>>>     rte_eth_dev_flow_ctrl_get@DPDK_22 21.11
>>>     rte_eth_dev_flow_ctrl_set@DPDK_22 21.11
>>>     rte_eth_dev_fw_version_get@DPDK_22 21.11
>>> + rte_eth_dev_get_by_name@INTERNAL 21.11.1~rc1-0ubuntu1~jammyppa2
>>>     rte_eth_dev_get_dcb_info@DPDK_22 21.11
>>>     rte_eth_dev_get_eeprom@DPDK_22 21.11
>>>     rte_eth_dev_get_eeprom_length@DPDK_22 21.11
>>>
>>
>> This is a new internal
>> Added by:
>> commit 721d0bbd1668d2a4b617a4a4de0b93dd60283d58
>> Author: Kumara Parameshwaran <kparameshwar@vmware.com>
>> Date:   Mon Jan 31 20:02:33 2022 +0530
>>
>>       ethdev: add internal function to device struct from name
>>
>>       [ upstream commit 961fb4029b8c52c0e8230d34993c354d70e10e14 ]
>>
>> Used by:
>> commit ac180f4d2662503ecd18a2e94689a229104d3d61
>> Author: Kumara Parameshwaran <kparameshwar@vmware.com>
>> Date:   Mon Jan 31 20:02:34 2022 +0530
>>
>>       net/tap: fix to populate FDs in secondary process
>>
>>       [ upstream commit c36ce7099c2187926cd62cff7ebd479823554929 ]
>>
>> Looks ok to me.
>>
>>> dpkg-gensymbols: warning: some new symbols appeared in the symbols
>>> file: see diff output below
>>> dpkg-gensymbols: error: some symbols or patterns disappeared in the
>>> symbols file: see diff output below
>>> dpkg-gensymbols: warning: debian/librte-regexdev22/DEBIAN/symbols
>>> doesn't match completely debian/librte-regexdev22.symbols
>>> --- debian/librte-regexdev22.symbols
>>> (librte-regexdev22_21.11.1~rc1-0ubuntu1~jammyppa2_amd64)
>>> +++ dpkg-gensymbolsPD0Ygo 2022-04-11 06:46:33.368872490 +0000
>>> @@ -1,6 +1,8 @@
>>>    librte_regexdev.so.22 librte-regexdev22 #MINVER#
>>>     EXPERIMENTAL@EXPERIMENTAL 20.11
>>> - rte_regex_devices@Base 20.11
>>> + INTERNAL@INTERNAL 21.11.1~rc1-0ubuntu1~jammyppa2
>>> + rte_regex_devices@EXPERIMENTAL 21.11.1~rc1-0ubuntu1~jammyppa2
>>>     rte_regexdev_attr_get@EXPERIMENTAL 20.11
>>>     rte_regexdev_attr_set@EXPERIMENTAL 20.11
>>>     rte_regexdev_close@EXPERIMENTAL 20.11
>>> @@ -8,12 +10,16 @@
>>>     rte_regexdev_count@EXPERIMENTAL 20.11
>>>     rte_regexdev_dump@EXPERIMENTAL 20.11
>>>     rte_regexdev_get_dev_id@EXPERIMENTAL 20.11
>>> - rte_regexdev_get_device_by_name@Base 20.11
>>> + rte_regexdev_get_device_by_name@INTERNAL 21.11.1~rc1-0ubuntu1~jammyppa2
>>>     rte_regexdev_info_get@EXPERIMENTAL 20.11
>>> - rte_regexdev_is_valid_dev@Base 20.11
>>> - rte_regexdev_logtype@Base 20.11
>>> + rte_regexdev_is_valid_dev@EXPERIMENTAL 21.11.1~rc1-0ubuntu1~jammyppa2
>>> + rte_regexdev_logtype@EXPERIMENTAL 21.11.1~rc1-0ubuntu1~jammyppa2
>>>     rte_regexdev_queue_pair_setup@EXPERIMENTAL 20.11
>>> - rte_regexdev_register@Base 20.11
>>> + rte_regexdev_register@INTERNAL 21.11.1~rc1-0ubuntu1~jammyppa2
>>>     rte_regexdev_rule_db_compile_activate@EXPERIMENTAL 20.11
>>>     rte_regexdev_rule_db_export@EXPERIMENTAL 20.11
>>>     rte_regexdev_rule_db_import@EXPERIMENTAL 20.11
>>> @@ -21,7 +27,8 @@
>>>     rte_regexdev_selftest@EXPERIMENTAL 20.11
>>>     rte_regexdev_start@EXPERIMENTAL 20.11
>>>     rte_regexdev_stop@EXPERIMENTAL 20.11
>>> - rte_regexdev_unregister@Base 20.11
>>> + rte_regexdev_unregister@INTERNAL 21.11.1~rc1-0ubuntu1~jammyppa2
>>>     rte_regexdev_xstats_by_name_get@EXPERIMENTAL 20.11
>>>     rte_regexdev_xstats_get@EXPERIMENTAL 20.11
>>>     rte_regexdev_xstats_names_get@EXPERIMENTAL 20.11
>>>
>>
>> +cc Ori, regex maintainer.
>>
>> Regex library is an experimental API so everything should have been
>> experimental or internal. This is fixing that issue.
>>
>> It is fixed with
>> commit 6e7f8939f23c2c8ed80602bc0d62990eebe52013
>> Author: Thomas Monjalon <thomas@monjalon.net>
>> Date:   Sun Mar 6 10:20:22 2022 +0100
>>
>>       regexdev: fix section attribute of symbols
>>
>>       [ upstream commit 89e290eb8ca99af9f7cfc3292d93860f8e672708 ]
>>
>> and
>>
>>     commit 026470bafaa02cba0d46ed7b7e835262399a009a
>> Author: Thomas Monjalon <thomas@monjalon.net>
>> Date:   Sun Mar 6 10:20:23 2022 +0100
>>
>>       build: hide local symbols in shared libraries
>>
>>       [ upstream commit b403498e14229ee903c8fff9baefcb72894062f3 ]
>>
>>
>> The fact that they are redesignated to correctly be
>> experimental/internal seems ok to me.
>>
>>> dpkg-gensymbols: error: some symbols or patterns disappeared in the
>>> symbols file: see diff output below
>>> dpkg-gensymbols: warning: debian/librte-gpudev22/DEBIAN/symbols
>>> doesn't match completely debian/librte-gpudev22.symbols
>>> --- debian/librte-gpudev22.symbols
>>> (librte-gpudev22_21.11.1~rc1-0ubuntu1~jammyppa2_amd64)
>>> +++ dpkg-gensymbols4qkXdt 2022-04-11 06:46:34.552883776 +0000
>>> @@ -1,7 +1,7 @@
>>>    librte_gpudev.so.22 librte-gpudev22 #MINVER#
>>>     EXPERIMENTAL@EXPERIMENTAL 21.11
>>>     INTERNAL@INTERNAL 21.11
>>> - gpu_logtype@Base 21.11
>>>     rte_gpu_add_child@EXPERIMENTAL 21.11
>>>     rte_gpu_allocate@INTERNAL 21.11
>>>     rte_gpu_attach@INTERNAL 21.11
>>>
>>
>> The missing wildcard meant this symbol escaped in 21.11.
>>
>> It is fixed by:
>> commit 026470bafaa02cba0d46ed7b7e835262399a009a
>> Author: Thomas Monjalon <thomas@monjalon.net>
>> Date:   Sun Mar 6 10:20:23 2022 +0100
>>
>>       build: hide local symbols in shared libraries
>>
>>       [ upstream commit b403498e14229ee903c8fff9baefcb72894062f3 ]
>>
>> In this case the symbol is not redesignated but removed, but it doesn't
>> look to have any use to a user, so I think it can be safe to remove.
> 
> I'm 100% with all others, thanks for having a look.
> On this one I can easily follow the argument of the fix for the newest release.
> But for stable we can never really know if there are users.
> In theory for anything that shipped in a Distribution someone might
> have coded and linked something against it - we would not know.
> The meant to be "stable" update will then break them the hard way.
> 
> In this case gladly the function wasn't anything that one would
> consider useful for use from outside, so I think it is ok.
> 
> But still I wanted to make the point that in general a symbol:
> 1. once released might be used and we can not never be sure if no one uses them

The same argument can also be made for 22.03/22.07 which is ABI 
compatible with 22.11. I accept there will be exception cases where it 
makes sense to change in main because it is enabling some future work, 
which are not similarly valid for stable. So the bar should be a bit 
higher in stable.

> 2. even being EXPERIMENTAL, touching them too much in stable updates
> means not-stable. Should we at least try to minimize the impact to
> stable releases?
> 

APIs marked experimental are not part of the ABI version, but I agree it 
is a good goal to minimize changes for these in stable where possible.

> For now I'm adapting my checkers and will continue testing ...
> 

Thanks for raising Christian.

(P.S. on PTO, so won't be able to continue discussion for now)

>> There are updates to the libabigail.ignore for regex and gpu_dev to
>> ignore ABI changes for these fixes.
>>
>> --
>>
>> I'm ok with changes above for 21.11.1, what do others think?
>>
>> Kevin.
>>
>>>
>>> Full log:
>>> https://launchpadlibrarian.net/596047842/buildlog_ubuntu-jammy-amd64.dpdk_21.11.1~rc1-0ubuntu1~jammyppa2_BUILDING.txt.gz
>>>
>>
> 
>