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

[Rafael J. Wysocki]

On Thu, Jan 12, 2023 at 8:15 PM Edgecombe, Rick P
<rick.p.edgecombe@xxxxxxxxx> wrote:
>
> 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.

AFAICS, you only need hibernate_map_page() to return an address that
will be passed to do_copy_page() (that doesn't need to be touched
otherwise) and hibernate_unmap_page() would just do

clear_fixmap(FIX_HIBERNATE);

so it may as well take the page as the argument.

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

But only if the arch decides to override the originals.

Now, the only caller of both hibernate_map_page() and
hibernate_unmap_page() is safe_copy_page() in the "else" branch.  Your
current patch replaces that branch completely with an arch version, so
they are not going to be invoked on x86 anyway.

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

I first need to be convinced that it is indeed better.