Re: [RFC PATCH 00/10] mm/swap, PM: hibernate: improve image slot allocation and I/O

[Youngjun Park <her0gyugyu@gmail.com>]

> Hi Youngjun
>
> Thanks for the patch!

Hi Kairui,

Sorry for the late reply. I was away on a family trip.

Some of your questions and points need more thought, so I'll follow up
on those once I've reached a conclusion.

> Splitting out the hibernation specific part out of swapfile.c
> looks a good idea to me. We can then compile that file conditionally
> rather than having a huge ifdef block in swapfile.c.  Would be nice
> to get more feedback from hibnernation side.

Ack, I will keep going that way. :)

> That's pretty nice indeed. But I'm a bit concerned about having a
> specific cluster isolation path for hibernation.
>
> Will it be cleaner to provide a more generic cluster sized
> allocation (PMD sized) so common swap can benefit too?

I am still thinking about it. For now I am not sure what common swap
would gain. The gain would be mostly cleanup, e.g. the allocator would
no longer treat an allocation without a folio as hibernation.

> And I'm not sure is IO batching working properly before?

It depended on the case! Before this series the image is written with
a bio per page, and the block layer merges them back only while the
slots are contiguous. They are not always contiguous. Two cases come
to mind.

  1. The image shares the per-CPU cluster with other swap-outs on that
     CPU, so their slots can fall in between.
  2. When nonfull clusters have holes, the image fills the holes first.

When these happen often, batching does not work well.

And plus, on this RFC I manually collect bio before submit.
batch will work well from this patch as I think.

> How much performance gain is due to the cluster sized IO?

By cluster sized IO, do you mean the batched I/O of patches 7 and 8?
Sometimes it is cluster same sized, small sized and bigger sized(little situation maybe).

If so, allocator + bio cut the write time by 18 to 25% and the read
time by 11 to 20% in the cover's tests. The large contiguous I/O is
also friendlier to flash as I think

> But if other appraoches won't work or hibernation is really
> special, and we can keep all the hibernation tricky clean and
> simple in just one place, maybe it's not too bad.

Since you prefer the common swap code, I will try it in the existing
allocation path and compare it with the current swap_hibernate.c
approach. I will come back on this with the next series.

> > Note.  [3] gives hibernation slots their own swap table entry, keeps
> > readahead off them, and frees them by offset alone.  The single slot
> > path of patch 5 builds on that.
>
> Nice, maybe that series need a refresh to get merged first.

Yes, I will refresh and resend it soon. :)

> > 2. Whether the reservation in patches 9 and 10 is worth keeping.  It
> >    makes sure the image gets contiguous slots when swap has room to
> >    spare.
> >
> > 3. A block device of its own for hibernation instead of swap.  Not
> >    taken for now.  Sharing one device keeps the spare space useful, the
> >    existing infrastructure stays, and the ideas above give much the same
> >    effect.
>
> So is the idea for 2 and 3 here to make sure hibernation always success by
> avoid the allocator using too much for common swap?
>
> We only want one of them I think, and we need to be careful here to not
> make the maintainance messup by adding too many knobs...

Yes, that's right. Both keep space for the image that normal swap
cannot use, so hibernation has room and gets it contiguous. I agree we
only want one. 3 looks like too much to me, so I would like comments
on 2.

> > 4. Whether the extent tree can go.  A normal hibernation never walks
> >    it, the swap state comes back as it was at the snapshot.  It is only
> >    walked to free the slots after an error or a wake from hybrid sleep.
> >    With the slots marked in the swap table [3] and taken as whole
> >    clusters, a free could find them without it.
>
> The extent tree is not a hibernation issue right? At least for block based
> swap the extent tree is useless (only one node). I think swap_ops can be
> used to make this limited to certain swap_ops (e.g. file swap ops).

To clarify, item 4 is about hibernation's own tree, swsusp_extents in
kernel/power/swap.c, not the swap device's extent tree. It records the
image's slots so they can be freed later.

> And maybe, the swap_ops can provide some interface for hibenation usage to
> make things cleaner?

I need to think more about swap_ops here. Nothing concrete comes to
mind yet.

> > 5. Whether SNAPSHOT_ALLOC_SWAP_PAGE should refuse a request made before
> >    storage is suspended.  Such a request gets single slots from the
> >    normal allocator today, and s2disk only asks after
> >    SNAPSHOT_CREATE_IMAGE anyway.
>
> Is that a even a right thing to do during hibernation?

Sorry, I don't quite get the question (intention). Could you clarify it?

> > 6. Two cases are not measured yet.  A device with both a shuffled free
> >    list and partly used clusters.  An image bigger than the free
> >    clusters, so part of it comes from nonfull and frag clusters.
>
> I think that's fine, free cluster shuffle should not effect the
> performance much as 2M is a pretty big IO unit. For the
> fragmentation batching IO should be very helpful.

You are right. I listed it only because it is the worst case for the
long runs this RFC builds. For the I/O, cluster sized runs are enough.
Longer runs mainly help the allocator, which can then hand out more
slots at once.

Thanks!
Youngjun Park