On Tue, Mar 10, 2020 at 5:38 PM Van Haaren, Harry <harry.van.haa...@intel.com> wrote: > > > -----Original Message----- > > From: David Marchand <david.march...@redhat.com> > > Sent: Tuesday, March 10, 2020 4:31 PM > > To: Van Haaren, Harry <harry.van.haa...@intel.com> > > Cc: dev <dev@dpdk.org>; Aaron Conole <acon...@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.haa...@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
