From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Monjalon Subject: Re: [PATCH v6 3/4] bond mode 4: allow external state machine Date: Tue, 03 Nov 2015 10:38:19 +0100 Message-ID: <1689449.OkGDT9EhWe@xps13> References: <1445268976-27491-1-git-send-email-ehkinzie@gmail.com> <20975652.eOFrqL9zPp@xps13> <563870C0.9090602@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7Bit Cc: dev@dpdk.org, Eric Kinzie To: Panu Matilainen Return-path: Received: from mail-wm0-f51.google.com (mail-wm0-f51.google.com [74.125.82.51]) by dpdk.org (Postfix) with ESMTP id BEEA98E6E for ; Tue, 3 Nov 2015 10:39:30 +0100 (CET) Received: by wmff134 with SMTP id f134so79186663wmf.1 for ; Tue, 03 Nov 2015 01:39:30 -0800 (PST) In-Reply-To: <563870C0.9090602@redhat.com> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 2015-11-03 10:30, Panu Matilainen: > On 11/03/2015 10:16 AM, Thomas Monjalon wrote: > > 2015-11-03 08:48, Panu Matilainen: > >> On 11/02/2015 06:42 PM, Eric Kinzie wrote: > >>> On Mon Nov 02 12:23:47 +0200 2015, Panu Matilainen wrote: > >>>> On 11/01/2015 08:17 PM, Thomas Monjalon wrote: > >>>>> 2015-10-19 08:36, Eric Kinzie: > >>>>>> Size of struct rte_eth_bond_8023ad_conf changed. Increment LIBABIVER > >>>>>> and version bond_mode_8023ad_setup and bond_mode_8023ad_conf_get > >>>>>> functions. > >>>>> [...] > >>>>>> +VERSION_SYMBOL(bond_mode_8023ad_setup, _v20, 2.0); > >>>>> [...] > >>>>>> +BIND_DEFAULT_SYMBOL(bond_mode_8023ad_setup, _v22, 2.2); > >>>>>> +MAP_STATIC_SYMBOL(void bond_mode_8023ad_setup(struct rte_eth_dev *dev, struct rte_eth_bond_8023ad_conf *conf), \ > >>>>>> + bond_mode_8023ad_setup_v22); > >>>>> > >>>>> I'm sorry it doesn't work well when trying to build a combined lib: > >>>>> > >>>>> ld: libdpdk.so: version node not found for symbol bond_mode_8023ad_setup@@DPDK_2.2 > >>>>> > >>>>> The symbols are OK in the .o file: > >>>>> 0000000000002340 g F .text 0000000000000171 bond_mode_8023ad_setup@@DPDK_2.2 > >>>>> 0000000000002260 g F .text 00000000000000da bond_mode_8023ad_setup@DPDK_2.0 > >>>>> 0000000000002260 g F .text 00000000000000da bond_mode_8023ad_setup_v20 > >>>>> 0000000000002340 g F .text 0000000000000171 bond_mode_8023ad_setup_v22 > >>>>> 0000000000000000 *UND* 0000000000000000 bond_mode_8023ad_setup > >>>>> > >>>>> I don't understand the problem and I am considering disabling versioning in > >>>>> combined library. > >>>>> > >>>>> Any idea? > >>>>> > >>>> > >>>> The .map additions look incorrect to me: > >>>> > >>>>> diff --git a/drivers/net/bonding/rte_eth_bond_version.map b/drivers/net/bonding/rte_eth_bond_version.map > >>>>> index 22bd920..7f78717 100644 > >>>>> --- a/drivers/net/bonding/rte_eth_bond_version.map > >>>>> +++ b/drivers/net/bonding/rte_eth_bond_version.map > >>>>> @@ -17,6 +17,9 @@ DPDK_2.0 { > >>>>> rte_eth_bond_slaves_get; > >>>>> rte_eth_bond_xmit_policy_get; > >>>>> rte_eth_bond_xmit_policy_set; > >>>>> + rte_eth_bond_8023ad_ext_collect; > >>>>> + rte_eth_bond_8023ad_ext_distrib; > >>>>> + rte_eth_bond_8023ad_ext_slowtx; > >>>> > >>>> These symbols didn't exist in DPDK 2.0 but are only being added > >>>> here. So why are they being added to the 2.0 section? > >>> > >>> Yes, I think these should probably be moved. > >>> > >>> > >>>>> > >>>>> local: *; > >>>>> }; > >>>>> @@ -27,3 +30,10 @@ DPDK_2.1 { > >>>>> rte_eth_bond_free; > >>>>> > >>>>> } DPDK_2.0; > >>>>> + > >>>>> +DPDK_2.2 { > >>>>> + local > >>>>> + > >>>>> + bond_mode_8023ad_conf_get; > >>>>> + bond_mode_8023ad_setup; > >>>>> +} DPDK_2.1; > >>>> > >>>> These are marked local, as in, "not exported" which doesn't seem > >>>> right. Also they're lacking the rte_eth_ prefix. AFAICS this is what > >>>> the symbol export map should look like here: > >>> > >>> > >>> These were not exported to begin with. But after versioning these > >>> functions, they are exported unless explicitly declared to be local here. > >>> > >> > >> And this does not ring any warning bells? :) > >> > >> Sorry, I was not looking at the patch as a whole. You're declaring these > >> symbols as exported with the versioning macros, eg > >> > >> BIND_DEFAULT_SYMBOL(bond_mode_8023ad_setup, _v22, 2.2); > >> > >> ...and then explicitly telling it to not export them by declaring local, > >> and then we wonder why it has trouble finding the symbols. > >> The versioning macros wont invent the librte_ prefix for you, you need > >> to rename the functions accordingly. > >> > >> But all this versioning gymnastics is moot anyway because you declare > >> the ABI incompatible: > >> > >> -LIBABIVER := 1 > >> +LIBABIVER := 2 > >> > >> This changes the library soname, so no binary compiled against the > >> previous version can possibly use it anymore. As in, by definition there > >> can be no callers of the _v20 variants after this ABI version bump. > > > > Panu, thanks for your careful review. > > We must remove the private functions from the export map.and not bump LIBABIVER. > > Actually the LIBABIVER bump seems appropriate since a public struct > (rte_eth_bond_8023ad_conf) changes size. Not sure. It is added at the end of a struct which is not embedded elsewhere. > As for the rest, I'm not quite sure what the patch is trying to achieve. > I initially assumed its trying to add a new incompatible version of > rte_bond_mode_8023ad_setup() and rte_eth_bond_8023ad_conf_get() which > could be a perfectly reasonable thing to do in some circumstances. As it > is, the patch actually adds versioning to an internal symbol which just > doesn't make sense at all. Symbol versioning is only meaningful for > public symbols. Yes