[PATCH] dumpcap: fix mbuf pool ring type

[Morten Brørup]

> From: David Marchand [mailto:david.marchand at redhat.com]
> Sent: Monday, 2 October 2023 09.34
> 
> On Fri, Aug 4, 2023 at 6:16 PM Stephen Hemminger
> <stephen at networkplumber.org> wrote:
> >
> > The ring used to store mbufs needs to be multiple producer,
> > multiple consumer because multiple queues might on multiple
> > cores might be allocating and same time (consume) and in
> > case of ring full, the mbufs will be returned (multiple producer).
> 
> I think I get the idea, but can you rephrase please?
> 
> 
> >
> > Bugzilla ID: 1271
> > Fixes: cb2440fd77af ("dumpcap: fix mbuf pool ring type")
> 
> This Fixes: tag looks wrong.
> 
> 
> > Signed-off-by: Stephen Hemminger <stephen at networkplumber.org>
> > ---
> >  app/dumpcap/main.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c
> > index 64294bbfb3e6..991174e95022 100644
> > --- a/app/dumpcap/main.c
> > +++ b/app/dumpcap/main.c
> > @@ -691,10 +691,9 @@ static struct rte_mempool *create_mempool(void)
> >                         data_size = mbuf_size;
> >         }
> >
> > -       mp = rte_pktmbuf_pool_create_by_ops(pool_name, num_mbufs,
> > -                                           MBUF_POOL_CACHE_SIZE, 0,
> > -                                           data_size,
> > -                                           rte_socket_id(), "ring_mp_sc");
> > +       mp = rte_pktmbuf_pool_create(pool_name, num_mbufs,
> > +                                    MBUF_POOL_CACHE_SIZE, 0,
> > +                                    data_size, rte_socket_id());
> 
> Switching to rte_pktmbuf_pool_create() still leaves the user with the
> possibility to shoot himself in the foot (I was thinking of setting
> some --mbuf-pool-ops-name EAL option).
> 
> This application has explicit requirements in terms of concurrent
> access (and I don't think the mempool library exposes per driver
> capabilities in that regard).
> The application was enforcing the use of mempool/ring so far.
> 
> I think it is safer to go with an explicit
> rte_pktmbuf_pool_create_by_ops(... "ring_mp_mc").
> WDYT?

<feature creep>
Or perhaps one of "ring_mt_rts" or "ring_mt_hts", if any of those mbuf pool drivers are specified on the command line; otherwise fall back to "ring_mp_mc".

Actually, I prefer Stephen's suggestion of using the default mbuf pool driver. The option is there for a reason.

However, David is right: We want to prevent the user from using a thread-unsafe mempool driver in this use case.

And I guess there might be other use cases than this one, where a thread-safe mempool driver is required. So adding a generalized function to get the "upgraded" (i.e. thread safe) variant of a mempool driver would be nice.
</feature creep>

Feel free to ignore my suggested feature creep, and go ahead with David's suggestion instead.

> 
> 
> >         if (mp == NULL)
> >                 rte_exit(EXIT_FAILURE,
> >                          "Mempool (%s) creation failed: %s\n", pool_name,
> > --
> > 2.39.2
> >
> 
> Thanks.
> 
> --
> David Marchand