From: Fengnan Chang <changfengnan@bytedance.com>
To: Olivier Matz <olivier.matz@6wind.com>
Cc: "David Marchand" <david.marchand@redhat.com>,
andrew.rybchenko@oktetlabs.ru, dev@dpdk.org,
"Morten Brørup" <mb@smartsharesystems.com>
Subject: Re: [External] Re: [PATCH] mempool: fix rte_mempool_avail_count may segment fault when used in multiprocess
Date: Tue, 15 Nov 2022 19:30:05 +0800 [thread overview]
Message-ID: <CAPFOzZt_RQGndsUayhAfDhOHx8oLJVSw04TPL=JTZ9xVwuHojA@mail.gmail.com> (raw)
In-Reply-To: <Y3NN3z2ZVtNj3CCL@platinum>
Olivier Matz <olivier.matz@6wind.com> 于2022年11月15日周二 16:29写道：
>
> 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@bytedance.com> wrote:
> > >
> > > David Marchand <david.marchand@redhat.com> 于2022年11月15日周二 04:44写道：
> > > >
> > > > On Mon, Nov 14, 2022 at 9:13 AM changfengnan <changfengnan@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:
I found this problem in spdk, if this is fixed in the secondary app,
it needs modify
spdk too.
The following options work, but in spdk, I prefer to publish the mempool at
mempool_ops_alloc_once, I'll send a new version like this.
>
> - 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@bytedance.com>
> > > >
> > > > Please use your real name.
> > >
> > > It's my real name.
> >
> > Sorry, I meant your full name, like Fengnan Chang <changfengnan@bytedance.com>
> >
> >
> > --
> > David Marchand
> >
prev parent reply other threads:[~2022-11-15 11:33 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-14 7:14 changfengnan
2022-11-14 9:55 ` Morten Brørup
2022-11-14 11:23 ` changfengnan
2022-11-14 20:43 ` David Marchand
2022-11-15 1:51 ` [External] " Fengnan Chang
2022-11-15 7:23 ` Morten Brørup
2022-11-15 7:47 ` David Marchand
2022-11-15 8:29 ` Olivier Matz
2022-11-15 11:30 ` Fengnan Chang [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAPFOzZt_RQGndsUayhAfDhOHx8oLJVSw04TPL=JTZ9xVwuHojA@mail.gmail.com' \
--to=changfengnan@bytedance.com \
--cc=andrew.rybchenko@oktetlabs.ru \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=mb@smartsharesystems.com \
--cc=olivier.matz@6wind.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).