All of lore.kernel.org
 help / color / mirror / Atom feed
From: Evan Green <evgreen@chromium.org>
To: David Hildenbrand <david@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	linux-api@vger.kernel.org, Michal Hocko <mhocko@suse.com>,
	Pavel Machek <pavel@ucw.cz>, Alex Shi <alexs@kernel.org>,
	Alistair Popple <apopple@nvidia.com>,
	Johannes Weiner <hannes@cmpxchg.org>,
	Joonsoo Kim <iamjoonsoo.kim@lge.com>,
	"Matthew Wilcox (Oracle)" <willy@infradead.org>,
	Miaohe Lin <linmiaohe@huawei.com>,
	Minchan Kim <minchan@kernel.org>,
	Suren Baghdasaryan <surenb@google.com>,
	Vlastimil Babka <vbabka@suse.cz>,
	LKML <linux-kernel@vger.kernel.org>,
	linux-mm@kvack.org
Subject: Re: [PATCH v3] mm: Enable suspend-only swap spaces
Date: Fri, 23 Jul 2021 10:42:51 -0700	[thread overview]
Message-ID: <CAE=gft4+tw2Lh_aVm1-K0P12Lb5byh4Nv8nGk_qQVQ8MAiTnRw@mail.gmail.com> (raw)
In-Reply-To: <3c46df04-abf4-4bcb-b9cf-430bb1d15b07@redhat.com>

On Thu, Jul 22, 2021 at 11:58 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 22.07.21 20:00, Evan Green wrote:
> > On Thu, Jul 22, 2021 at 12:12 AM David Hildenbrand <david@redhat.com> wrote:
> >>
> >> On 21.07.21 23:40, Evan Green wrote:
> >>> Currently it's not possible to enable hibernation without also enabling
> >>> generic swap for a given swap area. These two use cases are not the
> >>> same. For example there may be users who want to enable hibernation,
> >>> but whose drives don't have the write endurance for generic swap
> >>> activities. Swap and hibernate also have different security/integrity
> >>> requirements, prompting folks to possibly set up something like block-level
> >>> integrity for swap and image-level integrity for hibernate. Keeping swap
> >>> and hibernate separate in these cases becomes not just a matter of
> >>> preference, but correctness.
> >>>
> >>> Add a new SWAP_FLAG_NOSWAP that adds a swap region but refuses to allow
> >>> generic swapping to it. This region can still be wired up for use in
> >>> suspend-to-disk activities, but will never have regular pages swapped to
> >>> it. This flag will be passed in by utilities like swapon(8), usage would
> >>> probably look something like: swapon -o noswap /dev/sda2.
> >>
> >> Just a minor comment, I'd call it rather SWAP_FLAG_HIBERNATE_ONLY and
> >> SWAP_FLAG_HIBERNATE_ONLY -- that calls the child by its name.
> >
> > I went back and forth on this too. It seemed pretty close to toss-up
> > to me. I went with NOSWAP ultimately because it seemed more closely
> > tied to what the flag was actually doing, rather than building in my
> > one expected use case into the name. In some world years from now
> > where either hibernate has diverged, been deleted, or maybe some new
> > usage has been invented for swap space, the NOSWAP name felt like it
> > had a better chance of holding up. The argument is weak though, as
> > these features are pretty well cast in stone, and the likelihood of
> > any of those outcomes seems low. I can change it if you feel strongly,
> > but would probably keep it as-is otherwise.
>
> Just imagine technology Z popping up and using also the swap
> infrastructure. What would be the semantics of NOSWAP? With
> HIBERNATE_ONLY it's clear -- enable that device only for hibernation,
> nothing else.
>
> But you raise a good point: if hibernation isn't even possible in a
> configuration (e.g., not configured into the kernel), we should simply
> reject that flag. So if hibernation would vanish at some point
> completely from the system, it would all be handled accordingly.
>
> That would result in quite a consistent definition of
> SWAP_FLAG_HIBERNATE_ONLY IMHO.
>
> Makes sense?

Ok, I'll change the name, and reject the flag if hibernation is not enabled.

>
> >
> >>
> >> I think some other flags might not apply with that new flag set, right?
> >> For example, does SWAP_FLAG_DISCARD_ONCE or SWP_AREA_DISCARD still have
> >> any meaning with the new flag being set?
> >>
> >> We should most probably disallow enabling any flag that doesn't make any
> >> sense in combination.
> >
> > Good point, I can send a followup patch for that. From my reading
>
> I'd actually enjoy if we'd have that logic in the introducing patch.

Ok.

>
> > SWAP_FLAG_DISCARD and SWAP_FLAG_DISCARD_ONCE are still valid, since
> > the discard can be run at swapon() time. SWAP_FLAG_PREFER (specifying
> > the priority) doesn't make sense, and SWAP_FLAG_DISCARD_PAGES never
> > kicks in because it's called at the cluster level. Hm, that sort of
> > seems like a bug that freed hibernate swap doesn't get discarded. I
> > can disallow it now as unsupported, but might send a patch to fix it
> > later.
>
> Might be worth fixing, indeed.
>
> >
> >>
> >> Apart from that, I'd love to see a comment in here why the workaround
> >> suggested by Michal isn't feasible -- essentially a summary of what we
> >> discussed.
> >
> > Ah sorry, I had tried to clarify that in the commit text, but didn't
> > explicitly address the workaround. To summarize, the workaround keeps
> > generic swap out of your hibernate region... until hibernate time. But
> > once hibernate starts, a lot of swapping tends to happen when the
> > hiber-image is allocated. At this point the hibernate region is
> > eligible for general swap even with the workaround. The reasons I gave
> > for wanting to exclusively steer swap and hibernate are SSD write
> > wearing, different integrity solutions for swap vs hibernate, and our
> > own security changes that no-op out the swapon/swapoff syscalls after
> > init.
> >
>
> That would be nice to have in the patch description :)

Sure, I'll add that as well and send out a v4 shortly.
-Evan

  reply	other threads:[~2021-07-23 17:43 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-21 21:40 Evan Green
2021-07-21 22:09 ` Andrew Morton
2021-07-22 17:16   ` Evan Green
2021-07-22 17:16     ` Evan Green
2021-07-22  7:12 ` David Hildenbrand
2021-07-22 18:00   ` Evan Green
2021-07-22 18:00     ` Evan Green
2021-07-23  6:58     ` David Hildenbrand
2021-07-23 17:42       ` Evan Green [this message]
2021-07-23 17:42         ` Evan Green

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAE=gft4+tw2Lh_aVm1-K0P12Lb5byh4Nv8nGk_qQVQ8MAiTnRw@mail.gmail.com' \
    --to=evgreen@chromium.org \
    --cc=akpm@linux-foundation.org \
    --cc=alexs@kernel.org \
    --cc=apopple@nvidia.com \
    --cc=david@redhat.com \
    --cc=hannes@cmpxchg.org \
    --cc=iamjoonsoo.kim@lge.com \
    --cc=linmiaohe@huawei.com \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mhocko@suse.com \
    --cc=minchan@kernel.org \
    --cc=pavel@ucw.cz \
    --cc=surenb@google.com \
    --cc=vbabka@suse.cz \
    --cc=willy@infradead.org \
    --subject='Re: [PATCH v3] mm: Enable suspend-only swap spaces' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.