[1/2] eal: fix failure race and behavior of thread create
Checks
Commit Message
In rte_thread_create setting affinity after pthread_create may fail.
Such a failure should result in the entire rte_thread_create failing
but doesn't.
Additionally if there is a failure to set affinity a race exists where
the creating thread will free ctx and depending on scheduling of the new
thread it may also free ctx (double free).
Resolve both of the above issues by using the pthread_setaffinity_np
prior to thread creation to set the affinity of the created thread. By
doing this no failure paths exist after pthread_create returns
successfully.
Fixes: ce6e911d20f6 ("eal: add thread lifetime API")
Cc: stable@dpdk.org
Cc: roretzla@linux.microsoft.com
Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
lib/eal/unix/rte_thread.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
Comments
On Thu, Mar 2, 2023 at 7:44 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> In rte_thread_create setting affinity after pthread_create may fail.
> Such a failure should result in the entire rte_thread_create failing
> but doesn't.
>
> Additionally if there is a failure to set affinity a race exists where
> the creating thread will free ctx and depending on scheduling of the new
> thread it may also free ctx (double free).
>
> Resolve both of the above issues by using the pthread_setaffinity_np
> prior to thread creation to set the affinity of the created thread. By
> doing this no failure paths exist after pthread_create returns
> successfully.
>
> Fixes: ce6e911d20f6 ("eal: add thread lifetime API")
> Cc: stable@dpdk.org
> Cc: roretzla@linux.microsoft.com
>
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
Reviewed-by: David Marchand <david.marchand@redhat.com>
On Tue, Mar 7, 2023 at 3:33 PM David Marchand <david.marchand@redhat.com> wrote:
> On Thu, Mar 2, 2023 at 7:44 PM Tyler Retzlaff
> <roretzla@linux.microsoft.com> wrote:
> >
> > In rte_thread_create setting affinity after pthread_create may fail.
> > Such a failure should result in the entire rte_thread_create failing
> > but doesn't.
> >
> > Additionally if there is a failure to set affinity a race exists where
> > the creating thread will free ctx and depending on scheduling of the new
> > thread it may also free ctx (double free).
> >
> > Resolve both of the above issues by using the pthread_setaffinity_np
> > prior to thread creation to set the affinity of the created thread. By
> > doing this no failure paths exist after pthread_create returns
> > successfully.
> >
> > Fixes: ce6e911d20f6 ("eal: add thread lifetime API")
> > Cc: stable@dpdk.org
> > Cc: roretzla@linux.microsoft.com
> >
> > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> Reviewed-by: David Marchand <david.marchand@redhat.com>
Series applied, thanks.
09/03/2023 10:17, David Marchand:
> On Tue, Mar 7, 2023 at 3:33 PM David Marchand <david.marchand@redhat.com> wrote:
> > On Thu, Mar 2, 2023 at 7:44 PM Tyler Retzlaff
> > <roretzla@linux.microsoft.com> wrote:
> > >
> > > In rte_thread_create setting affinity after pthread_create may fail.
> > > Such a failure should result in the entire rte_thread_create failing
> > > but doesn't.
> > >
> > > Additionally if there is a failure to set affinity a race exists where
> > > the creating thread will free ctx and depending on scheduling of the new
> > > thread it may also free ctx (double free).
> > >
> > > Resolve both of the above issues by using the pthread_setaffinity_np
> > > prior to thread creation to set the affinity of the created thread. By
> > > doing this no failure paths exist after pthread_create returns
> > > successfully.
> > >
> > > Fixes: ce6e911d20f6 ("eal: add thread lifetime API")
> > > Cc: stable@dpdk.org
> > > Cc: roretzla@linux.microsoft.com
> > >
> > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > Reviewed-by: David Marchand <david.marchand@redhat.com>
>
> Series applied, thanks.
Unfortunately we cannot merge this patch
because it does not compile on Alpine Linux (musl libc):
lib/eal/unix/rte_thread.c:160:31: error:
implicit declaration of function 'pthread_attr_setaffinity_np'
Is it possible to fix the race without using pthread_attr_setaffinity_np?
On Thu, Mar 09, 2023 at 10:58:06AM +0100, Thomas Monjalon wrote:
> 09/03/2023 10:17, David Marchand:
> > On Tue, Mar 7, 2023 at 3:33 PM David Marchand <david.marchand@redhat.com> wrote:
> > > On Thu, Mar 2, 2023 at 7:44 PM Tyler Retzlaff
> > > <roretzla@linux.microsoft.com> wrote:
> > > >
> > > > In rte_thread_create setting affinity after pthread_create may fail.
> > > > Such a failure should result in the entire rte_thread_create failing
> > > > but doesn't.
> > > >
> > > > Additionally if there is a failure to set affinity a race exists where
> > > > the creating thread will free ctx and depending on scheduling of the new
> > > > thread it may also free ctx (double free).
> > > >
> > > > Resolve both of the above issues by using the pthread_setaffinity_np
> > > > prior to thread creation to set the affinity of the created thread. By
> > > > doing this no failure paths exist after pthread_create returns
> > > > successfully.
> > > >
> > > > Fixes: ce6e911d20f6 ("eal: add thread lifetime API")
> > > > Cc: stable@dpdk.org
> > > > Cc: roretzla@linux.microsoft.com
> > > >
> > > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > Reviewed-by: David Marchand <david.marchand@redhat.com>
> >
> > Series applied, thanks.
>
> Unfortunately we cannot merge this patch
> because it does not compile on Alpine Linux (musl libc):
>
> lib/eal/unix/rte_thread.c:160:31: error:
> implicit declaration of function 'pthread_attr_setaffinity_np'
i didn't get any CI failure for this. did i just miss it?
>
> Is it possible to fix the race without using pthread_attr_setaffinity_np?
>
it seems we never allowed threads to be created with a set affinity when
using pthread_create directly (that was portable to alpine linux). for worker
threads the start_routine is setting the affinity from the new thread.
certainly we can make this work by doing the same thing, but we'll have
to adjust the start routine wrapper to synchronize/wait for the new
thread to set the affinity and if it fails terminate the new thread
cleanly.
i don't have a way to build for alpine linux or run the unit tests, does
someone want to make the above suggested adjustment? or i can try and
make a patch but someone else will have to carefully review and test.
let me know how you'd like to proceed.
ty
On Thu, Mar 9, 2023 at 9:49 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> On Thu, Mar 09, 2023 at 10:58:06AM +0100, Thomas Monjalon wrote:
> > 09/03/2023 10:17, David Marchand:
> > > On Tue, Mar 7, 2023 at 3:33 PM David Marchand <david.marchand@redhat.com> wrote:
> > > > On Thu, Mar 2, 2023 at 7:44 PM Tyler Retzlaff
> > > > <roretzla@linux.microsoft.com> wrote:
> > > > >
> > > > > In rte_thread_create setting affinity after pthread_create may fail.
> > > > > Such a failure should result in the entire rte_thread_create failing
> > > > > but doesn't.
> > > > >
> > > > > Additionally if there is a failure to set affinity a race exists where
> > > > > the creating thread will free ctx and depending on scheduling of the new
> > > > > thread it may also free ctx (double free).
> > > > >
> > > > > Resolve both of the above issues by using the pthread_setaffinity_np
> > > > > prior to thread creation to set the affinity of the created thread. By
> > > > > doing this no failure paths exist after pthread_create returns
> > > > > successfully.
> > > > >
> > > > > Fixes: ce6e911d20f6 ("eal: add thread lifetime API")
> > > > > Cc: stable@dpdk.org
> > > > > Cc: roretzla@linux.microsoft.com
> > > > >
> > > > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > > Reviewed-by: David Marchand <david.marchand@redhat.com>
> > >
> > > Series applied, thanks.
> >
> > Unfortunately we cannot merge this patch
> > because it does not compile on Alpine Linux (musl libc):
> >
> > lib/eal/unix/rte_thread.c:160:31: error:
> > implicit declaration of function 'pthread_attr_setaffinity_np'
>
> i didn't get any CI failure for this. did i just miss it?
Count on me, I would have complained if there was a CI issue ;-).
>
> >
> > Is it possible to fix the race without using pthread_attr_setaffinity_np?
> >
>
> it seems we never allowed threads to be created with a set affinity when
> using pthread_create directly (that was portable to alpine linux). for worker
> threads the start_routine is setting the affinity from the new thread.
>
> certainly we can make this work by doing the same thing, but we'll have
> to adjust the start routine wrapper to synchronize/wait for the new
> thread to set the affinity and if it fails terminate the new thread
> cleanly.
>
> i don't have a way to build for alpine linux or run the unit tests, does
> someone want to make the above suggested adjustment? or i can try and
> make a patch but someone else will have to carefully review and test.
>
> let me know how you'd like to proceed.
UNH is looking into re-enabling the Alpine job.
For the time being, if you have a github repository, I can propose a
quick patch using GHA:
https://github.com/david-marchand/dpdk/commit/ci
I had tested compilation with a previous version of this patch.
I just added running the unit tests (adding a checks: tests line in
the job matrix), let's see how it goes...
https://github.com/david-marchand/dpdk/actions/runs/4378715081/jobs/7663806639
v4:
* refactor patch to use pthread_cond_t to wait for thread start
wrapper to complete work.
* rename some variables to group those that are part of the
wrapper synchronization.
* use stack instead of heap allocation for thread start context.
v3:
* don't free wrapper context from new thread now that wrapper
completion is complete before returning from creating thread.
v2:
* new approach over v1 of the patch to avoid using pthread np API that
is not available on Alpine Linux.
* to conform to rte_thread_create parameter const qualification include
an additional patch to const qualify rte_thread_set_affinity cpusetp
parameter.
Tyler Retzlaff (2):
eal: make cpusetp to rte thread set affinity const
eal: fix failure path race setting new thread affinity
lib/eal/common/eal_common_thread.c | 6 ++--
lib/eal/include/rte_thread.h | 2 +-
lib/eal/unix/rte_thread.c | 70 +++++++++++++++++++++++---------------
3 files changed, 47 insertions(+), 31 deletions(-)
@@ -155,6 +155,17 @@ struct thread_routine_ctx {
RTE_LOG(DEBUG, EAL, "pthread_attr_setschedparam failed\n");
goto cleanup;
}
+
+ if (CPU_COUNT(&thread_attr->cpuset) > 0) {
+ ret = pthread_attr_setaffinity_np(attrp,
+ sizeof(thread_attr->cpuset),
+ &thread_attr->cpuset);
+ if (ret != 0) {
+ RTE_LOG(DEBUG, EAL,
+ "pthread_attr_setaffinity_np failed\n");
+ goto cleanup;
+ }
+ }
}
ret = pthread_create((pthread_t *)&thread_id->opaque_id, attrp,
@@ -164,15 +175,6 @@ struct thread_routine_ctx {
goto cleanup;
}
- if (thread_attr != NULL && CPU_COUNT(&thread_attr->cpuset) > 0) {
- ret = rte_thread_set_affinity_by_id(*thread_id,
- &thread_attr->cpuset);
- if (ret != 0) {
- RTE_LOG(DEBUG, EAL, "rte_thread_set_affinity_by_id failed\n");
- goto cleanup;
- }
- }
-
ctx = NULL;
cleanup:
free(ctx);