Re: [PATCH net-next] ppp: Replace per-CPU recursion counter with lock-owner field

On 2025-07-08 10:28:24 [+0200], Paolo Abeni wrote:

Hi,

Hi,

I'm sorry for the latency, OoO here in between.

All good. I appreciate someone looking at it.

On 7/4/25 5:48 PM, Sebastian Andrzej Siewior wrote:

quoted

On 2025-07-03 09:55:21 [+0200], Paolo Abeni wrote:

quoted

Is there any special reason to not use local_lock here? I find this
patch quite hard to read and follow, as opposed to the local_lock usage
pattern. Also the fact that the code change does not affect RT enabled
build only is IMHO a negative thing.

Adding a local_lock_t to "protect" the counter isn't that simple. I
still have to check for the owner of the lock before the lock is
acquired to avoid recursion on that local_lock_t. I need to acquire the
lock before checking the counter because another task might have
incremented the counter (so acquiring the lock would not deadlock). This
is similar to the recursion detection in openvswitch. That means I would
need to add the local_lock_t and an owner field next to the recursion
counter.

IMHO using a similar approach to something already implemented is a
plus, and the OVS code did not look that scaring. Also it had the IMHO
significant advantage of keeping the changes constrained to the RT build.

I intended to improve the code and making it more understandable of what
happens here and why. Additionally it would also fit with RT and not
just make this change to fit with RT.

quoted

I've been looking at the counter and how it is used and it did not look
right. The recursion, it should detect, was described in commit
55454a565836e ("ppp: avoid dealock on recursive xmit"). There are two
locks that can be acquired due to recursion and that one counter is
supposed to catch both cases based on current code flow.

It is also not obvious why ppp_channel_push() makes the difference
depending on pch->ppp while ->start_xmit callback is invoked based on
pch->chan.
It looked more natural to avoid the per-CPU usage and detect the
recursion based on the lock that might be acquired recursively. I hope
this makes it easier to understand what is going on here.

Actually I'm a bit lost. According to 55454a565836e a single recursion
check in ppp_xmit_process() should be enough, and I think that keeping
the complexity constraint there be better.

Okay. I didn't think that this complicated the code flow.

quoted

While looking through the code I wasn't sure if
ppp_channel_bridge_input() requires the same kind of check for recursion
but adding it based on the lock, that is about to be acquired, would be
easier.

(still lost in PPP, but) The xmit -> input path transition should have
already break the recursion (via the backlog). Recursion check in tx
should be sufficient.

All in all I think it would be safer the local lock based approach.

Okay. I disagree but let me do as you suggested.

Thank you.

Thanks,

Paolo

Sebastian