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.