Date:   Mon, 13 Dec 2021 08:41:44 -0800
From:   "Paul E. McKenney" <paulmck@kernel.org>
To:     Nicolas Saenz Julienne <nsaenzju@redhat.com>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        Mark Rutland <mark.rutland@arm.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
        rcu@vger.kernel.org, Peter Zijlstra <peterz@infradead.org>,
        mtosatti <mtosatti@redhat.com>, frederic <frederic@kernel.org>,
        Jonathan Corbet <corbet@lwn.net>
Subject: Re: [PATCH v2] Documentation: Fill the gaps about entry/noinstr
 constraints
Message-ID: <20211213164144.GN641268@paulmck-ThinkPad-P17-Gen-1>
Reply-To: paulmck@kernel.org
References: <875ys9dacq.ffs@tglx>
 <20211130091356.7336e277@gandalf.local.home>
 <878rx5b7i5.ffs@tglx>
 <YadU1aSE6/0yGWny@FVFF77S0Q05N>
 <87v9088a5q.ffs@tglx>
 <Yae9tbtZW5mjcBVt@FVFF77S0Q05N>
 <87ee6w83yw.ffs@tglx>
 <87bl2083mu.ffs@tglx>
 <20211203200808.GT641268@paulmck-ThinkPad-P17-Gen-1>
 <09e4a37b390479baa4e0947670d49c44d58c2855.camel@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <09e4a37b390479baa4e0947670d49c44d58c2855.camel@redhat.com>
Precedence: bulk

On Mon, Dec 13, 2021 at 11:36:56AM +0100, Nicolas Saenz Julienne wrote:
> Hi All,
> now that this is good shape I'm taking over Thomas and preparing v3.
> 
> Paul, I introduced most (if not all) your paragraph corrections. Some questions
> below.

Thank you for taking this on!

> On Fri, 2021-12-03 at 12:08 -0800, Paul E. McKenney wrote:
> > > +The update order depends on the transition type and is explained below in
> > > +the transition type sections.
> > @@@
> 
> Sorry, I'm not 100% sure I get what you meant by this. Maybe introducing some
> sort of link?

What this sentence is trying to get across is that there are different
orders of state updates, depending on the type of transition.  It would
be good to link to the following sections, if that can be done reasonably.

> [...]
> 
> > > +syscall_exit_to_user_mode() handles all work which needs to be done before
> > > +returning to user space like tracing, audit, signals, task work etc. After
> > > +that it invokes exit_to_user_mode() which again handles the state
> > > +transition in the reverse order:
> > > +
> > > +  * Tracing
> > > +  * RCU / Context tracking
> > > +  * Lockdep
> > > +
> > > +syscall_enter_from_user_mode() and syscall_exit_to_user_mode() are also
> > > +available as fine grained subfunctions in cases where the architecture code
> > > +has to do extra work between the various steps. In such cases it has to
> > > +ensure that enter_from_user_mode() is called first on entry and
> > > +exit_to_user_mode() is called last on exit.
> > 
> > !!! Here I have a question.  Can calls to enter_from_user_mode()
> > be nested?  RCU is OK with this, but I am not so sure that everything
> > else is.  If nesting is prohibited, this paragraph should explicitly
> > say that.  If nesting is theoretically possible, but should be avoided,
> > it would be good to say that as well.  (Otherwise "It looks like it
> > might work, so let's go for it!")
> 
> 
> In __enter_from_user_mode() I see:
> 
> 	CT_WARN_ON(ct_state() != CONTEXT_USER);
> 
> IIUC this signals that a nested syscall entry isn't expected from CT's point of
> view. I remember reading through RCU's dyntick code that the rationale for
> nesting in the syscall path was half interrupts (or upcalls). I did some
> research, but couldn't find an example of this. Is this something we can
> discard as an old technique not used anymore?

Indeed, there are thankfully no more half interrupts.

> On the other hand, interrupts are prone to nesting:
>  - Weird interrupt handlers that re-enable interrupts
>  - NMIs interrupting Hard IRQ context
>  - NMIs interrupting NMIs

Plus there are odd cases where (from RCU's viewpoint) an interrupt can
happen within an NMI handler.

> Please let me know if I'm off-base, but I think the topic of nesting is worth a
> sentence or two in each section.

I completely agree.  We should be clear on what nesting is permitted and
not.

> [...]
> 
> > > +Interrupts and regular exceptions
> > > +---------------------------------
> > > +
> > > +Interrupts entry and exit handling is slightly more complex than syscalls
> > > +and KVM transitions.
> > > +
> > > +If an interrupt is raised while the CPU executes in user space, the entry
> > > +and exit handling is exactly the same as for syscalls.
> > > +
> > > +If the interrupt is raised while the CPU executes in kernel space the entry
> > > +and exit handling is slightly different. RCU state is only updated when the
> > > +interrupt was raised in context of the CPU's idle task because that's the
> > > +only kernel context where RCU can be not watching on NOHZ enabled kernels.
> > > +Lockdep and tracing have to be updated unconditionally.
> > 
> > !!! You lost me on this one.  Does that second-to-last sentence instead
> > want to end something like this?  "... where RCU will not be watching
> > when running on non-nohz_full CPUs."
> 
> The paragraph covers IRQ entry from kernel space. In that context RCU is only
> shut-off during idle. That only happens on a NOHZ-enabled kernel, be it
> NO_HZ_IDLE or NO_HZ_FULL.

OK, good.  So the RCU-not-watching case is on a nohz-full CPU.

> I'll try to reword it a bit so it's more explicit.

Sounds good!  And thank you again for taking this on!

							Thanx, Paul

> Thanks!
> 
> -- 
> Nicol?s S?enz
>