From: Petr Mladek <pmladek@suse.com> To: Valentin Schneider <vschneid@redhat.com> Cc: "Eric W. Biederman" <ebiederm@xmission.com>, linux-kernel@vger.kernel.org, kexec@lists.infradead.org, linux-rt-users@vger.kernel.org, Arnd Bergmann <arnd@arndb.de>, Thomas Gleixner <tglx@linutronix.de>, Sebastian Andrzej Siewior <bigeasy@linutronix.de>, Juri Lelli <jlelli@redhat.com>, "Luis Claudio R. Goncalves" <lgoncalv@redhat.com>, Andrew Morton <akpm@linux-foundation.org>, Vivek Goyal <vgoyal@redhat.com> Subject: Re: [PATCH v2] panic, kexec: Make __crash_kexec() NMI safe Date: Wed, 29 Jun 2022 13:55:39 +0200 [thread overview] Message-ID: <20220629115539.GB12720@pathway.suse.cz> (raw) In-Reply-To: <xhsmhmtdw66cr.mognet@vschneid.remote.csb> On Tue 2022-06-28 18:33:08, Valentin Schneider wrote: > On 27/06/22 13:42, Valentin Schneider wrote: > > On 25/06/22 12:04, Eric W. Biederman wrote: > >> At this point I recommend going back to being ``unconventional'' with > >> the kexec locking and effectively reverting commit 8c5a1cf0ad3a ("kexec: > >> use a mutex for locking rather than xchg()"). > >> > >> That would also mean that we don't have to worry about the lockdep code > >> doing something weird in the future and breaking kexec. > >> > >> Your change starting to is atomic_cmpxchng is most halfway to a revert > >> of commit 8c5a1cf0ad3a ("kexec: use a mutex for locking rather than > >> xchg()"). So we might as well go the whole way and just document that > >> the kexec on panic code can not use conventional kernel locking > >> primitives and has to dig deep and build it's own. At which point it > >> makes no sense for the rest of the kexec code to use anything different. > >> > > > > Hm, I'm a bit torn about that one, ideally I'd prefer to keep "homegrown" > > locking primitives to just where they are needed (loading & kexec'ing), but > > I'm also not immensely fond of the "hybrid" mutex+cmpxchg approach. > > > > 8c5a1cf0ad3a ("kexec: use a mutex for locking rather than xchg()") was > straightforward enough because it turned > > if (xchg(&lock, 1)) > return -EBUSY; > > into > > if (!mutex_trylock(&lock)) > return -EBUSY; > > Now, most of the kexec_mutex uses are trylocks, except for: > - crash_get_memory_size() > - crash_shrink_memory() > > I really don't want to go down the route of turning those into cmpxchg > try-loops, would it be acceptable to make those use trylocks (i.e. return > -EBUSY if the cmpxchg fails)? IMHO, -EBUSY is acceptable for both crash_get_memory_size() and crash_shrink_memory(). They are used in the sysfs interface. > Otherwise, we keep the mutexes for functions like those which go nowhere > near an NMI. If we go this way then I would hide the locking into some wrappers, like crash_kexec_trylock()/unlock() that would do both mutex and xchg. The xchg part might be hidden in a separate wrapper __crash_kexec_trylock()/unlock() or crash_kexec_atomic_trylock()/unlock(). Best Regards, Petr
WARNING: multiple messages have this Message-ID (diff)
From: Petr Mladek <pmladek@suse.com> To: Valentin Schneider <vschneid@redhat.com> Cc: "Eric W. Biederman" <ebiederm@xmission.com>, linux-kernel@vger.kernel.org, kexec@lists.infradead.org, linux-rt-users@vger.kernel.org, Arnd Bergmann <arnd@arndb.de>, Thomas Gleixner <tglx@linutronix.de>, Sebastian Andrzej Siewior <bigeasy@linutronix.de>, Juri Lelli <jlelli@redhat.com>, "Luis Claudio R. Goncalves" <lgoncalv@redhat.com>, Andrew Morton <akpm@linux-foundation.org>, Vivek Goyal <vgoyal@redhat.com> Subject: Re: [PATCH v2] panic, kexec: Make __crash_kexec() NMI safe Date: Wed, 29 Jun 2022 13:55:39 +0200 [thread overview] Message-ID: <20220629115539.GB12720@pathway.suse.cz> (raw) In-Reply-To: <xhsmhmtdw66cr.mognet@vschneid.remote.csb> On Tue 2022-06-28 18:33:08, Valentin Schneider wrote: > On 27/06/22 13:42, Valentin Schneider wrote: > > On 25/06/22 12:04, Eric W. Biederman wrote: > >> At this point I recommend going back to being ``unconventional'' with > >> the kexec locking and effectively reverting commit 8c5a1cf0ad3a ("kexec: > >> use a mutex for locking rather than xchg()"). > >> > >> That would also mean that we don't have to worry about the lockdep code > >> doing something weird in the future and breaking kexec. > >> > >> Your change starting to is atomic_cmpxchng is most halfway to a revert > >> of commit 8c5a1cf0ad3a ("kexec: use a mutex for locking rather than > >> xchg()"). So we might as well go the whole way and just document that > >> the kexec on panic code can not use conventional kernel locking > >> primitives and has to dig deep and build it's own. At which point it > >> makes no sense for the rest of the kexec code to use anything different. > >> > > > > Hm, I'm a bit torn about that one, ideally I'd prefer to keep "homegrown" > > locking primitives to just where they are needed (loading & kexec'ing), but > > I'm also not immensely fond of the "hybrid" mutex+cmpxchg approach. > > > > 8c5a1cf0ad3a ("kexec: use a mutex for locking rather than xchg()") was > straightforward enough because it turned > > if (xchg(&lock, 1)) > return -EBUSY; > > into > > if (!mutex_trylock(&lock)) > return -EBUSY; > > Now, most of the kexec_mutex uses are trylocks, except for: > - crash_get_memory_size() > - crash_shrink_memory() > > I really don't want to go down the route of turning those into cmpxchg > try-loops, would it be acceptable to make those use trylocks (i.e. return > -EBUSY if the cmpxchg fails)? IMHO, -EBUSY is acceptable for both crash_get_memory_size() and crash_shrink_memory(). They are used in the sysfs interface. > Otherwise, we keep the mutexes for functions like those which go nowhere > near an NMI. If we go this way then I would hide the locking into some wrappers, like crash_kexec_trylock()/unlock() that would do both mutex and xchg. The xchg part might be hidden in a separate wrapper __crash_kexec_trylock()/unlock() or crash_kexec_atomic_trylock()/unlock(). Best Regards, Petr _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
next prev parent reply other threads:[~2022-06-29 11:55 UTC|newest] Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-06-20 11:15 [PATCH v2] panic, kexec: Make __crash_kexec() NMI safe Valentin Schneider 2022-06-20 11:15 ` Valentin Schneider 2022-06-23 9:31 ` Sebastian Andrzej Siewior 2022-06-23 9:31 ` Sebastian Andrzej Siewior 2022-06-23 11:39 ` Valentin Schneider 2022-06-23 11:39 ` Valentin Schneider 2022-06-23 13:35 ` Sebastian Andrzej Siewior 2022-06-23 13:35 ` Sebastian Andrzej Siewior 2022-06-24 1:30 ` Baoquan He 2022-06-24 1:30 ` Baoquan He 2022-06-24 13:37 ` Valentin Schneider 2022-06-24 13:37 ` Valentin Schneider 2022-06-26 10:37 ` Baoquan He 2022-06-26 10:37 ` Baoquan He 2022-06-26 10:45 ` Baoquan He 2022-06-26 10:45 ` Baoquan He 2022-06-25 17:04 ` Eric W. Biederman 2022-06-25 17:04 ` Eric W. Biederman 2022-06-27 12:42 ` Valentin Schneider 2022-06-27 12:42 ` Valentin Schneider 2022-06-28 17:33 ` Valentin Schneider 2022-06-28 17:33 ` Valentin Schneider 2022-06-29 11:55 ` Petr Mladek [this message] 2022-06-29 11:55 ` Petr Mladek 2022-06-29 12:23 ` Valentin Schneider 2022-06-29 12:23 ` Valentin Schneider
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=20220629115539.GB12720@pathway.suse.cz \ --to=pmladek@suse.com \ --cc=akpm@linux-foundation.org \ --cc=arnd@arndb.de \ --cc=bigeasy@linutronix.de \ --cc=ebiederm@xmission.com \ --cc=jlelli@redhat.com \ --cc=kexec@lists.infradead.org \ --cc=lgoncalv@redhat.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-rt-users@vger.kernel.org \ --cc=tglx@linutronix.de \ --cc=vgoyal@redhat.com \ --cc=vschneid@redhat.com \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.