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

[Ruifeng Wang <Ruifeng.Wang@arm.com>]

> -----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.

> 
> --
> Thanks,
> Anatoly