On Mon, Sep 21, 2020 at 2:18 PM Peter Xu wrote: > > There's one special path for copy_one_pte() with swap entries, in which > add_swap_count_continuation(GFP_ATOMIC) might fail. In that case we'll return > the swp_entry_t so that the caller will release the locks and redo the same > thing with GFP_KERNEL. > > It's confusing when copy_one_pte() must return a swp_entry_t (even if all the > ptes are non-swap entries). More importantly, we face other requirement to > extend this "we need to do something else, but without the locks held" case. > > Rework the return value into something easier to understand, as defined in enum > copy_mm_ret. We'll pass the swp_entry_t back using the newly introduced union > copy_mm_data parameter. Ok, I'm reading this series, and I do hate this. And I think it's unnecessary. There's a very simple way to avoid this all: split out the "!pte_present(pte)" case from the function entirely. That actually makes the code much more legible: that non-present case is very different, and it's also unlikely() and causes deeper indentation etc. Because it's unlikely, it probably also shouldn't be inline. That unlikely case is also why when then have that special "out_set_pte" label, which should just go away and be copied into the (now uninlined) function. Once that re-organization has been done, the second step is to then just move the "pte_present()" check into the caller, and suddenly all the ugly return value games will go entirely away. I'm attaching the two patches that do this here, but I do want to note how that first patch is much more legible with "--ignore-all-space", and then you really see that the diff is a _pure_ code movement thing. Otherwise it looks like it's doing a big change. Comments? NOTE! The intent here is that now we can easily add new argument (a pre-allocated page or NULL) and a return value to "copy_present_page()": it can return "I needed a temporary page but you hadn't allocated one yet" or "I used up the temporary page you gave me" or "all good, keep the temporary page around for the future". But these two patches are very intentionally meant to be just "this clearly changes NO semantics at all". Linus