[dpdk-dev] [PATCH v11 2/6] app/testpmd: add multiple pools per core creation
Ferruh Yigit
ferruh.yigit at intel.com
Fri Oct 16 17:52:18 CEST 2020
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"?
> 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.
More information about the dev
mailing list