From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f43.google.com (mail-wm0-f43.google.com [74.125.82.43]) by dpdk.org (Postfix) with ESMTP id 1B3332BA2 for ; Wed, 20 Jul 2016 19:20:26 +0200 (CEST) Received: by mail-wm0-f43.google.com with SMTP id q128so64720360wma.1 for ; Wed, 20 Jul 2016 10:20:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=subject:to:references:from:message-id:date:user-agent:mime-version :in-reply-to:content-transfer-encoding; bh=uu9Ozh1xV/SuVHb5Hsb6Q2WvUEd8p61DCjskSFkxnSg=; b=P0teOMUEGA7zBMP34Vqt8EP/yWhRqg34QGZlcjGSenJSiE+ToTdrWRq3W63nSq8bAG RLW0Qi1J8HchFE0s+zgEpATpvrCscBy+opHHaAFlMhAPMYSYvRDca91OBExaUSHoPEbT e58FL+gK+gojOy+lRAbNQgA/0oHf6fQ02xitQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=uu9Ozh1xV/SuVHb5Hsb6Q2WvUEd8p61DCjskSFkxnSg=; b=ZH6vWqeIXALlRysNniB7pv7YOnrceToKz07M3pPv6fgb7YJR2aCw9VaL+4RBjfHgkB gsoCjpuF2WFlfDWsWCmiblDxyMhPDi4lHvCmwvulHTG9mFqSl2nFxgTSZxb5LfdxAZeh D86FGz9bQVCgfRxfX9thWOw/h8KCVQgw2fDlIaskG9d4Y5n9B3CxLh8uwE6MC8PGNz6T pi68sRFDm2O4upE8G7fp6giGSyKUgxoBMRHw3f+hT45Bt6G3ekxY31n2dTmjuxVn4IRB dXavlNievBkJ0kAs29naCjc6aNkDMKMLYn3hPht1zvCWmRPbV5dwLemGHXEudT81U5yh 2udg== X-Gm-Message-State: ALyK8tJ/WwRzwYZ/7GCrF3gUdXlwiE7N0TD/JRKEeljnasBP6BXs9oJqTE5QwZWE+xcOKRuZ X-Received: by 10.194.117.42 with SMTP id kb10mr2418754wjb.90.1469035225728; Wed, 20 Jul 2016 10:20:25 -0700 (PDT) Received: from [172.18.45.108] ([195.11.233.227]) by smtp.googlemail.com with ESMTPSA id e4sm2231198wjy.20.2016.07.20.10.20.24 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 20 Jul 2016 10:20:25 -0700 (PDT) To: Olivier Matz , Zoltan Kiss , dev@dpdk.org 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> From: Zoltan Kiss Message-ID: <364d0db4-da78-79ae-3b54-31ac61a3ce68@linaro.org> Date: Wed, 20 Jul 2016 18:20:24 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <3f98372c-2bc7-e464-c14a-bce44417165f@6wind.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit 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 17:20:26 -0000 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 >>>>> >>>>> 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 >