Re: [PATCH v2] x86/hibernate: Use fixmap for saving unmapped pages

[Edgecombe, Rick P]

On Thu, 2023-01-12 at 15:15 +0100, Rafael J. Wysocki wrote:
> > > 
> > > I don't think the above is needed.  The code using this function
> > > cannot be preempted anyway AFAICS.
> > 
> > The reason I thought it was useful was because this function is now
> > defined in a header. Someone else might decide to use it. Does it
> > seem
> > more useful?
> 
> Well, it is exposed now, but only in order to allow the __weak
> function to be overridden.  I don't think it is logically valid to
> use
> it anywhere beyond its original call site.
> 
> To make that clear, I would call it something hibernation-specific,
> like hibernate_copy_unmapped_page() and I would add a kerneldoc
> comment to it to describe its intended use.

Ok, I'll change the name, that makes sense.

On the warning, ok, I'll drop it. But to me the code stand out as
questionable with the PTE change and only the local TLB flush. It's a
bit of a comment as code on a rare path.

> 
> Furthermore, I'm not sure about the new code layout.
> 
> Personally, I would prefer hibernate_map_page() and
> hibernate_unmap_page() to be turned into __weak functions and
> possibly
> overridden by the arch code, which would allow the amount of changes
> to be reduced and do_copy_page() wouldn't need to be moved into the
> header any more.

Currently hibernate_map_page() maps the page on the direct map and
doesn't return anything. This new code effectively creates a readable
alias in the fixmap. So it would have to return an address to use so
the core hibernate code would know where to copy from. Then it would
have to pass it back into hibernate_unmap_page() for the arch to decide
what to do to clean it up. I think it would be more complicated.

There is also already multiple paths in hibernate_map_page() that would
have to be duplicated in the arch versions.

So I see the idea, but I'm not sure it ends up better. Can we leave
this one?

Thanks,
Rick