From: Ira Weiny <ira.weiny@intel.com> To: Thomas Gleixner <tglx@linutronix.de>, "Paul E. McKenney" <paulmck@kernel.org> Cc: Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>, Andy Lutomirski <luto@kernel.org>, Peter Zijlstra <peterz@infradead.org>, x86@kernel.org, Dave Hansen <dave.hansen@linux.intel.com>, Andrew Morton <akpm@linux-foundation.org>, Fenghua Yu <fenghua.yu@intel.com>, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-nvdimm@lists.01.org, linux-mm@kvack.org, linux-kselftest@vger.kernel.org Subject: Re: [PATCH 06/10] x86/entry: Move nmi entry/exit into common code Date: Tue, 27 Oct 2020 00:07:50 -0700 [thread overview] Message-ID: <20201027070750.GM534324@iweiny-DESK2.sc.intel.com> (raw) In-Reply-To: <874kmk6298.fsf@nanos.tec.linutronix.de> On Fri, Oct 23, 2020 at 11:50:11PM +0200, Thomas Gleixner wrote: > On Thu, Oct 22 2020 at 15:26, ira weiny wrote: > > > From: Thomas Gleixner <tglx@linutronix.de> > > > > Lockdep state handling on NMI enter and exit is nothing specific to X86. It's > > not any different on other architectures. Also the extra state type is not > > necessary, irqentry_state_t can carry the necessary information as well. > > > > Move it to common code and extend irqentry_state_t to carry lockdep > > state. > > This lacks something like: > > [ Ira: Made the states a union as they are mutually exclusive and added > the missing kernel doc ] Fair enough. done. > > Hrm. > > > #ifndef irqentry_state > > typedef struct irqentry_state { > > - bool exit_rcu; > > + union { > > + bool exit_rcu; > > + bool lockdep; > > + }; > > } irqentry_state_t; > > #endif > > -E_NO_KERNELDOC Adding: Paul McKenney I'm happy to write something but I'm very unfamiliar with this code. So I'm getting confused what exactly exit_rcu is flagging. I can see that exit_rcu is a bad name for the state used in irqentry_nmi_[enter|exit](). Furthermore, I see why 'lockdep' is a better name. But similar lockdep handling is used in irqentry_exit() if exit_rcu is true... Given my limited knowledge; here is my proposed text: /** * struct irqentry_state - Opaque object for exception state storage * @exit_rcu: Used exclusively in the irqentry_*() calls; tracks if the * exception hit the idle task which requires special handling, * including calling rcu_irq_exit(), when the exception exits. * @lockdep: Used exclusively in the irqentry_nmi_*() calls; ensures lockdep * tracking is maintained if hardirqs were already enabled * * This opaque object is filled in by the irqentry_*_enter() functions and * should be passed back into the corresponding irqentry_*_exit() functions * when the exception is complete. * * Callers of irqentry_*_[enter|exit]() should consider this structure opaque * and all members private. Descriptions of the members are provided to aid in * the maintenance of the irqentry_*() functions. */ Perhaps Paul can enlighten me on how exit_rcu is used beyond just flagging a call to rcu_irq_exit()? Why do we call lockdep_hardirqs_off() only when in the idle task? That implies that regs_irqs_disabled() can only be false if we were in the idle task to match up the lockdep on/off calls. This does not make sense to me because why do we need the extra check for exit_rcu? I'm still trying to understand when regs_irqs_disabled() is false. } else if (!regs_irqs_disabled(regs)) { ... } else { /* * IRQ flags state is correct already. Just tell RCU if it * was not watching on entry. */ if (state.exit_rcu) rcu_irq_exit(); } Also, the comment in irqentry_enter() refers to irq_enter_from_user_mode() which does not seem to exist anymore. So I'm not sure what careful sequence it is referring to. /* * If RCU is not watching then the same careful * sequence vs. lockdep and tracing is required * as in irq_enter_from_user_mode(). */ ? Ira _______________________________________________ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
WARNING: multiple messages have this Message-ID (diff)
From: Ira Weiny <ira.weiny@intel.com> To: Thomas Gleixner <tglx@linutronix.de>, "Paul E. McKenney" <paulmck@kernel.org> Cc: Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>, Andy Lutomirski <luto@kernel.org>, Peter Zijlstra <peterz@infradead.org>, x86@kernel.org, Dave Hansen <dave.hansen@linux.intel.com>, Dan Williams <dan.j.williams@intel.com>, Andrew Morton <akpm@linux-foundation.org>, Fenghua Yu <fenghua.yu@intel.com>, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-nvdimm@lists.01.org, linux-mm@kvack.org, linux-kselftest@vger.kernel.org Subject: Re: [PATCH 06/10] x86/entry: Move nmi entry/exit into common code Date: Tue, 27 Oct 2020 00:07:50 -0700 [thread overview] Message-ID: <20201027070750.GM534324@iweiny-DESK2.sc.intel.com> (raw) In-Reply-To: <874kmk6298.fsf@nanos.tec.linutronix.de> On Fri, Oct 23, 2020 at 11:50:11PM +0200, Thomas Gleixner wrote: > On Thu, Oct 22 2020 at 15:26, ira weiny wrote: > > > From: Thomas Gleixner <tglx@linutronix.de> > > > > Lockdep state handling on NMI enter and exit is nothing specific to X86. It's > > not any different on other architectures. Also the extra state type is not > > necessary, irqentry_state_t can carry the necessary information as well. > > > > Move it to common code and extend irqentry_state_t to carry lockdep > > state. > > This lacks something like: > > [ Ira: Made the states a union as they are mutually exclusive and added > the missing kernel doc ] Fair enough. done. > > Hrm. > > > #ifndef irqentry_state > > typedef struct irqentry_state { > > - bool exit_rcu; > > + union { > > + bool exit_rcu; > > + bool lockdep; > > + }; > > } irqentry_state_t; > > #endif > > -E_NO_KERNELDOC Adding: Paul McKenney I'm happy to write something but I'm very unfamiliar with this code. So I'm getting confused what exactly exit_rcu is flagging. I can see that exit_rcu is a bad name for the state used in irqentry_nmi_[enter|exit](). Furthermore, I see why 'lockdep' is a better name. But similar lockdep handling is used in irqentry_exit() if exit_rcu is true... Given my limited knowledge; here is my proposed text: /** * struct irqentry_state - Opaque object for exception state storage * @exit_rcu: Used exclusively in the irqentry_*() calls; tracks if the * exception hit the idle task which requires special handling, * including calling rcu_irq_exit(), when the exception exits. * @lockdep: Used exclusively in the irqentry_nmi_*() calls; ensures lockdep * tracking is maintained if hardirqs were already enabled * * This opaque object is filled in by the irqentry_*_enter() functions and * should be passed back into the corresponding irqentry_*_exit() functions * when the exception is complete. * * Callers of irqentry_*_[enter|exit]() should consider this structure opaque * and all members private. Descriptions of the members are provided to aid in * the maintenance of the irqentry_*() functions. */ Perhaps Paul can enlighten me on how exit_rcu is used beyond just flagging a call to rcu_irq_exit()? Why do we call lockdep_hardirqs_off() only when in the idle task? That implies that regs_irqs_disabled() can only be false if we were in the idle task to match up the lockdep on/off calls. This does not make sense to me because why do we need the extra check for exit_rcu? I'm still trying to understand when regs_irqs_disabled() is false. } else if (!regs_irqs_disabled(regs)) { ... } else { /* * IRQ flags state is correct already. Just tell RCU if it * was not watching on entry. */ if (state.exit_rcu) rcu_irq_exit(); } Also, the comment in irqentry_enter() refers to irq_enter_from_user_mode() which does not seem to exist anymore. So I'm not sure what careful sequence it is referring to. /* * If RCU is not watching then the same careful * sequence vs. lockdep and tracing is required * as in irq_enter_from_user_mode(). */ ? Ira
next prev parent reply other threads:[~2020-10-27 7:07 UTC|newest] Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-10-22 22:26 [PATCH 00/10] PKS: Add Protection Keys Supervisor (PKS) support ira.weiny 2020-10-22 22:26 ` ira.weiny 2020-10-22 22:26 ` [PATCH 01/10] x86/pkeys: Create pkeys_common.h ira.weiny 2020-10-22 22:26 ` ira.weiny 2020-10-22 22:26 ` [PATCH 02/10] x86/fpu: Refactor arch_set_user_pkey_access() for PKS support ira.weiny 2020-10-22 22:26 ` ira.weiny 2020-10-22 22:26 ` [PATCH 03/10] x86/pks: Enable Protection Keys Supervisor (PKS) ira.weiny 2020-10-22 22:26 ` ira.weiny 2020-10-22 22:26 ` [PATCH 04/10] x86/pks: Preserve the PKRS MSR on context switch ira.weiny 2020-10-22 22:26 ` ira.weiny 2020-10-22 22:26 ` [PATCH 05/10] x86/pks: Add PKS kernel API ira.weiny 2020-10-22 22:26 ` ira.weiny 2020-10-22 22:26 ` [PATCH 06/10] x86/entry: Move nmi entry/exit into common code ira.weiny 2020-10-22 22:26 ` ira.weiny 2020-10-23 21:50 ` Thomas Gleixner 2020-10-23 21:50 ` Thomas Gleixner 2020-10-27 7:07 ` Ira Weiny [this message] 2020-10-27 7:07 ` Ira Weiny 2020-10-27 14:18 ` Thomas Gleixner 2020-10-27 14:18 ` Thomas Gleixner 2020-10-22 22:26 ` [PATCH 07/10] x86/entry: Pass irqentry_state_t by reference ira.weiny 2020-10-22 22:26 ` ira.weiny 2020-10-23 21:56 ` Thomas Gleixner 2020-10-23 21:56 ` Thomas Gleixner 2020-10-27 7:11 ` Ira Weiny 2020-10-27 7:11 ` Ira Weiny 2020-10-22 22:26 ` [PATCH 08/10] x86/entry: Preserve PKRS MSR across exceptions ira.weiny 2020-10-22 22:26 ` ira.weiny 2020-10-22 22:27 ` [PATCH 09/10] x86/fault: Report the PKRS state on fault ira.weiny 2020-10-22 22:27 ` ira.weiny 2020-10-22 22:27 ` [PATCH 10/10] x86/pks: Add PKS test code ira.weiny 2020-10-22 22:27 ` ira.weiny
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=20201027070750.GM534324@iweiny-DESK2.sc.intel.com \ --to=ira.weiny@intel.com \ --cc=akpm@linux-foundation.org \ --cc=bp@alien8.de \ --cc=dave.hansen@linux.intel.com \ --cc=fenghua.yu@intel.com \ --cc=linux-doc@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-kselftest@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=linux-nvdimm@lists.01.org \ --cc=luto@kernel.org \ --cc=mingo@redhat.com \ --cc=paulmck@kernel.org \ --cc=peterz@infradead.org \ --cc=tglx@linutronix.de \ --cc=x86@kernel.org \ /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.