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