From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 0E89E378B for ; Wed, 20 Jul 2016 16:02:48 +0200 (CEST) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga101.jf.intel.com with ESMTP; 20 Jul 2016 07:02:14 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.28,394,1464678000"; d="scan'208";a="1025582876" Received: from irsmsx102.ger.corp.intel.com ([163.33.3.155]) by fmsmga002.fm.intel.com with ESMTP; 20 Jul 2016 07:02:02 -0700 Received: from irsmsx156.ger.corp.intel.com (10.108.20.68) by IRSMSX102.ger.corp.intel.com (163.33.3.155) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 20 Jul 2016 15:02:01 +0100 Received: from irsmsx103.ger.corp.intel.com ([169.254.3.204]) by IRSMSX156.ger.corp.intel.com ([10.108.20.68]) with mapi id 14.03.0248.002; Wed, 20 Jul 2016 15:02:01 +0100 From: "Richardson, Bruce" To: Olivier Matz , Zoltan Kiss , Zoltan Kiss , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH] mempool: adjust name string size in related data types Thread-Index: AQHR4da0w8kXQ2YUV0CGo3VR8TTxlaAf3akAgAFWBICAAA+WAIAAFufw Date: Wed, 20 Jul 2016 14:01:59 +0000 Message-ID: <59AF69C657FD0841A61C55336867B5B035AA83C3@IRSMSX103.ger.corp.intel.com> References: <1468939061-19734-1-git-send-email-zoltan.kiss@schaman.hu> <1468939061-19734-2-git-send-email-zoltan.kiss@schaman.hu> <600cac7d-b0aa-7919-93f2-7a396ef7b3ea@6wind.com> <578E4E60.80802@linaro.org> <85d13abf-b05e-67f0-16fd-3279cb4064be@6wind.com> <578F7183.6030805@linaro.org> <3f98372c-2bc7-e464-c14a-bce44417165f@6wind.com> In-Reply-To: <3f98372c-2bc7-e464-c14a-bce44417165f@6wind.com> Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYmRlYWZhYjUtZjQ0NC00ZmM3LWJlMzAtM2UzMGU5NjZhMTBhIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6IjRcL3NzbE5JV2FYclZBeFFkQ1A0YjFZUkRKVmFmXC8xeHFkVTZRcWd1eHR4RT0ifQ== x-ctpclassification: CTP_IC x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH] mempool: adjust name string size in related data types 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: Wed, 20 Jul 2016 14:02:49 -0000 > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz > Sent: Wednesday, July 20, 2016 2:37 PM > To: Zoltan Kiss ; Zoltan Kiss > ; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] mempool: adjust name string size in > related data types >=20 > Hi, >=20 > On 07/20/2016 02:41 PM, Zoltan Kiss wrote: > > > > > > On 19/07/16 17:17, Olivier Matz wrote: > >> Hi Zoltan, > >> > >> On 07/19/2016 05:59 PM, Zoltan Kiss wrote: > >>> > >>> > >>> On 19/07/16 16:37, Olivier Matz wrote: > >>>> Hi Zoltan, > >>>> > >>>> On 07/19/2016 04:37 PM, Zoltan Kiss wrote: > >>>>> A recent fix brought up an issue about the size of the 'name' > fields: > >>>>> > >>>>> 85cf0079 mem: avoid memzone/mempool/ring name truncation > >>>>> > >>>>> These relations should be observed: > >>>>> > >>>>> RTE_RING_NAMESIZE <=3D RTE_MEMZONE_NAMESIZE - > >>>>> strlen(RTE_RING_MZ_PREFIX) RTE_MEMPOOL_NAMESIZE <=3D > >>>>> RTE_RING_NAMESIZE - > >>>>> strlen(RTE_MEMPOOL_MZ_PREFIX) > >>>>> > >>>>> Setting all of them to 32 hides this restriction from the > application. > >>>>> This patch increases the memzone string size to accomodate for > >>>>> these prefixes, and the same happens with the ring name string. > >>>>> The ABI needs to be broken to fix this API issue, this way doesn't > >>>>> break applications previously not failing due to the truncating > >>>>> bug now fixed. > >>>>> > >>>>> Signed-off-by: Zoltan Kiss > >>>> > >>>> I agree it is a problem for an application because it cannot know > >>>> what is the maximum name length. On the other hand, breaking the > >>>> ABI for this looks a bit overkill. Maybe we could reduce > >>>> RTE_MEMPOOL_NAMESIZE and RTE_RING_NAMESIZE instead of increasing > >>>> RTE_MEMZONE_NAMESIZE? That way, we could keep the ABI as is. > >>> > >>> But that would break the ABI too, wouldn't it? Unless you keep the > >>> array the same size (32 bytes) by using RTE_MEMZONE_NAMESIZE. > >> > >> Yes, that was the idea. > >> > >>> And even then, the API breaks anyway. There are applications - I > >>> have at least some - which use all 32 bytes to store the name. > >>> Decrease that would cause headache to change the naming scheme, > >>> because it's a 30 character long id, and chopping the last few chars > >>> would cause name collisions and annoying bugs. > >> > >> Before my patch (85cf0079), long names were silently truncated when > >> mempool created its ring and/or memzones. Now, it returns an error. > > > > With 16.04 an application could operate as expected if the first 26 > > character were unique. Your patch revealed the problem that characters > > after these were left out of the name. Now applications fail where > > this never been a bug because their naming scheme guarantees the > > uniqueness on the first 26 chars (or makes it very unlikely) Where the > > first 26 is not unique, it failed earlier too, because at memzone > > creation it checks for duplicate names. >=20 > Yes, I understand that there is a behavior change for applications using > names larger than 26 between 16.04 and 16.07. I also understand that ther= e > is no way for an application to know what is the maximum usable size for = a > mempool or a ring. >=20 >=20 > >> I'm not getting why changing the struct to something like below would > >> break the API, since it would already return an error today. > >> > >> #define RTE_MEMPOOL_NAMESIZE \ > > > > Wait, this would mean applications need to recompile to use the > > smaller value. AFAIK that's an ABI break too, right? At the moment I > > don't see a way to fix this without breaking the ABI >=20 > With this modification, if you don't recompile the application, its > behavior will still be the same as today -> it will return ENAMETOOLONG. > If you recompile it, the application will be aware of the maximum length. > To me, it seems to be a acceptable compromise for this release. >=20 > The patch you're proposing also changes the ABI of librte_ring and > librte_eal, which cannot be accepted for the release. >=20 >=20 > > > >> (RTE_MEMZONE_NAMESIZE - sizeof(pool_prefix) - sizeof(ring > prefix)) > >> struct rte_mempool { > >> union { > >> char name[RTE_MEMPOOL_NAMESIZE]; > >> char pad[32]; > >> }; > >> ... > >> } > >> > >> Anyway, it may not be the proper solution since it supposes that a > >> mempool includes a ring based on a memzone, which is not always true > >> now with mempool handlers. > > > > Oh, as we dug deeper it gets better! > > Indeed, we don't necessarily have this ring + memzone pair underneath, > > but the user is not aware of that, and I think we should keep it that > > way. It should only care that the string passed shouldn't be bigger > > than a certain amount. >=20 > Yes. What I'm just saying here is that it's not a good solution to write > in the #define that "a mempool is based on a ring + a memzone", because i= f > some someone adds a new mempool handler replacing the ring, and also > creating a memzone prefixed by something larger than "rg_", we will have > to break the ABI again. >=20 >=20 > > Also, even though we don't necessarily have the ring, we still reserve > > memzone's in rte_mempool_populate_default(). And their name has a 3 > > letter prefix, and a "_%d" postfix, where the %d could be as much as > > RTE_MAX_MEMZONE in worst case (2560 by default) So actually: > > > > RTE_MEMPOOL_NAMESIZE <=3D RTE_MEMZONE_NAMESIZE - > > strlen(RTE_MEMPOOL_MZ_PREFIX) - strlen("_2560") > > > > > > As a side note, there is another bug around here: rte_ring_create() > > doesn't check for name duplications. However the user of the library > > can lookup based on the name with rte_ring_lookup(), and it will > > return the first ring with that name >=20 > The name uniqueness is checked by rte_memzone_reserve(). >=20 >=20 > >>>> It would even be better to get rid of this static char[] for the > >>>> structure names and replace it by an allocated const char *. I > >>>> didn't check it's feasible for memzones. What do you think? > >>> > >>> It would work too, but I don't think it would help a lot. We would > >>> still need max sizes for the names. Storing them somewhere else > >>> won't help us in this problem. > >> > >> Why should we have a maximum length for the names? > > > > What happens if an application loads DPDK and create a mempool with a > > name string 2 million characters long? Maybe nothing we should worry > > about, but in general I think unlimited length function parameters are > > problematic at the very least. The length should be passed at least > > (which also creates a max due to the size of the param). But I think > > it would be just easier to have these maximums set, observing the > > above constrains. >=20 > I think have a maximum name length brings more problems than not having > it, especially ABI problems. >=20 I disagree. I think we should have reasonable max names, and allow function= s to return an error in case of a name being too long. However, what I thin= k we also need to do is to guarantee a minimum maximum name length to allow= apps to ensure they never hit that name-too-long error. We can guarantee t= hat for ring/mempool etc, that the max allowed name will always be at least= 20 characters, for example. That gives plenty of scope for adjusting the m= ax as we need to, while giving others reasonable guarantees too. /Bruce