Re: [PATCH] test/mbuf: fix the forked process segment fault

[Thomas Monjalon <thomas@monjalon.net>]

23/05/2023 12:12, Burakov, Anatoly:
> On 5/23/2023 4:45 AM, Ruifeng Wang wrote:
> >> -----Original Message-----
> >> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> >> Sent: Monday, May 22, 2023 6:19 PM
> >> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; olivier.matz@6wind.com
> >> Cc: dev@dpdk.org; stable@dpdk.org; thomas@monjalon.net; stephen@networkplumber.org; Justin
> >> He <Justin.He@arm.com>; Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; nd
> >> <nd@arm.com>
> >> Subject: Re: [PATCH] test/mbuf: fix the forked process segment fault
> >>
> >> On 5/22/2023 10:55 AM, Ruifeng Wang wrote:
> >>>> -----Original Message-----
> >>>> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> >>>> Sent: Monday, May 22, 2023 5:24 PM
> >>>> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; olivier.matz@6wind.com
> >>>> Cc: dev@dpdk.org; stable@dpdk.org; thomas@monjalon.net;
> >>>> stephen@networkplumber.org; Justin He <Justin.He@arm.com>; Honnappa
> >>>> Nagarahalli <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
> >>>> Subject: Re: [PATCH] test/mbuf: fix the forked process segment fault
> >>>>
> >>>> On 5/22/2023 7:01 AM, Ruifeng Wang wrote:
> >>>>> Access of any memory in the hugepage shared file-backed area will
> >>>>> trigger an unexpected forked child process segment fault. The root
> >>>>> cause is DPDK doesn't support fork model [1] (calling rte_eal_init() before fork()).
> >>>>> Forked child process can't be treated as a secondary process.
> >>>>>
> >>>>> Hence fix it by avoiding fork and doing verification in the main process.
> >>>>>
> >>>>> [1] https://mails.dpdk.org/archives/dev/2018-July/108106.html
> >>>>>
> >>>>> Fixes: af75078fece3 ("first public release")
> >>>>> Cc: stable@dpdk.org
> >>>>>
> >>>>> Signed-off-by: Jia He <justin.he@arm.com>
> >>>>> Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> >>>>> ---
> >>>>
> >>>> Would this be something that a secondary process-based test could test?
> >>>> That's how we test rte_panic() and other calls.
> >>>
> >>> This case validates mbuf. IMO there is no need to do validation in a secondary process.
> >>> Unit test for rte_panic() also uses fork() and could have the same issue.
> >>>
> >>
> >> In that case, rte_panic() test should be fixed as well.
> >>
> >> My concern is that ideally, we shouldn't intentionally crash the test app if something
> >> goes wrong, and calling rte_panic() accomplishes just that - which is why I suggested
> >> running them in secondary processes instead, so that any call into rte_panic happens
> >> inside a secondary process, and the main test process doesn't crash even if the test has
> >> failed.
> > 
> > Agree that intentionally crashing the test app is bad.
> > In this patch, verification of mbuf is changed to use another API without rte_panic().
> > Then the verification can be done directly in the primary. And the indirectness of
> > using a secondary process is removed. Because verification will not crash the process.
> > 
> 
> Oh,
> 
> My apologies, I did not notice that. In that case,
> 
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

Applied, thanks.