Re: [PATCH net-next] net: dev: Always serialize on Qdisc::busylock in __dev_xmit_skb() on PREEMPT_RT.

From: Paolo Abeni <pabeni@redhat.com>
To: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: netdev@vger.kernel.org, "David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Eric Dumazet <edumazet@google.com>,
	Thomas Gleixner <tglx@linutronix.de>
Subject: Re: [PATCH net-next] net: dev: Always serialize on Qdisc::busylock in __dev_xmit_skb() on PREEMPT_RT.
Date: Mon, 13 Dec 2021 11:27:13 +0100	[thread overview]
Message-ID: <9b76f8593aa69596c2fe6f6bd2910b4078a1f6b8.camel@redhat.com> (raw)
In-Reply-To: <YbOEaSQW+LtWjuzI@linutronix.de>

On Fri, 2021-12-10 at 17:46 +0100, Sebastian Andrzej Siewior wrote:
> On 2021-12-10 17:35:21 [+0100], Paolo Abeni wrote:
> > On Fri, 2021-12-10 at 16:41 +0100, Sebastian Andrzej Siewior wrote:
> > > The root-lock is dropped before dev_hard_start_xmit() is invoked and after
> > > setting the __QDISC___STATE_RUNNING bit. If the Qdisc owner is preempted
> > > by another sender/task with a higher priority then this new sender won't
> > > be able to submit packets to the NIC directly instead they will be
> > > enqueued into the Qdisc. The NIC will remain idle until the Qdisc owner
> > > is scheduled again and finishes the job.
> > > 
> > > By serializing every task on the ->busylock then the task will be
> > > preempted by a sender only after the Qdisc has no owner.
> > > 
> > > Always serialize on the busylock on PREEMPT_RT.
> > 
> > Not sure how much is relevant in the RT context, but this should impact
> > the xmit tput in a relevant, negative way.
> 
> Negative because everyone blocks on lock and transmits packets directly
> instead of adding it to the queue and leaving for more?

In the uncontended scenario this adds an atomic operation and increases
the data set size. Thinking again about it, in RT build this is much
less noticeable (I forgot spinlock are actually mutex in RT...)

> > If I read correctly, you use the busylock to trigger priority
> > ceiling
> > on each sender. I'm wondering if there are other alternative ways
> > (no
> > contended lock, just some RT specific annotation) to mark a whole
> > section of code for priority ceiling ?!?
> 
> priority ceiling as you call it, happens always with the help of a
> lock.
> The root_lock is dropped in sch_direct_xmit().
> qdisc_run_begin() sets only a bit with no owner association.
> If I know why the busy-lock bad than I could add another one. The
> important part is force the sende out of the section so the task with
> the higher priority can send packets instead of queueing them only.

I thought about adding a local_lock() instead, but that will not solve.

I don't see a better solution than this patch, so I'm ok with that.
Please follow Jakub's suggestion to clean the code a bit.

Thanks!

Paolo