| Message ID | 20230706225037.1164380-1-axelrasmussen@google.com |
|---|---|
| Headers | show |
| Series | add UFFDIO_POISON to simulate memory poisoning with UFFD | expand |
On Thu, Jul 06, 2023 at 03:50:29PM -0700, Axel Rasmussen wrote: > diff --git a/mm/madvise.c b/mm/madvise.c > index 886f06066622..59e954586e2a 100644 > --- a/mm/madvise.c > +++ b/mm/madvise.c > @@ -660,7 +660,7 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr, > free_swap_and_cache(entry); > pte_clear_not_present_full(mm, addr, pte, tlb->fullmm); > } else if (is_hwpoison_entry(entry) || > - is_swapin_error_entry(entry)) { > + is_error_swp_entry(entry)) { is_error_swp_entry() made me think whether we should call this marker as ERROR at all - it's too generic, is_error_swp_entry() can be easily read as "this swap entry is not legal". Can we come up with a less generic one? PTE_MARKER_PAGE_POISONED / PTE_MARKER_POISONED / PTE_MARKER_PAGE_ERR / ...? We do use poisoned only in real bad physical pages before, but I think we can still keep using it when applying it to a pte. I think it's fine to call a pte poisoned if it's not legal to access, just like a poisoned page. > pte_clear_not_present_full(mm, addr, pte, tlb->fullmm); > } > continue; The code change across the whole patch look good to me otherwise, thanks.
On Thu, Jul 06, 2023 at 03:50:31PM -0700, Axel Rasmussen wrote: > This code is already duplicated twice, and UFFDIO_POISON will do the > same check a third time. So, it's worth extracting into a helper to save > repetitive lines of code. > > Signed-off-by: Axel Rasmussen <axelrasmussen@google.com> Reviewed-by: Peter Xu <peterx@redhat.com>
On Thu, Jul 06, 2023 at 03:50:33PM -0700, Axel Rasmussen wrote: > The behavior here is the same as it is for anon/shmem. This is done > separately because hugetlb pte marker handling is a bit different. > > Signed-off-by: Axel Rasmussen <axelrasmussen@google.com> Acked-by: Peter Xu <peterx@redhat.com>
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,