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 at 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
>
