From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id 25E58902 for ; Wed, 20 Jul 2016 15:37:38 +0200 (CEST) Received: from alille-653-1-293-182.w90-1.abo.wanadoo.fr ([90.1.53.182] helo=[192.168.1.13]) by mail.droids-corp.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.84_2) (envelope-from ) id 1bPrjO-0006Je-28; Wed, 20 Jul 2016 15:40:13 +0200 To: Zoltan Kiss , 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> From: Olivier Matz Message-ID: <3f98372c-2bc7-e464-c14a-bce44417165f@6wind.com> Date: Wed, 20 Jul 2016 15:37:26 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Icedove/45.1.0 MIME-Version: 1.0 In-Reply-To: <578F7183.6030805@linaro.org> Content-Type: text/plain; charset=windows-1252 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 13:37:38 -0000 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. > >> (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