> -----Original Message-----
> From: Ferruh Yigit <ferruh.yi...@intel.com>
> Sent: Friday, October 16, 2020 18:39
> To: Slava Ovsiienko <viachesl...@nvidia.com>; dev@dpdk.org
> Cc: NBU-Contact-Thomas Monjalon <tho...@monjalon.net>;
> step...@networkplumber.org; olivier.m...@6wind.com;
> jerinjac...@gmail.com; maxime.coque...@redhat.com;
> david.march...@redhat.com; arybche...@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 <viachesl...@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
