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

[Slava Ovsiienko]

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

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.

With best regards, Slava