[dpdk-dev] [PATCH] eal/service: fix exit by resetting service lcores
David Marchand
david.marchand at redhat.com
Wed Mar 11 10:09:04 CET 2020
On Tue, Mar 10, 2020 at 5:38 PM Van Haaren, Harry
<harry.van.haaren at intel.com> wrote:
>
> > -----Original Message-----
> > From: David Marchand <david.marchand at redhat.com>
> > Sent: Tuesday, March 10, 2020 4:31 PM
> > To: Van Haaren, Harry <harry.van.haaren at intel.com>
> > Cc: dev <dev at dpdk.org>; Aaron Conole <aconole at redhat.com>
> > Subject: Re: [PATCH] eal/service: fix exit by resetting service lcores
> >
> > On Tue, Mar 10, 2020 at 2:32 PM Harry van Haaren
> > <harry.van.haaren at intel.com> wrote:
> > >
> > > This commit releases all service cores from thier role,
> > > returning them to ROLE_RTE on rte_service_finalize().
> > >
> > > This may fix an issue relating to the service cores causing
> > > a race-condition on eal_cleanup(), where the service core
> > > could still be executing while the main thread has already
> > > free-d the service memory, leading to a segfault.
> >
> > Adding rte_service_lcore_reset_all() just tells a (remaining) service
> > lcore to quit its loop, but does not close the race on lcore_states.
> >
> > The backtrace shows the same.
> >
> > (gdb) bt full
> > #0 rte_service_runner_func (arg=<optimized out>) at
> > ../lib/librte_eal/common/rte_service.c:455
> > service_mask = 1
> > i = <optimized out>
> > lcore = 1
> > cs = 0x1003ea200
> > #1 0x00007ffff72030ef in eal_thread_loop (arg=<optimized out>) at
> > ../lib/librte_eal/linux/eal/eal_thread.c:153
> > fct_arg = <optimized out>
> > c = 0 '\000'
> > n = <optimized out>
> > ret = <optimized out>
> > lcore_id = <optimized out>
> > thread_id = 140737203603200
> > m2s = 14
> > s2m = 22
> > cpuset = "1", '\000' <repeats 175 times>,
> > "\200\000\000\000\000\000\000\000\221\354e\360\377\177", '\000'
> > <repeats 65 times>
> > __func__ = "eal_thread_loop"
> > #2 0x00007ffff065ddd5 in start_thread () from /lib64/libpthread.so.0
> > No symbol table info available.
> > #3 0x00007ffff038702d in clone () from /lib64/libc.so.6
> > No symbol table info available.
> >
> >
> > I added a rte_eal_mp_wait_lcore(), to ensure that each service lcore
> > _did_ quit its loop.
> > @@ -123,6 +123,7 @@ rte_service_finalize(void)
> > return;
> >
> > rte_service_lcore_reset_all();
> > + rte_eal_mp_wait_lcore();
> >
> > rte_free(rte_services);
> > rte_free(lcore_states);
> >
> >
> > I can't reproduce with this.
>
> OK - that's good news, thanks for the quick testing & feedback.
>
> Agree with your analysis of the above, indeed waiting for the cores
> explicitly seems the right solution to remove the race.
Another thing that seemed odd with your patch is that the unit test
already calls rte_service_lcore_reset_all() as part of the
unregister_all() helper.
Why don't we ensure that calling
rte_service_lcore_start|stop|reset_all guarantee the service lcores
status?
Putting explicit (and documented) synchronisation points in the
rte_service API seems the right fix to me and could help remove those
rte_delay we have in the unit test.
--
David Marchand
More information about the dev
mailing list