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.
