From: Evan Green <email@example.com> To: David Hildenbrand <firstname.lastname@example.org> Cc: Andrew Morton <email@example.com>, firstname.lastname@example.org, Michal Hocko <email@example.com>, Pavel Machek <firstname.lastname@example.org>, Alex Shi <email@example.com>, Alistair Popple <firstname.lastname@example.org>, Johannes Weiner <email@example.com>, Joonsoo Kim <firstname.lastname@example.org>, "Matthew Wilcox (Oracle)" <email@example.com>, Miaohe Lin <firstname.lastname@example.org>, Minchan Kim <email@example.com>, Suren Baghdasaryan <firstname.lastname@example.org>, Vlastimil Babka <email@example.com>, LKML <firstname.lastname@example.org>, email@example.com 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: <firstname.lastname@example.org> On Thu, Jul 22, 2021 at 11:58 PM David Hildenbrand <email@example.com> wrote: > > On 22.07.21 20:00, Evan Green wrote: > > On Thu, Jul 22, 2021 at 12:12 AM David Hildenbrand <firstname.lastname@example.org> 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
next prev parent 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' \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --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.