diff mbox series

eal/service: fix exit by resetting service lcores

Message ID 20200310133304.39951-1-harry.van.haaren@intel.com (mailing list archive)
State Superseded, archived
Delegated to: David Marchand
Headers show
Series eal/service: fix exit by resetting service lcores | expand

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/travis-robot success Travis build: passed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/checkpatch warning coding style issues

Commit Message

Van Haaren, Harry March 10, 2020, 1:33 p.m. UTC
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.

Fixes: 21698354c832 ("service: introduce service cores concept")

Reported-by: David Marchand <david.marchand@redhat.com>
Reported-by: Aaron Conole <aconole@redhat.com>
Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>

---

Please note that this patch is being sent to community for
testing as I cannot reliably reproduce the reported issue on
my local setup (despite code-changes in attempts to make the
problem more visible, and instructions from David on how he
can reproduce it). Email discusson on this topic here:
https://mails.dpdk.org/archives/dev/2020-March/159584.html
---
 lib/librte_eal/common/rte_service.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

David Marchand March 10, 2020, 4:31 p.m. UTC | #1
On Tue, Mar 10, 2020 at 2:32 PM Harry van Haaren
<harry.van.haaren@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.

--
David Marchand
Van Haaren, Harry March 10, 2020, 4:38 p.m. UTC | #2
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Tuesday, March 10, 2020 4:31 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>
> Cc: dev <dev@dpdk.org>; Aaron Conole <aconole@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@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.

Will I spin up a v2 patchset with your co-authored-by added and the above
change included?
Aaron Conole March 10, 2020, 5:44 p.m. UTC | #3
"Van Haaren, Harry" <harry.van.haaren@intel.com> writes:

>> -----Original Message-----
>> From: David Marchand <david.marchand@redhat.com>
>> Sent: Tuesday, March 10, 2020 4:31 PM
>> To: Van Haaren, Harry <harry.van.haaren@intel.com>
>> Cc: dev <dev@dpdk.org>; Aaron Conole <aconole@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@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.
>
> Will I spin up a v2 patchset with your co-authored-by added and the above
> change included?

Please spin the v2 - I am currently testing with David's incremental on
my setup now.
Aaron Conole March 10, 2020, 7:14 p.m. UTC | #4
Aaron Conole <aconole@redhat.com> writes:

> "Van Haaren, Harry" <harry.van.haaren@intel.com> writes:
>
>>> -----Original Message-----
>>> From: David Marchand <david.marchand@redhat.com>
>>> Sent: Tuesday, March 10, 2020 4:31 PM
>>> To: Van Haaren, Harry <harry.van.haaren@intel.com>
>>> Cc: dev <dev@dpdk.org>; Aaron Conole <aconole@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@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.
>>
>> Will I spin up a v2 patchset with your co-authored-by added and the above
>> change included?
>
> Please spin the v2 - I am currently testing with David's incremental on
> my setup now.

Additionally, with the incremental:

Acked-by: Aaron Conole <aconole@redhat.com>

Please make sure to cc stable@.
David Marchand March 11, 2020, 9:09 a.m. UTC | #5
On Tue, Mar 10, 2020 at 5:38 PM Van Haaren, Harry
<harry.van.haaren@intel.com> wrote:
>
> > -----Original Message-----
> > From: David Marchand <david.marchand@redhat.com>
> > Sent: Tuesday, March 10, 2020 4:31 PM
> > To: Van Haaren, Harry <harry.van.haaren@intel.com>
> > Cc: dev <dev@dpdk.org>; Aaron Conole <aconole@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@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.
diff mbox series

Patch

diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index 7e537b8cd..d400ccf79 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -122,6 +122,8 @@  rte_service_finalize(void)
 	if (!rte_service_library_initialized)
 		return;
 
+	rte_service_lcore_reset_all();
+
 	rte_free(rte_services);
 	rte_free(lcore_states);