From mboxrd@z Thu Jan 1 00:00:00 1970 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) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind_com.20150623.gappssmtp.com; s=20150623; h=from:to:cc:subject:date:message-id:organization:user-agent :in-reply-to:references:mime-version:content-transfer-encoding :content-type; bh=40Pkr4nwczYHtZlvOav3OXEJ3m/4M2WeAUIz6Xu8n1A=; b=tJ2bym9c+y9/Y1JNHQVVhtJm39K1fPo7Kyv/lUSCo4AH+bPlJ+5k2GHdn7HIgXeeXI J2TmTIbAhzfInWDkht0Owr44qATHRVhZLi2r2QVAWtA6eA7pMdxkzarkbTwAGiX1K4EK T4X7uX9wnWECG6BKpFzRCKAc7L90jX7H4RoU9Ud6P0KG2SbmkYDUufyy1cvR51WFthHj 4qytbjPu8XwMzwND3KA/qsR45q+LPIDJbZhYNyY8+bZvgvcC5hLQz9fZRMi/qIrA/cpg nVeKfAcX3toET/eQclaCoR+kCRCQo1MRE3bHgnq3slKVZAKVaG0aLBXGlTWPhKJvTbBO nR7g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:organization :user-agent:in-reply-to:references:mime-version :content-transfer-encoding:content-type; bh=40Pkr4nwczYHtZlvOav3OXEJ3m/4M2WeAUIz6Xu8n1A=; b=M820gn1E+yft1plpxisb1w/bQTknDkv8u64pkHHBrcwoWRRWkdRaQjW8svUZY3pBb8 D/glrxIVFwCiEW4gKE0sCYinrMgXOmw+ksd3YMgDwMPMjzmtW648i9cBjN9Qi20Uipiz 2Rcrxci7BOBmu9JRp7ADmNEtBc5QmIl6oQfJUD2OR7QLXfEZqE+y3jTuEw7ICixOWKOm mFJ4Oqecobd9pIxJU9xeYx5IpR4AzRjdYCVAGSC2mi8e/x1ZMtqIv/pbw0BLtthzECmt wvsbT07PK3xoUQsmqWMIFCSxw8FBC8uE5WRbjpJKqQEBV251j5J+/aTujhBSioi+j//D gnBw== X-Gm-Message-State: ALoCoQnsrm5Cj1eyD2fq4P8BdhsmCydELsJIh4Lbvq2+j5JplO50oCxJEV8nuVRQW86sQI++SIqW X-Received: by 10.28.139.18 with SMTP id n18mr19522112wmd.63.1446543570545; Tue, 03 Nov 2015 01:39:30 -0800 (PST) Received: from xps13.localnet (guy78-3-82-239-227-177.fbx.proxad.net. [82.239.227.177]) by smtp.gmail.com with ESMTPSA id t20sm22385474wme.0.2015.11.03.01.39.29 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 03 Nov 2015 01:39:29 -0800 (PST) From: Thomas Monjalon To: Panu Matilainen Date: Tue, 03 Nov 2015 10:38:19 +0100 Message-ID: <1689449.OkGDT9EhWe@xps13> Organization: 6WIND User-Agent: KMail/4.14.10 (Linux/4.1.6-1-ARCH; KDE/4.14.11; x86_64; ; ) In-Reply-To: <563870C0.9090602@redhat.com> References: <1445268976-27491-1-git-send-email-ehkinzie@gmail.com> <20975652.eOFrqL9zPp@xps13> <563870C0.9090602@redhat.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Cc: dev@dpdk.org, Eric Kinzie Subject: Re: [dpdk-dev] [PATCH v6 3/4] bond mode 4: allow external state machine X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 03 Nov 2015 09:39:31 -0000 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