| Message ID | 20240325194100.775052-1-edliaw@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] selftests/mm: Fix ARM related issue with fork after pthread_create | expand |
On Mon, 25 Mar 2024 19:40:52 +0000 Edward Liaw <edliaw@google.com> wrote: > Following issue was observed while running the uffd-unit-tests selftest > on ARM devices. On x86_64 no issues were detected: > > pthread_create followed by fork caused deadlock in certain cases > wherein fork required some work to be completed by the created thread. > Used synchronization to ensure that created thread's start function has > started before invoking fork. hm, you cc'ed the whole world apart from peterx. Fixed. > Signed-off-by: Lokesh Gidra <lokeshgidra@google.com> > [edliaw: Refactored to use atomic_bool] > Signed-off-by: Edward Liaw <edliaw@google.com> I'll add cc:stable. For which a Fixes: is desirable. I used 760aee0b71e3 ("selftests/mm: add tests for RO pinning vs fork()"), please check that.
Hello: This patch was applied to netdev/net.git (main) by Andrew Morton <akpm@linux-foundation.org>: On Mon, 25 Mar 2024 19:40:52 +0000 you wrote: > Following issue was observed while running the uffd-unit-tests selftest > on ARM devices. On x86_64 no issues were detected: > > pthread_create followed by fork caused deadlock in certain cases > wherein fork required some work to be completed by the created thread. > Used synchronization to ensure that created thread's start function has > started before invoking fork. > > [...] Here is the summary with links: - [v3] selftests/mm: Fix ARM related issue with fork after pthread_create https://git.kernel.org/netdev/net/c/8c864371b2a1 You are awesome, thank you!
On 3/26/24 12:40 AM, Edward Liaw wrote: > Following issue was observed while running the uffd-unit-tests selftest > on ARM devices. On x86_64 no issues were detected: > > pthread_create followed by fork caused deadlock in certain cases > wherein fork required some work to be completed by the created thread. > Used synchronization to ensure that created thread's start function has > started before invoking fork. > > Signed-off-by: Lokesh Gidra <lokeshgidra@google.com> > [edliaw: Refactored to use atomic_bool] > Signed-off-by: Edward Liaw <edliaw@google.com> Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.com> > --- > > v2: restored accidentally removed uffd_test_case_ops when merging > v3: fixed commit subject to use selftests/mm prefix > > tools/testing/selftests/mm/uffd-common.c | 3 +++ > tools/testing/selftests/mm/uffd-common.h | 2 ++ > tools/testing/selftests/mm/uffd-unit-tests.c | 10 ++++++++++ > 3 files changed, 15 insertions(+) > > diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c > index b0ac0ec2356d..7ad6ba660c7d 100644 > --- a/tools/testing/selftests/mm/uffd-common.c > +++ b/tools/testing/selftests/mm/uffd-common.c > @@ -18,6 +18,7 @@ bool test_uffdio_wp = true; > unsigned long long *count_verify; > uffd_test_ops_t *uffd_test_ops; > uffd_test_case_ops_t *uffd_test_case_ops; > +atomic_bool ready_for_fork; > > static int uffd_mem_fd_create(off_t mem_size, bool hugetlb) > { > @@ -518,6 +519,8 @@ void *uffd_poll_thread(void *arg) > pollfd[1].fd = pipefd[cpu*2]; > pollfd[1].events = POLLIN; > > + ready_for_fork = true; > + > for (;;) { > ret = poll(pollfd, 2, -1); > if (ret <= 0) { > diff --git a/tools/testing/selftests/mm/uffd-common.h b/tools/testing/selftests/mm/uffd-common.h > index cb055282c89c..cc5629c3d2aa 100644 > --- a/tools/testing/selftests/mm/uffd-common.h > +++ b/tools/testing/selftests/mm/uffd-common.h > @@ -32,6 +32,7 @@ > #include <inttypes.h> > #include <stdint.h> > #include <sys/random.h> > +#include <stdatomic.h> > > #include "../kselftest.h" > #include "vm_util.h" > @@ -103,6 +104,7 @@ extern bool map_shared; > extern bool test_uffdio_wp; > extern unsigned long long *count_verify; > extern volatile bool test_uffdio_copy_eexist; > +extern atomic_bool ready_for_fork; > > extern uffd_test_ops_t anon_uffd_test_ops; > extern uffd_test_ops_t shmem_uffd_test_ops; > diff --git a/tools/testing/selftests/mm/uffd-unit-tests.c b/tools/testing/selftests/mm/uffd-unit-tests.c > index 2b9f8cc52639..4a48dc617c6b 100644 > --- a/tools/testing/selftests/mm/uffd-unit-tests.c > +++ b/tools/testing/selftests/mm/uffd-unit-tests.c > @@ -775,6 +775,8 @@ static void uffd_sigbus_test_common(bool wp) > char c; > struct uffd_args args = { 0 }; > > + ready_for_fork = false; > + > fcntl(uffd, F_SETFL, uffd_flags | O_NONBLOCK); > > if (uffd_register(uffd, area_dst, nr_pages * page_size, > @@ -790,6 +792,9 @@ static void uffd_sigbus_test_common(bool wp) > if (pthread_create(&uffd_mon, NULL, uffd_poll_thread, &args)) > err("uffd_poll_thread create"); > > + while (!ready_for_fork) > + ; /* Wait for the poll_thread to start executing before forking */ > + > pid = fork(); > if (pid < 0) > err("fork"); > @@ -829,6 +834,8 @@ static void uffd_events_test_common(bool wp) > char c; > struct uffd_args args = { 0 }; > > + ready_for_fork = false; > + > fcntl(uffd, F_SETFL, uffd_flags | O_NONBLOCK); > if (uffd_register(uffd, area_dst, nr_pages * page_size, > true, wp, false)) > @@ -838,6 +845,9 @@ static void uffd_events_test_common(bool wp) > if (pthread_create(&uffd_mon, NULL, uffd_poll_thread, &args)) > err("uffd_poll_thread create"); > > + while (!ready_for_fork) > + ; /* Wait for the poll_thread to start executing before forking */ > + > pid = fork(); > if (pid < 0) > err("fork"); > -- > 2.44.0.396.g6e790dbe36-goog > >
diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c index b0ac0ec2356d..7ad6ba660c7d 100644 --- a/tools/testing/selftests/mm/uffd-common.c +++ b/tools/testing/selftests/mm/uffd-common.c @@ -18,6 +18,7 @@ bool test_uffdio_wp = true; unsigned long long *count_verify; uffd_test_ops_t *uffd_test_ops; uffd_test_case_ops_t *uffd_test_case_ops; +atomic_bool ready_for_fork; static int uffd_mem_fd_create(off_t mem_size, bool hugetlb) { @@ -518,6 +519,8 @@ void *uffd_poll_thread(void *arg) pollfd[1].fd = pipefd[cpu*2]; pollfd[1].events = POLLIN; + ready_for_fork = true; + for (;;) { ret = poll(pollfd, 2, -1); if (ret <= 0) { diff --git a/tools/testing/selftests/mm/uffd-common.h b/tools/testing/selftests/mm/uffd-common.h index cb055282c89c..cc5629c3d2aa 100644 --- a/tools/testing/selftests/mm/uffd-common.h +++ b/tools/testing/selftests/mm/uffd-common.h @@ -32,6 +32,7 @@ #include <inttypes.h> #include <stdint.h> #include <sys/random.h> +#include <stdatomic.h> #include "../kselftest.h" #include "vm_util.h" @@ -103,6 +104,7 @@ extern bool map_shared; extern bool test_uffdio_wp; extern unsigned long long *count_verify; extern volatile bool test_uffdio_copy_eexist; +extern atomic_bool ready_for_fork; extern uffd_test_ops_t anon_uffd_test_ops; extern uffd_test_ops_t shmem_uffd_test_ops; diff --git a/tools/testing/selftests/mm/uffd-unit-tests.c b/tools/testing/selftests/mm/uffd-unit-tests.c index 2b9f8cc52639..4a48dc617c6b 100644 --- a/tools/testing/selftests/mm/uffd-unit-tests.c +++ b/tools/testing/selftests/mm/uffd-unit-tests.c @@ -775,6 +775,8 @@ static void uffd_sigbus_test_common(bool wp) char c; struct uffd_args args = { 0 }; + ready_for_fork = false; + fcntl(uffd, F_SETFL, uffd_flags | O_NONBLOCK); if (uffd_register(uffd, area_dst, nr_pages * page_size, @@ -790,6 +792,9 @@ static void uffd_sigbus_test_common(bool wp) if (pthread_create(&uffd_mon, NULL, uffd_poll_thread, &args)) err("uffd_poll_thread create"); + while (!ready_for_fork) + ; /* Wait for the poll_thread to start executing before forking */ + pid = fork(); if (pid < 0) err("fork"); @@ -829,6 +834,8 @@ static void uffd_events_test_common(bool wp) char c; struct uffd_args args = { 0 }; + ready_for_fork = false; + fcntl(uffd, F_SETFL, uffd_flags | O_NONBLOCK); if (uffd_register(uffd, area_dst, nr_pages * page_size, true, wp, false)) @@ -838,6 +845,9 @@ static void uffd_events_test_common(bool wp) if (pthread_create(&uffd_mon, NULL, uffd_poll_thread, &args)) err("uffd_poll_thread create"); + while (!ready_for_fork) + ; /* Wait for the poll_thread to start executing before forking */ + pid = fork(); if (pid < 0) err("fork");