[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: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" ?