Re: [dpdk-dev] [PATCH v11 2/6] app/testpmd: add multiple pools per core creation

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 10/16/2020 4:48 PM, Slava Ovsiienko wrote:
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Friday, October 16, 2020 18:39
>> To: Slava Ovsiienko <viacheslavo@nvidia.com>; dev@dpdk.org
>> Cc: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>;
>> stephen@networkplumber.org; olivier.matz@6wind.com;
>> jerinjacobk@gmail.com; maxime.coquelin@redhat.com;
>> david.marchand@redhat.com; arybchenko@solarflare.com
>> Subject: Re: [dpdk-dev] [PATCH v11 2/6] app/testpmd: add multiple pools per
>> core creation
>>
>> On 10/16/2020 4:05 PM, Ferruh Yigit wrote:
>>> On 10/16/2020 2:39 PM, Viacheslav Ovsiienko wrote:
>>>> The command line parameter --mbuf-size is updated, it can handle the
>>>> multiple values like the following:
>>>>
>>>> --mbuf-size=2176,512,768,4096
>>>>
>>>> specifying the creation the extra memory pools with the requested
>>>> mbuf data buffer sizes. If some buffer split feature is engaged the
>>>> extra memory pools can be used to configure the Rx queues with
>>>> rte_the_dev_rx_queue_setup_ex().
>>>>
>>>> The extra pools are created with requested sizes, and pool names are
>>>> assigned with appended index: mbuf_pool_socket_%socket_%index.
>>>> Index zero is used to specify the first mandatory pool to maintain
>>>> compatibility with existing code.
>>>>
>>>> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
>>>
>>> <...>
>>>
>>>>    /* Mbuf Pools */
>>>>    static inline void
>>>> -mbuf_poolname_build(unsigned int sock_id, char* mp_name, int
>>>> name_size)
>>>> +mbuf_poolname_build(unsigned int sock_id, char *mp_name,
>>>> +            int name_size, unsigned int idx)
>>>>    {
>>>> -    snprintf(mp_name, name_size, "mbuf_pool_socket_%u", sock_id);
>>>> +    if (!idx)
>>>> +        snprintf(mp_name, name_size, "mbuf_pool_socket_%u",
>>>> +sock_id);
>>>> +    else
>>>> +        snprintf(mp_name, name_size, "mbuf_pool_socket_%u_%u",
>>>> +             sock_id, idx);
>>>
>>> 'mp_name' can theoretically overflow and gives a compiler warning,
>>> although not sure if this truncation is a problem in practice.
>>>
>>> ../app/test-pmd/testpmd.c: In function ‘rx_queue_setup’:
>>> ../app/test-pmd/testpmd.h:666:53: error: ‘%u’ directive output may be
>>> truncated writing between 1 and 10 bytes into a region of size between
>>> 0 and 7 [-Werror=format-truncation=]
>>>    666 |   snprintf(mp_name, name_size, "mbuf_pool_socket_%u_%u",
>>>        |                                                     ^~
>>> ../app/test-pmd/testpmd.h:666:32: note: directive argument in the
>>> range [1, 4294967295]
>>>     666 |   snprintf(mp_name, name_size, "mbuf_pool_socket_%u_%u",
>>>         |                                ^~~~~~~~~~~~~~~~~~~~~~~~
>>> ../app/test-pmd/testpmd.h:666:3: note: ‘snprintf’ output between 21
>>> and 39 bytes into a destination of size 26
>>>     666 |   snprintf(mp_name, name_size, "mbuf_pool_socket_%u_%u",
>>>         |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>     667 |     sock_id, idx);
>>>         |     ~~~~~~~~~~~~~
>>>
>>> Any suggestion for fix? Can we shorten the string above, is it used
>>> somewhere else? Or casting variables to a smaller size may work too..
>>
>> What do you think to following:
>>
>>    diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
>>    index 4cd0f967f0..4ac1c1f86e 100644
>>    --- a/app/test-pmd/testpmd.h
>>    +++ b/app/test-pmd/testpmd.h
>>    @@ -661,10 +661,11 @@ mbuf_poolname_build(unsigned int sock_id, char
>> *mp_name,
>>                        int name_size, unsigned int idx)
>>     {
>>            if (!idx)
>>    -               snprintf(mp_name, name_size, "mbuf_pool_socket_%u", sock_id);
>>    +               snprintf(mp_name, name_size, "mbuf_pool_s%u",
>>    +                       (uint16_t)sock_id);
>>            else
>>    -               snprintf(mp_name, name_size, "mbuf_pool_socket_%u_%u",
>>    -                        sock_id, idx);
>>    +               snprintf(mp_name, name_size, "mbuf_pool_s%u_%u",
>>    +                       (uint16_t)sock_id, (uint16_t)idx);
>>     }
>>
>>     static inline struct rte_mempool *
> 
> Testpmd build mbuf pool names with mbuf_poolname_build() routine only
> (invoked from mbuf_pool_find/mbuf_pool_create). I suppose it is safe
> to replace "mbuf_pool_socket_" prefix with shorter one.
> 
> Say something like this: #define TESTPMD_MBUF_POOL_PFX "mbuf"
> 

Just 'mbuf' may not give enough context in the logs, what about "mbuf_pool_%u_%u"?

> Truncating idx to uint16_t is OK, let me update the types of routines.
> sock_id seems to be OK either. If you agree on both - please, let me update the  patch.
> 

If variables are not truncated, it leaves very small place for prefix, 3 chars 
if I calculate correctly, so it seems we need to truncate it anyway.