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

[Burakov, Anatoly]

On 5/23/2023 4:45 AM, Ruifeng Wang wrote:
>> -----Original Message-----
>> From: Burakov, Anatoly <anatoly.burakov at intel.com>
>> Sent: Monday, May 22, 2023 6:19 PM
>> To: Ruifeng Wang <Ruifeng.Wang at arm.com>; olivier.matz at 6wind.com
>> Cc: dev at dpdk.org; stable at dpdk.org; thomas at monjalon.net; stephen at networkplumber.org; Justin
>> He <Justin.He at arm.com>; Honnappa Nagarahalli <Honnappa.Nagarahalli at arm.com>; nd
>> <nd at 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 at intel.com>
>>>> Sent: Monday, May 22, 2023 5:24 PM
>>>> To: Ruifeng Wang <Ruifeng.Wang at arm.com>; olivier.matz at 6wind.com
>>>> Cc: dev at dpdk.org; stable at dpdk.org; thomas at monjalon.net;
>>>> stephen at networkplumber.org; Justin He <Justin.He at arm.com>; Honnappa
>>>> Nagarahalli <Honnappa.Nagarahalli at arm.com>; nd <nd at 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 at dpdk.org
>>>>>
>>>>> Signed-off-by: Jia He <justin.he at arm.com>
>>>>> Signed-off-by: Ruifeng Wang <ruifeng.wang at 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 at intel.com>

-- 
Thanks,
Anatoly