Received-SPF: pass (google.com: domain of linux-kernel+bounces-87293-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Thu, 29 Feb 2024 10:29:32 -0800
From: "Paul E. McKenney" <paulmck@kernel.org>
To: Joel Fernandes <joel@joelfernandes.org>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>,
	Steven Rostedt <rostedt@goodmis.org>, Yan Zhai <yan@cloudflare.com>,
	Eric Dumazet <edumazet@google.com>,
	Network Development <netdev@vger.kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Jiri Pirko <jiri@resnulli.us>, Simon Horman <horms@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Lorenzo Bianconi <lorenzo@kernel.org>,
	Coco Li <lixiaoyan@google.com>, Wei Wang <weiwan@google.com>,
	Alexander Duyck <alexanderduyck@fb.com>,
	Hannes Frederic Sowa <hannes@stressinduktion.org>,
	LKML <linux-kernel@vger.kernel.org>, rcu@vger.kernel.org,
	bpf <bpf@vger.kernel.org>, kernel-team <kernel-team@cloudflare.com>,
	Mark Rutland <mark.rutland@arm.com>
Subject: Re: [PATCH] net: raise RCU qs after each threaded NAPI poll
Message-ID: <99b2ccae-07f6-4350-9c55-25ec7ae065c0@paulmck-laptop>
Reply-To: paulmck@kernel.org
References: <55900c6a-f181-4c5c-8de2-bca640c4af3e@paulmck-laptop>
 <10FC3F5F-AA33-4F81-9EB6-87EB2D41F3EE@joelfernandes.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <10FC3F5F-AA33-4F81-9EB6-87EB2D41F3EE@joelfernandes.org>

On Thu, Feb 29, 2024 at 12:41:55PM -0500, Joel Fernandes wrote:
> > On Feb 29, 2024, at 11:57 AM, Paul E. McKenney <paulmck@kernel.org> wrote:
> > ﻿On Thu, Feb 29, 2024 at 09:21:48AM -0500, Joel Fernandes wrote:
> >>> On 2/28/2024 5:58 PM, Paul E. McKenney wrote:
> >>> On Wed, Feb 28, 2024 at 02:48:44PM -0800, Alexei Starovoitov wrote:
> >>>> On Wed, Feb 28, 2024 at 2:31 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> >>>>> 
> >>>>> On Wed, 28 Feb 2024 14:19:11 -0800
> >>>>> "Paul E. McKenney" <paulmck@kernel.org> wrote:
> >>>>> 
> >>>>>>>> 
> >>>>>>>> Well, to your initial point, cond_resched() does eventually invoke
> >>>>>>>> preempt_schedule_common(), so you are quite correct that as far as
> >>>>>>>> Tasks RCU is concerned, cond_resched() is not a quiescent state.
> >>>>>>> 
> >>>>>>> Thanks for confirming. :-)
> >>>>>> 
> >>>>>> However, given that the current Tasks RCU use cases wait for trampolines
> >>>>>> to be evacuated, Tasks RCU could make the choice that cond_resched()
> >>>>>> be a quiescent state, for example, by adjusting rcu_all_qs() and
> >>>>>> .rcu_urgent_qs accordingly.
> >>>>>> 
> >>>>>> But this seems less pressing given the chance that cond_resched() might
> >>>>>> go away in favor of lazy preemption.
> >>>>> 
> >>>>> Although cond_resched() is technically a "preemption point" and not truly a
> >>>>> voluntary schedule, I would be happy to state that it's not allowed to be
> >>>>> called from trampolines, or their callbacks. Now the question is, does BPF
> >>>>> programs ever call cond_resched()? I don't think they do.
> >>>>> 
> >>>>> [ Added Alexei ]
> >>>> 
> >>>> I'm a bit lost in this thread :)
> >>>> Just answering the above question.
> >>>> bpf progs never call cond_resched() directly.
> >>>> But there are sleepable (aka faultable) bpf progs that
> >>>> can call some helper or kfunc that may call cond_resched()
> >>>> in some path.
> >>>> sleepable bpf progs are protected by rcu_tasks_trace.
> >>>> That's a very different one vs rcu_tasks.
> >>> 
> >>> Suppose that the various cond_resched() invocations scattered throughout
> >>> the kernel acted as RCU Tasks quiescent states, so that as soon as a
> >>> given task executed a cond_resched(), synchronize_rcu_tasks() might
> >>> return or call_rcu_tasks() might invoke its callback.
> >>> 
> >>> Would that cause BPF any trouble?
> >>> 
> >>> My guess is "no", because it looks like BPF is using RCU Tasks (as you
> >>> say, as opposed to RCU Tasks Trace) only to wait for execution to leave a
> >>> trampoline.  But I trust you much more than I trust myself on this topic!
> >> 
> >> But it uses RCU Tasks Trace as well (for sleepable bpf programs), not just
> >> Tasks? Looks like that's what Alexei said above as well, and I confirmed it in
> >> bpf/trampoline.c
> >> 
> >>        /* The trampoline without fexit and fmod_ret progs doesn't call original
> >>         * function and doesn't use percpu_ref.
> >>         * Use call_rcu_tasks_trace() to wait for sleepable progs to finish.
> >>         * Then use call_rcu_tasks() to wait for the rest of trampoline asm
> >>         * and normal progs.
> >>         */
> >>        call_rcu_tasks_trace(&im->rcu, __bpf_tramp_image_put_rcu_tasks);
> >> 
> >> The code comment says it uses both.
> > 
> > BPF does quite a few interesting things with these.
> > 
> > But would you like to look at the update-side uses of RCU Tasks Rude
> > to see if lazy preemption affects them?  I don't believe that there
> > are any problems here, but we do need to check.
> 
> Sure I will be happy to. I am planning look at it in detail over the 3 day weekend. Too much fun! ;-)

Thank you, and looking forward to seeing what you come up with!

The canonical concern would be that someone somewhere is using either
call_rcu_tasks_rude() or synchronize_rcu_tasks_rude() to wait for
non-preemptible regions of code that does not account for the possibility
of preemption in CONFIG_PREEMPT_NONE or PREEMPT_PREEMPT_VOLUNTARY kernels.

I *think* that these are used only to handle the possibility
of tracepoints on functions on the entry/exit path and on the
RCU-not-watching portions of the idle loop.  If so, then there is no
difference in behavior for lazy preemption.  But who knows?

						Thanx, Paul