Received-SPF: pass (google.com: domain of linux-kernel+bounces-85424-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Wed, 28 Feb 2024 09:18:45 -0800
From: "Paul E. McKenney" <paulmck@kernel.org>
To: Yan Zhai <yan@cloudflare.com>
Cc: Joel Fernandes <joel@joelfernandes.org>,
	Eric Dumazet <edumazet@google.com>, 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>,
	linux-kernel@vger.kernel.org, rcu@vger.kernel.org,
	bpf@vger.kernel.org, kernel-team@cloudflare.com,
	rostedt@goodmis.org, mark.rutland@arm.com
Subject: Re: [PATCH] net: raise RCU qs after each threaded NAPI poll
Message-ID: <e592faa3-db99-4074-9492-3f9021b4350c@paulmck-laptop>
Reply-To: paulmck@kernel.org
References: <Zd4DXTyCf17lcTfq@debian.debian>
 <CANn89iJQX14C1Qb_qbTVG4yoG26Cq7Ct+2qK_8T-Ok2JDdTGEA@mail.gmail.com>
 <d633c5b9-53a5-4cd6-9dbb-6623bb74c00b@paulmck-laptop>
 <f1d1e0fb-2870-4b8f-8936-881ac29a24f1@joelfernandes.org>
 <CAO3-Pboo32iQBBUHUELUkvvpSa=jZwUqefrwC-NBjDYx4yxYJQ@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAO3-Pboo32iQBBUHUELUkvvpSa=jZwUqefrwC-NBjDYx4yxYJQ@mail.gmail.com>

On Wed, Feb 28, 2024 at 10:37:51AM -0600, Yan Zhai wrote:
> On Wed, Feb 28, 2024 at 9:37 AM Joel Fernandes <joel@joelfernandes.org> wrote:
> > Also optionally, I wonder if calling rcu_tasks_qs() directly is better
> > (for documentation if anything) since the issue is Tasks RCU specific. Also
> > code comment above the rcu_softirq_qs() call about cond_resched() not taking
> > care of Tasks RCU would be great!
> >
> Yes it's quite surprising to me that cond_resched does not help here,

In theory, it would be possible to make cond_resched() take care of
Tasks RCU.  In practice, the lazy-preemption work is looking to get rid
of cond_resched().  But if for some reason cond_resched() needs to stay
around, doing that work might make sense.

> I will include that comment. Raising just the task RCU QS seems
> sufficient to the problem we encountered. But according to commit
> d28139c4e967 ("rcu: Apply RCU-bh QSes to RCU-sched and RCU-preempt
> when safe"), there might be additional threat factor in __do_softirq
> that also applies to threaded poll.

We did look into more focused alternatives for Tasks RCU a few days ago,
but they all had problems, for example, requiring that it be possible
to get exact information on the instruction pointers for interrupts on
any given CPU's stack.  The key point of Tasks RCU is to work out when
an old tracing trampoline may safely be freed, so a better way of doing
that would remove the need for this sort of addition to NAPI polling.

(Adding Steve and Mark for their thoughts.)

							Thanx, Paul

> Yan
> 
> 
> > Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> >
> > thanks,
> >
> >  - Joel
> > [1]
> > @@ -381,8 +553,10 @@ asmlinkage __visible void __softirq_entry __do_softirq(void)
> >                 pending >>= softirq_bit;
> >         }
> >
> > -       if (__this_cpu_read(ksoftirqd) == current)
> > +       if (!IS_ENABLED(CONFIG_PREEMPT_RT) &&
> > +           __this_cpu_read(ksoftirqd) == current)
> >                 rcu_softirq_qs();
> > +
> >         local_irq_disable();