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: Thu, 22 Jul 2021 11:00:56 -0700	[thread overview]
Message-ID: <CAE=gft7eY0scobDwQGq-OuFk4Ec2APFQF-4K6UVkTN-TOGwETw@mail.gmail.com> (raw)
In-Reply-To: <dd405f78-ac37-18d4-23f1-7d43507edee6@redhat.com>

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.

>
> 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
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.

>
> 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.

>
> I had a quick glimpse and nothing jumed at me, no mm/swapfile.c expert,
> though :)

Thanks David!
-Evan

>
>
>
> --
> Thanks,
>
> David / dhildenb
>

  reply	other threads:[~2021-07-22 18:01 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 [this message]
2021-07-22 18:00     ` Evan Green
2021-07-23  6:58     ` David Hildenbrand
2021-07-23 17:42       ` Evan Green
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=gft7eY0scobDwQGq-OuFk4Ec2APFQF-4K6UVkTN-TOGwETw@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.