| Message ID | 20230706225037.1164380-8-axelrasmussen@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add UFFDIO_POISON to simulate memory poisoning with UFFD | expand |
On Thu, Jul 06, 2023 at 03:50:35PM -0700, Axel Rasmussen wrote: > @@ -247,9 +245,13 @@ static int userfaultfd_stress(void) > { > void *area; > unsigned long nr; > - struct uffd_args args[nr_cpus]; > + struct uffd_args *args; > uint64_t mem_size = nr_pages * page_size; > > + args = calloc(nr_cpus, sizeof(struct uffd_args)); > + if (!args) > + err("allocating args array failed"); This is trivial, but I think I requested a "free" (or keep it allocate on stack) in previous version but it didn't get a response on why we cannot and it kept going.. could you help explain?
On Fri, Jul 7, 2023 at 6:42 AM Peter Xu <peterx@redhat.com> wrote: > > On Thu, Jul 06, 2023 at 03:50:35PM -0700, Axel Rasmussen wrote: > > @@ -247,9 +245,13 @@ static int userfaultfd_stress(void) > > { > > void *area; > > unsigned long nr; > > - struct uffd_args args[nr_cpus]; > > + struct uffd_args *args; > > uint64_t mem_size = nr_pages * page_size; > > > > + args = calloc(nr_cpus, sizeof(struct uffd_args)); > > + if (!args) > > + err("allocating args array failed"); > > This is trivial, but I think I requested a "free" (or keep it allocate on > stack) in previous version but it didn't get a response on why we cannot > and it kept going.. could you help explain? Oh, sorry! I had meant to change this after our discussion, and simply overlooked it while reworking the patches. I'll include this change in a v4 which also addresses e.g. the comments on commit 1. > > -- > Peter Xu >
On Fri, Jul 7, 2023 at 10:03 AM Axel Rasmussen <axelrasmussen@google.com> wrote: > > On Fri, Jul 7, 2023 at 6:42 AM Peter Xu <peterx@redhat.com> wrote: > > > > On Thu, Jul 06, 2023 at 03:50:35PM -0700, Axel Rasmussen wrote: > > > @@ -247,9 +245,13 @@ static int userfaultfd_stress(void) > > > { > > > void *area; > > > unsigned long nr; > > > - struct uffd_args args[nr_cpus]; > > > + struct uffd_args *args; > > > uint64_t mem_size = nr_pages * page_size; > > > > > > + args = calloc(nr_cpus, sizeof(struct uffd_args)); > > > + if (!args) > > > + err("allocating args array failed"); > > > > This is trivial, but I think I requested a "free" (or keep it allocate on > > stack) in previous version but it didn't get a response on why we cannot > > and it kept going.. could you help explain? > > Oh, sorry! I had meant to change this after our discussion, and simply > overlooked it while reworking the patches. > > I'll include this change in a v4 which also addresses e.g. the > comments on commit 1. Ah, so I tried switching back to the {0} initializer, and was reminded why I didn't do that in v1. :) Ignoring the missing braces warning I talked about before, using {0} here is actually an error ("variable-sized object may not be initialized") because this is a variable sized array (nr_cpus isn't constant). So, that option is out. I'm not a huge fan of adding the free() cleanup and dealing with all of the err() calls this function has. Originally I switched to calloc() because I'm not a big fan of VLAs anyway. But, as a compromise in v4 I'll leave it a VLA, and switch to memset() for initializing it. > > > > > -- > > Peter Xu > >
On Fri, Jul 07, 2023 at 01:38:16PM -0700, Axel Rasmussen wrote: > Ah, so I tried switching back to the {0} initializer, and was reminded > why I didn't do that in v1. :) Ignoring the missing braces warning I > talked about before, using {0} here is actually an error > ("variable-sized object may not be initialized") because this is a > variable sized array (nr_cpus isn't constant). So, that option is out. > > I'm not a huge fan of adding the free() cleanup and dealing with all > of the err() calls this function has. Oh, that's definitely not needed - as long as we know we're going to quit, we let kernel clean everything is fine. I just worry in the future there can be a loop of userfaultfd_stress() so it can OOM a host even if no err() hit but by looping. I hope I explained what I meant.. so it's still good we make sure things freed properly when in success paths and when we're at it. > > Originally I switched to calloc() because I'm not a big fan of VLAs > anyway. But, as a compromise in v4 I'll leave it a VLA, and switch to > memset() for initializing it. That'll be good enough to me. Thanks a lot,
diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c index ba20d7504022..02b89860e193 100644 --- a/tools/testing/selftests/mm/uffd-common.c +++ b/tools/testing/selftests/mm/uffd-common.c @@ -499,6 +499,9 @@ void *uffd_poll_thread(void *arg) int ret; char tmp_chr; + if (!args->handle_fault) + args->handle_fault = uffd_handle_page_fault; + pollfd[0].fd = uffd; pollfd[0].events = POLLIN; pollfd[1].fd = pipefd[cpu*2]; @@ -527,7 +530,7 @@ void *uffd_poll_thread(void *arg) err("unexpected msg event %u\n", msg.event); break; case UFFD_EVENT_PAGEFAULT: - uffd_handle_page_fault(&msg, args); + args->handle_fault(&msg, args); break; case UFFD_EVENT_FORK: close(uffd); diff --git a/tools/testing/selftests/mm/uffd-common.h b/tools/testing/selftests/mm/uffd-common.h index 197f5262fe0d..7c4fa964c3b0 100644 --- a/tools/testing/selftests/mm/uffd-common.h +++ b/tools/testing/selftests/mm/uffd-common.h @@ -77,6 +77,9 @@ struct uffd_args { unsigned long missing_faults; unsigned long wp_faults; unsigned long minor_faults; + + /* A custom fault handler; defaults to uffd_handle_page_fault. */ + void (*handle_fault)(struct uffd_msg *msg, struct uffd_args *args); }; struct uffd_test_ops { diff --git a/tools/testing/selftests/mm/uffd-stress.c b/tools/testing/selftests/mm/uffd-stress.c index 995ff13e74c7..50b1224d72c7 100644 --- a/tools/testing/selftests/mm/uffd-stress.c +++ b/tools/testing/selftests/mm/uffd-stress.c @@ -189,10 +189,8 @@ static int stress(struct uffd_args *args) locking_thread, (void *)cpu)) return 1; if (bounces & BOUNCE_POLL) { - if (pthread_create(&uffd_threads[cpu], &attr, - uffd_poll_thread, - (void *)&args[cpu])) - return 1; + if (pthread_create(&uffd_threads[cpu], &attr, uffd_poll_thread, &args[cpu])) + err("uffd_poll_thread create"); } else { if (pthread_create(&uffd_threads[cpu], &attr, uffd_read_thread, @@ -247,9 +245,13 @@ static int userfaultfd_stress(void) { void *area; unsigned long nr; - struct uffd_args args[nr_cpus]; + struct uffd_args *args; uint64_t mem_size = nr_pages * page_size; + args = calloc(nr_cpus, sizeof(struct uffd_args)); + if (!args) + err("allocating args array failed"); + if (uffd_test_ctx_init(UFFD_FEATURE_WP_UNPOPULATED, NULL)) err("context init failed");
Previously, we had "one fault handler to rule them all", which used several branches to deal with all of the scenarios required by all of the various tests. In upcoming patches, I plan to add a new test, which has its own slightly different fault handling logic. Instead of continuing to add cruft to the existing fault handler, let's allow tests to define custom ones, separate from other tests. Signed-off-by: Axel Rasmussen <axelrasmussen@google.com> --- tools/testing/selftests/mm/uffd-common.c | 5 ++++- tools/testing/selftests/mm/uffd-common.h | 3 +++ tools/testing/selftests/mm/uffd-stress.c | 12 +++++++----- 3 files changed, 14 insertions(+), 6 deletions(-)