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

On Fri, 2021-12-10 at 17:46 +0100, Sebastian Andrzej Siewior wrote:

On 2021-12-10 17:35:21 [+0100], Paolo Abeni wrote:

quoted

On Fri, 2021-12-10 at 16:41 +0100, Sebastian Andrzej Siewior wrote:

quoted

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...)

quoted

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