Re: [PATCH v3 1/1] selftests/mm/uffd: Refactor non-composite global vars into struct

[Brendan Jackman <jackmanb@google.com>]

On Mon May 19, 2025 at 1:50 PM UTC, Ujwal Kundur wrote:
> 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)

Sounds like that's your issue - for the kernel, tab is supposed to be
as wide as 8 spaces, not 4.

>> 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.

There is a .clang-format in the tree. The issue is that (AFAIK) there's
no way to retrofit clang-format really, it has to be applied to the
entire source file so if there are pre-existing deviations from its
configuration then it will "fix" those too, creating a bunch of diff
noise.

I think we just have to do it manually. checkpatch.pl can help with some
formatting issues (and it is diff-aware) but I don't think it knows
anything about this kind of hanging indentation stuff.

>
>>  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?

I'm afraid I'm too ignorant of this code to be able to suggest something
good here. But, can we just remove the comment and plumb the gopts
through to uffd_poll_thread()->uffd_handle_page_fault()->__copy_page()?

This is not pretty but it lets us remove the global vars which is
clearly a step in the right direciton.