From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f48.google.com (mail-wm0-f48.google.com [74.125.82.48]) by dpdk.org (Postfix) with ESMTP id 909AC1DB1 for ; Wed, 20 Jul 2016 14:41:40 +0200 (CEST) Received: by mail-wm0-f48.google.com with SMTP id q128so54728344wma.1 for ; Wed, 20 Jul 2016 05:41:40 -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=6dUBfuMHmN7NCvyQOpJLREhBUQgLuTLuB8hHpHCDxGY=; b=IWSRBhi6EPPEQ1EuurgWENCsb+TTX/6gCGIQqpfaVtj/jVB21CqY++mZKXKoB+50fY xF8rEVy2cqp2fUAftYPLTkQ8xw1SNSbDJHyvVHZppMS8RuUqO9ff7AnobQl/zlJ49oSH /LEqtEwIKdTi/vhPiAy/oGuRCtK1ZnIWngPS8= 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=6dUBfuMHmN7NCvyQOpJLREhBUQgLuTLuB8hHpHCDxGY=; b=B7/OkGI5Nhi4PULGkqSBlH4e2/FLaBApcrTTO2pKrjOZJFU2T+YNwdakvTCJG94gCf 4ay0TAp7ywrCyFfct39EqYUy2x9SED0g1qI1zTenKfY4YreiYzLt6Zf7eU/R3B8Qmffq pwWVYdHFn81EZj0mNJUbVO7Os/2JdLUgDp4q8v3xBObyrSbk2RQd0oHmPj7ka/I3zECS KVcvHnhkdKnBErafmVGkz1Vmjjd596RI5dQJBNxYtu011qWweheNpIURWoqizrai96Gj sD45zXVgwIzMg7CoKvsdYZv4TRNDXUBKz+KcwCjmPm3xlLl3BfCYUsnJOYO0ITZfZ8i9 Q2ww== X-Gm-Message-State: ALyK8tICjID+cWmGjtxynvZn7bvFHW5c5MDw5pT0hpYu2VMqhPMlJXa9FRTUIRxDXgtcmjAC X-Received: by 10.28.48.71 with SMTP id w68mr10898613wmw.4.1469018500328; Wed, 20 Jul 2016 05:41:40 -0700 (PDT) Received: from [172.18.45.108] ([195.11.233.227]) by smtp.googlemail.com with ESMTPSA id e7sm1103563wjg.17.2016.07.20.05.41.39 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 20 Jul 2016 05:41:39 -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> From: Zoltan Kiss Message-ID: <578F7183.6030805@linaro.org> Date: Wed, 20 Jul 2016 13:41:39 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 MIME-Version: 1.0 In-Reply-To: <85d13abf-b05e-67f0-16fd-3279cb4064be@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 12:41:40 -0000 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. > > 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 >