[External] Re: [PATCH] mempool: fix rte_mempool_avail_count may segment fault when used in multiprocess

[Olivier Matz]

Hi,

On Tue, Nov 15, 2022 at 08:47:15AM +0100, David Marchand wrote:
> On Tue, Nov 15, 2022 at 2:51 AM Fengnan Chang
> <changfengnan at bytedance.com> wrote:
> >
> > David Marchand <david.marchand at redhat.com> 于2022年11月15日周二 04:44写道：
> > >
> > > On Mon, Nov 14, 2022 at 9:13 AM changfengnan <changfengnan at bytedance.com> wrote:
> > > >
> > > > rte_mempool_create put tailq entry into rte_mempool_tailq list before
> > > > populate, and pool_data set when populate. So in multi process, if
> > > > process A create mempool, and process B can get mempool through
> > > > rte_mempool_lookup before pool_data set, if B call rte_mempool_lookup,
> > > > it will cause segment fault.
> > >
> > > I fail to see how pool_data impacts rte_mempool_lookup.
> > > Something is fishy about this commitlog.
> >
> > oh, it's my fault about this commit. correct: if B can get mempool through
> > rte_mempool_lookup before pool_data set, and call rte_mempool_avail_count,
> > it will cause segment fault.
> 
> Ok, now it makes more sense :-).
> 
> >
> > >
> > >
> > > > Fix this by put tailq entry into rte_mempool_tailq after populate.
> > >
> > > Moving tailq manipulation to rte_mempool_create only, is probably incorrect.
> > > An application is allowed to call rte_mempool_create_empty() and
> > > rte_mempool_populate().
> > >
> > > I did not look in depth, but It is likely the reason why testpmd (as
> > > run with devtools/test-null.sh) won't pass anymore.
> > > The CI reported this issue in various envs.
> > >
> > > We can't take this patch.
> >
> > Yeah, this version makes CI fail.
> > I didn't notice rte_mempool_create_empty will called directly before, maybe
> > add a new flag bit to indicate when to put tailq entry into rte_mempool_tailq
> > list is a better way. If no better idea, I'll send a new version.
> 
> I don't think we need an other flag.
> Can we "publish" the mempool at the mempool_ops_alloc_once stage?

The mempool_ops_alloc_once() seems it is the proper place, yes.

Alternatively, I suppose this issue can be fixed in the secondary
application:

- it can wait that the flag RTE_MEMPOOL_F_POOL_CREATED is present before
  using the mempool.

- or it can wait the RTE_MEMPOOL_EVENT_READY

- or it can wait that the whole initialization of the primary
  application is finished by another mean (a sort of lock). I don't know
  the exact use case, but to me, it looks sane to do that, it would
  protect from other similar issues.


Olivier

> 
> 
> >
> > >
> > >
> > > >
> > > > Signed-off-by: changfengnan <changfengnan at bytedance.com>
> > >
> > > Please use your real name.
> >
> > It's my real name.
> 
> Sorry, I meant your full name, like Fengnan Chang <changfengnan at bytedance.com>
> 
> 
> -- 
> David Marchand
>