Message-ID: <CALkFLLLfxT1pQ_ySB1NU4KXOEGLd2wB8pbhpBG2HfK3_mLOYAQ@mail.gmail.com>
Date: Mon, 19 May 2025 19:20:31 +0530
From: Ujwal Kundur <ujwal.kundur@...il.com>
To: Brendan Jackman <jackmanb@...gle.com>
Cc: akpm@...ux-foundation.org, peterx@...hat.com, shuah@...nel.org, 
	linux-mm@...ck.org, linux-kselftest@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/1] selftests/mm/uffd: Refactor non-composite global
 vars into struct

Thanks for the review and testing!

>> -static void retry_copy_page(int ufd, struct uffdio_copy *uffdio_copy,
>> -                         unsigned long offset)
>> +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,
>> -                                  uffdio_copy->len,
>> -                                  offset);
>> -     if (ioctl(ufd, UFFDIO_COPY, uffdio_copy)) {
>> +     uffd_test_ops->alias_mapping(gopts,
>> +                                                             &uffdio_copy->dst,
>> +                                                             uffdio_copy->len,
>> +                                                             offset);

> Looks like your editor got a bit excited here :D
>
> There are a few other places where this happened too, e.g. the
> are_count() declaration and there's a pthread_create_call() that's quite
> messed up.

I use vim with `:set list` turned on to display control characters and
it looked fine to me when I submitted the patch :(
Here's the output of $ cat -A uffd-common.c | grep -A 15 retry_copy_page:
(where ^I represents a tab equivalent to 4 spaces)
static void retry_copy_page(uffd_global_test_opts_t *gopts, struct
uffdio_copy *uffdio_copy,$
^I^I^I^I^I^I^Iunsigned long offset)$
{$
^Iuffd_test_ops->alias_mapping(gopts,$
^I^I^I^I^I^I^I^I&uffdio_copy->dst,$
^I^I^I^I^I^I^I^Iuffdio_copy->len,$
^I^I^I^I^I^I^I^Ioffset);$
^Iif (ioctl(gopts->uffd, UFFDIO_COPY, uffdio_copy)) {$
^I^I/* real retval in ufdio_copy.copy */$
^I^Iif (uffdio_copy->copy != -EEXIST)$
^I^I^Ierr("UFFDIO_COPY retry error: %"PRId64,$
^I^I^I    (int64_t)uffdio_copy->copy);$
^I} else {$
^I^Ierr("UFFDIO_COPY retry unexpected: %"PRId64,$
^I^I    (int64_t)uffdio_copy->copy);$
^I}$

I checked this against master:
$ cat -A uffd-common.c | grep -A 15 retry_copy_page
static void retry_copy_page(int ufd, struct uffdio_copy *uffdio_copy,$
^I^I^I    unsigned long offset)$
{$
^Iuffd_test_ops->alias_mapping(&uffdio_copy->dst,$
^I^I^I^I     uffdio_copy->len,$
^I^I^I^I     offset);$
^Iif (ioctl(ufd, UFFDIO_COPY, uffdio_copy)) {$
^I^I/* real retval in ufdio_copy.copy */$
^I^Iif (uffdio_copy->copy != -EEXIST)$
^I^I^Ierr("UFFDIO_COPY retry error: %"PRId64,$
^I^I^I    (int64_t)uffdio_copy->copy);$
^I} else {$
^I^Ierr("UFFDIO_COPY retry unexpected: %"PRId64,$
^I^I    (int64_t)uffdio_copy->copy);$
^I}$
}$

and there seem to be spaces mixed in earlier causing the formatting
issues. It looks okay to me after I pulled in the patch to my local
repo.

> Unfortunately I don't know of any tool that can find/fix these issues
> automatically without also messing up the whole file. Could you just
> do a visual skim and fix what you can spot?

Yeah, I ran clang-format and it's playing by its own rules -- do we
have a standard .clang-format file?
Please let me know if there's a better way to find/fix whitespace
formatting issues, I found a few inconsistencies which I will fix.

>  static void sigalrm(int sig)
>  {
>       if (sig != SIGALRM)
>               abort();
> -     test_uffdio_copy_eexist = true;
> +     // TODO: Set this without access to global vars
> +     // gopts->test_uffdio_copy_eexist = true;
>
> Did you mean to leave this like that?

Nice catch! I was hoping someone would suggest a better way to handle
this. As far as I can tell, this was written the way it was as a
consequence of using global variables.
I think this sets up retries for copies in a racy way using alarm(2) /
signal(2), not sure how effective that has proven to be. I believe the
only way to keep this functionality would be to introduce some async
action (libev, libuv, etc.), but is that required?

> +                     /* TODO: remove this global var.. it's so ugly */
>
> That's done :)

Oh I misunderstood that as "remove nr_parallel" which is not the case, will fix.

> @@ -1734,6 +1737,27 @@ int main(int argc, char *argv[])
>               }
>               for (j = 0; j < n_mems; j++) {
>                       mem_type = &mem_types[j];
> +
> +                     // Initialize global test options
>
> Wrong comment style here

Will fix

I'm not sure about the protocol here, should I roll a PATCH v4 or a
new patch entirely?
Planning on addressing another TODO for dynamically setting the
BASE_PMD_ADDR, I can roll the fixes as part of that patch if required.

Powered by blists - more mailing lists