Re: [PATCH 1/4] selftests/mm/uffd: Refactor non-composite global vars into struct
On Sat, 3 May 2025 23:46:26 +0530 Ujwal Kundur wrote: > Thanks for the review and testing! > > I'll push a V2 with the indentation fixes soon. > > > this deletes the global vars before it deletes > > the references to them. That's gonna be a real pain for bisections, > > please can you restructure the series to prevent intervening build > > failures? > > (i.e. if I apply just 1/4, the selftests don't compile). > > Could you help me understand this better? The selftests don't compile > either way if both 1 and 2/3/4 aren't applied at the same time. > At present uffd-stress, uffd-unit-tests and uffd-wp-mremap compile successfuly. After adding your [1/4] patch they do not. This is undesirable because such an inter-series build breakage makes it harder for people to perform git-bisect regression searches - if the bisection point lands within this series, their build fails. Probably this isn't very important in selftests/mm. One could perhaps change [1/4] to provide back-compatibility defines such as #define test_uffdio_wp gopts->test_uffdio_wp then remove those at the end of the series. But this is just too much fuss and will probably cause other issues I suggest you convert this series into a single patch.
Re: [PATCH 1/4] selftests/mm/uffd: Refactor non-composite global vars into struct
Thanks for the review and testing! I'll push a V2 with the indentation fixes soon. > this deletes the global vars before it deletes > the references to them. That's gonna be a real pain for bisections, > please can you restructure the series to prevent intervening build > failures? > (i.e. if I apply just 1/4, the selftests don't compile). Could you help me understand this better? The selftests don't compile either way if both 1 and 2/3/4 aren't applied at the same time. --- Ujwal.
Re: [PATCH 1/4] selftests/mm/uffd: Refactor non-composite global vars into struct
On Fri, 2 May 2025 at 14:16, Brendan Jackman wrote: > Little bit of indentation mess here > > I don't understand this code but since I was hurt by these global > variables I had a look over the diff anyway, it LGTM (aside from the > formatting nits), thanks. > > Reviewed-by: Brendan Jackman Wait sorry, actually, this deletes the global vars before it deletes the references to them. That's gonna be a real pain for bisections, please can you restructure the series to prevent intervening build failures? (I.e. if I apply just 1/4, the selftests don't compile).
Re: [PATCH 1/4] selftests/mm/uffd: Refactor non-composite global vars into struct
On Thu May 1, 2025 at 4:38 PM UTC, Ujwal Kundur wrote:
> + * count is placed in the page after pthread_mutex_t naturally aligned
> + * to avoid non alignment faults on non-x86 archs.
> + */
> +volatile unsigned long long *area_count(
> + char *area, unsigned long nr,
> + uffd_global_test_opts_t *gopts)
> +{
> + return (volatile unsigned long long *)
> + ((unsigned long) (area + nr * gopts->page_size +
> + sizeof(pthread_mutex_t) +
> + sizeof(unsigned long long) - 1) &
> + ~(unsigned long)(sizeof(unsigned long long) - 1));
Stray space after the minus there
> -static void retry_copy_page(int ufd, struct uffdio_copy *uffdio_copy,
> +static void retry_copy_page(uffd_global_test_opts_t *gopts, struct
> uffdio_copy *uffdio_copy,
> unsigned long offset)
> {
> - uffd_test_ops->alias_mapping(&uffdio_copy->dst,
> + uffd_test_ops->alias_mapping(gopts,
> + &uffdio_copy->dst,
Little bit of indentation mess here
I don't understand this code but since I was hurt by these global
variables I had a look over the diff anyway, it LGTM (aside from the
formatting nits), thanks.
Reviewed-by: Brendan Jackman
