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

[Ferruh Yigit]

On 10/16/2020 4:55 PM, Slava Ovsiienko wrote:
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit at intel.com>
>> Sent: Friday, October 16, 2020 18:52
>> To: Slava Ovsiienko <viacheslavo at nvidia.com>; dev at dpdk.org
>> Cc: NBU-Contact-Thomas Monjalon <thomas at monjalon.net>;
>> stephen at networkplumber.org; olivier.matz at 6wind.com;
>> jerinjacobk at gmail.com; maxime.coquelin at redhat.com;
>> david.marchand at redhat.com; arybchenko at solarflare.com
>> Subject: Re: [dpdk-dev] [PATCH v11 2/6] app/testpmd: add multiple pools per
>> core creation
>>
>> On 10/16/2020 4:48 PM, Slava Ovsiienko wrote:
>>>> -----Original Message-----
>>>> From: Ferruh Yigit <ferruh.yigit at intel.com>
>>>> Sent: Friday, October 16, 2020 18:39
>>>> To: Slava Ovsiienko <viacheslavo at nvidia.com>; dev at dpdk.org
>>>> Cc: NBU-Contact-Thomas Monjalon <thomas at monjalon.net>;
>>>> stephen at networkplumber.org; olivier.matz at 6wind.com;
>>>> jerinjacobk at gmail.com; maxime.coquelin at redhat.com;
>>>> david.marchand at redhat.com; arybchenko at 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 at 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"?
> It is place in the pool structure, so it is an object name
> and tightly coupled with the pool.  OK, let's try "mb_pool" ?
> 

sounds good