Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a4ed5a8cc35f34a3cb11aded76b0f289c658c1a7.camel@redhat.com>
Subject: Re: Splat in kernel RT while processing incoming network packets
From:   Paolo Abeni <pabeni@redhat.com>
To:     Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
        Wander Lairson Costa <wander@redhat.com>
Cc:     linux-kernel@vger.kernel.org, linux-rt-users@vger.kernel.org,
        juri.lelli@redhat.com
Date:   Tue, 04 Jul 2023 12:29:33 +0200
In-Reply-To: <20230704100527.75hMNZ35@linutronix.de>
References: <bkw2aao62e3ppg7332dbhycgzdwr7k5brezj3bcij6zewphmnd@eigmbvjh6wuu>
         <20230703142908.RcxjjF_E@linutronix.de>
         <x6d2ae4ss4wvvuib2hmop6ztysjsbyno7gbjkyek5xng2kmgyd@yfmnfognlj5n>
         <20230704100527.75hMNZ35@linutronix.de>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4 (3.46.4-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

On Tue, 2023-07-04 at 12:05 +0200, Sebastian Andrzej Siewior wrote:
> On 2023-07-03 18:15:58 [-0300], Wander Lairson Costa wrote:
> > > Not sure how to proceed. One thing you could do is a hack similar lik=
e
> > > net-Avoid-the-IPI-to-free-the.patch which does it for defer_csd.
> >=20
> > At first sight it seems straightforward to implement.
> >=20
> > > On the other hand we could drop net-Avoid-the-IPI-to-free-the.patch a=
nd
> > > remove the warning because we have now commit
> > >    d15121be74856 ("Revert "softirq: Let ksoftirqd do its job"")
> >=20
> > But I am more in favor of a solution that removes code than one that
> > adds more :)
>=20
> Raising the softirq from anonymous (hardirq context) is not ideal for
> the reasons I stated below.
>=20
> > > Prior that, raising softirq from hardirq would wake ksoftirqd which i=
n
> > > turn would collect all pending softirqs. As a consequence all followi=
ng
> > > softirqs (networking, =E2=80=A6) would run as SCHED_OTHER and compete=
 with
> > > SCHED_OTHER tasks for resources. Not good because the networking work=
 is
> > > no longer processed within the networking interrupt thread. Also not =
a
> > > DDoS kind of situation where one could want to delay processing.
> > >=20
> > > With that change, this isn't the case anymore. Only an "unrelated" IR=
Q
> > > thread could pick up the networking work which is less then ideal. Th=
at
> > > is because the global softirq set is added, ksoftirq is marked for a
> > > wakeup and could be delayed because other tasks are busy. Then the di=
sk
> > > interrupt (for instance) could pick it up as part of its threaded
> > > interrupt.
> > >=20
> > > Now that I think about, we could make the backlog pseudo device a
> > > thread. NAPI threading enables one thread but here we would need one
> > > thread per-CPU. So it would remain kind of special. But we would avoi=
d
> > > clobbering the global state and delay everything to ksoftird. Process=
ing
> > > it in ksoftirqd might not be ideal from performance point of view.
> >=20
> > Before sending this to the ML, I talked to Paolo about using NAPI
> > thread. He explained that it is implemented per interface. For example,
> > for this specific case, it happened on the loopback interface, which
> > doesn't implement NAPI. I am cc'ing him, so the can correct me if I am
> > saying something wrong.
>=20
> It is per NAPI-queue/instance and you could have multiple instances per
> interface. However loopback has one and you need per-CPU threads if you
> want to RPS your skbs to any CPU.

Just to hopefully clarify the networking side of it, napi instances !=3D
network backlog (used by RPS). The network backlog (RPS) is available
for all the network devices, including the loopback and all the virtual
ones.=C2=A0

The napi instances (and the threaded mode) are available only on
network device drivers implementing the napi model. The loopback driver
does not implement the napi model, as most virtual devices and even
some H/W NICs (mostily low end ones).

The network backlog can't run in threaded mode: there is no API/sysctl
nor infrastructure for that. The backlog processing threaded mode could
be implemented, even if should not be completely trivial and it sounds
a bit weird to me.


Just for the records, I mentioned the following in the bz:

It looks like flush_smp_call_function_queue() has 2 only callers,
migration, and do_idle().

What about moving softirq processing from
flush_smp_call_function_queue() into cpu_stopper_thread(), outside the
unpreemptable critical section?

I *think*/wild guess the call from do_idle() could be just removed (at
least for RT build), as according to:

commit b2a02fc43a1f40ef4eb2fb2b06357382608d4d84
Author: Peter Zijlstra <peterz@infradead.org>
Date:   Tue May 26 18:11:01 2020 +0200

	smp: Optimize send_call_function_single_ipi()

is just an optimization.

Cheers,

Paolo