Message ID | 1468939061-19734-2-git-send-email-zoltan.kiss@schaman.hu (mailing list archive) |
---|---|
State | Superseded, archived |
Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id F30B6558D; Tue, 19 Jul 2016 16:38:33 +0200 (CEST) Received: from mail-lf0-f67.google.com (mail-lf0-f67.google.com [209.85.215.67]) by dpdk.org (Postfix) with ESMTP id 7F33D5586 for <dev@dpdk.org>; Tue, 19 Jul 2016 16:38:31 +0200 (CEST) Received: by mail-lf0-f67.google.com with SMTP id 33so1479318lfw.3 for <dev@dpdk.org>; Tue, 19 Jul 2016 07:38:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=schaman-hu.20150623.gappssmtp.com; s=20150623; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=xQQ6ydCn/LSLjhO+3Uq4kzqp1pAPdn8JbaQxQWbOpnE=; b=idIIGGzCUe3uSOQK9SuxDoncXaI6j8JkaBw67P4Df2ojru/SPLqyUAgPFWwArFjvoF x0GUPrVgFGnL1iKTEru4/woM6+DnHq7T9i8kpSpoQ8cxHoBCyYAVEOTO3722jDDFd9Xk N/E5WxzNvFLCxFfbSzjOYa80+uGI61YMHUrXNP0IiAVdyZQyCIuTQe1KWDRjMVVbh1jd eAXjq+lXT0bcXOrXu/ClfWcZevKDSVBAsxOq4h5jYILc3w85KRUH8nZOlQYcQltDTmaZ wuvr9b23bdsLAHITU33FVgzUfWc7Rh6oqto/XV24wpN6awIht/bveo/ls5N0A6JVRgdq 1Nyw== 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:in-reply-to :references; bh=xQQ6ydCn/LSLjhO+3Uq4kzqp1pAPdn8JbaQxQWbOpnE=; b=Uzn6EuWmCU9PTplJhfjm/KoiQfvxcuL5FBzxJCenu6RrPfFo6Aos99dgSUSG42KqN1 tlQUat1wpyDUIDEJkEgawuXfIexL3c2vIHRZkzGCWg7vuP1qRiXQPWPcla+lghmeO0UC b/9KLJhg/Nmpz7MdcdkHUeAyRxyC7NY6ozr0EfqFJbKRRCz0F19XYCRXwqRYSKbWJP1K P15eySN2QY4mHRt+jlVH/M0PsrvwAgID5CbHgry04RHCtqWyEqmIc5+xDdGbWd3vkztF waCjjhsOhGYukZoSbudcKvUP6PAWT54ghr9Tmc2heK8PaDWegulz4IZuVif0Ww2srGVS Jvcw== X-Gm-Message-State: ALyK8tIKjWy6UxQ/GoJY07aQdax6AEtQUajQ51NlwLTZh5e0JXG+1SKp3xhvHugNw8aItA== X-Received: by 10.25.144.75 with SMTP id s72mr18035272lfd.232.1468939111000; Tue, 19 Jul 2016 07:38:31 -0700 (PDT) Received: from localhost.localdomain ([195.11.233.227]) by smtp.googlemail.com with ESMTPSA id h191sm5758935lfh.0.2016.07.19.07.38.30 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Tue, 19 Jul 2016 07:38:30 -0700 (PDT) From: Zoltan Kiss <zoltan.kiss@schaman.hu> To: dev@dpdk.org Cc: olivier.matz@6wind.com Date: Tue, 19 Jul 2016 15:37:41 +0100 Message-Id: <1468939061-19734-2-git-send-email-zoltan.kiss@schaman.hu> X-Mailer: git-send-email 1.9.1 In-Reply-To: <1468939061-19734-1-git-send-email-zoltan.kiss@schaman.hu> References: <1468939061-19734-1-git-send-email-zoltan.kiss@schaman.hu> Subject: [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 <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Zoltan Kiss
July 19, 2016, 2:37 p.m. UTC
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 <= RTE_MEMZONE_NAMESIZE - strlen(RTE_RING_MZ_PREFIX)
RTE_MEMPOOL_NAMESIZE <= 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 <zoltan.kiss@schaman.hu>
---
lib/librte_eal/common/include/rte_memzone.h | 2 +-
lib/librte_mempool/rte_mempool.h | 4 +++-
lib/librte_ring/rte_ring.h | 5 ++++-
3 files changed, 8 insertions(+), 3 deletions(-)
Comments
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 <= RTE_MEMZONE_NAMESIZE - strlen(RTE_RING_MZ_PREFIX) > RTE_MEMPOOL_NAMESIZE <= 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 <zoltan.kiss@schaman.hu> 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. 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? In any case, I think it's a bit late for 16.07 for this kind of fix. Regards, Olivier
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 <= RTE_MEMZONE_NAMESIZE - strlen(RTE_RING_MZ_PREFIX) >> RTE_MEMPOOL_NAMESIZE <= 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 <zoltan.kiss@schaman.hu> > > 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. 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. > > 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. > > In any case, I think it's a bit late for 16.07 for this kind of fix. > > Regards, > Olivier >
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 <= RTE_MEMZONE_NAMESIZE - strlen(RTE_RING_MZ_PREFIX) >>> RTE_MEMPOOL_NAMESIZE <= 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 <zoltan.kiss@schaman.hu> >> >> 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. 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 \ (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. >> 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? Thanks, Olivier
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 <= RTE_MEMZONE_NAMESIZE - strlen(RTE_RING_MZ_PREFIX) >>>> RTE_MEMPOOL_NAMESIZE <= 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 <zoltan.kiss@schaman.hu> >>> >>> 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. > > 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 > (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. 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 <= 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 > >>> 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. > > > Thanks, > Olivier >
Hi, 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 <= RTE_MEMZONE_NAMESIZE - strlen(RTE_RING_MZ_PREFIX) >>>>> RTE_MEMPOOL_NAMESIZE <= 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 <zoltan.kiss@schaman.hu> >>>> >>>> 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. 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 there is no way for an application to know what is the maximum usable size for a mempool or a ring. >> 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 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. The patch you're proposing also changes the ABI of librte_ring and librte_eal, which cannot be accepted for the release. > >> (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. 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 if 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. > 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 <= 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 The name uniqueness is checked by rte_memzone_reserve(). >>>> 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. I think have a maximum name length brings more problems than not having it, especially ABI problems. Regards, Olivier
> -----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@linaro.org>; Zoltan Kiss > <zoltan.kiss@schaman.hu>; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] mempool: adjust name string size in > related data types > > Hi, > > 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 <= RTE_MEMZONE_NAMESIZE - > >>>>> strlen(RTE_RING_MZ_PREFIX) RTE_MEMPOOL_NAMESIZE <= > >>>>> 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 <zoltan.kiss@schaman.hu> > >>>> > >>>> 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. > > 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 there > is no way for an application to know what is the maximum usable size for a > mempool or a ring. > > > >> 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 > > 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. > > The patch you're proposing also changes the ABI of librte_ring and > librte_eal, which cannot be accepted for the release. > > > > > >> (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. > > 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 if > 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. > > > > 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 <= 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 > > The name uniqueness is checked by rte_memzone_reserve(). > > > >>>> 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. > > I think have a maximum name length brings more problems than not having > it, especially ABI problems. > I disagree. I think we should have reasonable max names, and allow functions to return an error in case of a name being too long. However, what I think 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 that 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 max as we need to, while giving others reasonable guarantees too. /Bruce
On 20/07/16 14:37, Olivier Matz wrote: > Hi, > > 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 <= RTE_MEMZONE_NAMESIZE - strlen(RTE_RING_MZ_PREFIX) >>>>>> RTE_MEMPOOL_NAMESIZE <= 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 <zoltan.kiss@schaman.hu> >>>>> >>>>> 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. > > 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 > there is no way for an application to know what is the maximum usable > size for a mempool or a ring. > > >>> 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 > > 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. > > The patch you're proposing also changes the ABI of librte_ring and > librte_eal, which cannot be accepted for the release. Ok, I've sent a new version with this approach. > > >> >>> (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. > > 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 > if 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. If someone adds a new handler, (s)he needs to keep in mind what's the max size for pool name, and any derived object using that name as a base should check if it fits. > > >> 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 <= 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 > > The name uniqueness is checked by rte_memzone_reserve(). > > >>>>> 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. > > I think have a maximum name length brings more problems than not having > it, especially ABI problems. > > > Regards, > Olivier >
diff --git a/lib/librte_eal/common/include/rte_memzone.h b/lib/librte_eal/common/include/rte_memzone.h index f69b5a8..ba3a1f0 100644 --- a/lib/librte_eal/common/include/rte_memzone.h +++ b/lib/librte_eal/common/include/rte_memzone.h @@ -74,7 +74,7 @@ extern "C" { */ struct rte_memzone { -#define RTE_MEMZONE_NAMESIZE 32 /**< Maximum length of memory zone name.*/ +#define RTE_MEMZONE_NAMESIZE (32 + 6) /**< Maximum length of memory zone name.*/ char name[RTE_MEMZONE_NAMESIZE]; /**< Name of the memory zone. */ phys_addr_t phys_addr; /**< Start physical address. */ diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h index 4a8fbb1..61e8d19 100644 --- a/lib/librte_mempool/rte_mempool.h +++ b/lib/librte_mempool/rte_mempool.h @@ -123,7 +123,9 @@ struct rte_mempool_objsz { /**< Total size of an object (header + elt + trailer). */ }; -#define RTE_MEMPOOL_NAMESIZE 32 /**< Maximum length of a memory pool. */ +/**< Maximum length of a memory pool's name. */ +#define RTE_MEMPOOL_NAMESIZE (RTE_RING_NAMESIZE - \ + sizeof(RTE_MEMPOOL_MZ_PREFIX) + 1) #define RTE_MEMPOOL_MZ_PREFIX "MP_" /* "MP_<name>" */ diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h index eb45e41..d6185de 100644 --- a/lib/librte_ring/rte_ring.h +++ b/lib/librte_ring/rte_ring.h @@ -100,6 +100,7 @@ extern "C" { #include <rte_lcore.h> #include <rte_atomic.h> #include <rte_branch_prediction.h> +#include <rte_memzone.h> #define RTE_TAILQ_RING_NAME "RTE_RING" @@ -126,8 +127,10 @@ struct rte_ring_debug_stats { } __rte_cache_aligned; #endif -#define RTE_RING_NAMESIZE 32 /**< The maximum length of a ring name. */ #define RTE_RING_MZ_PREFIX "RG_" +/**< The maximum length of a ring name. */ +#define RTE_RING_NAMESIZE (RTE_MEMZONE_NAMESIZE - \ + sizeof(RTE_RING_MZ_PREFIX) + 1) #ifndef RTE_RING_PAUSE_REP_COUNT #define RTE_RING_PAUSE_REP_COUNT 0 /**< Yield after pause num of times, no yield