> What are stacked HSR devices? Looking from the setup in the referenced
> report this is a bridge device attached to a HSR ring.
> An announce frame is sent and this goes to hsr_dev_xmit(). This does not
> look like a normal setup. Is this something you use or is this something
> the bot came up with?
It is the setup from the syzbot report, not something we use. In that
report the slave port of one HSR device is a bridge, and one bridge
port is a second HSR device. The announce frame of the first device
holds its seqnr_lock, goes through the bridge, and ends in
hsr_dev_xmit() of the second device, which takes its own seqnr_lock.
Lockdep sees the same lock class twice and reports it. With two
devices this is not a real deadlock. It only becomes one when the
transmit path comes back to the same device.
> This looks very complex and I am not a big fan of it. I would prefer to
> break lock chain early assuming the setup is valid/ sane (see above).
I understand. The queue exists only for the ordering you pointed out
in v6: frame N must reach every lower device before frame N+1 gets its
number, otherwise a receiver without the bitmap discard (aae9d6b616b5)
drops N as stale. In our test an unpatched HSRv0 peer received 19128
of 20000 frames with the narrowed lock, and all 20000 with the queue.
> The ID by itself can be assigned with an atomic_inc() but then you don't
> have the transmit ordering. So you introduce the workqueue for the
> ordering which is ugh. I'm not sure how this works if you get
> submissions from two CPUs given the lock is gone and the workqueue is
> per-CPU.
The FIFO and the owner flag are protected by fwd_lock. Only one
context owns the consumer at a time: either the CPU that found it idle
and processes its own frame, or the single work item of this HSR
device. A frame submitted from another CPU meanwhile is appended to
the FIFO and processed after the current frame, in that order. The
work item is one work_struct per device, so the per-CPU workqueue
cannot run it twice at the same time. But I agree it is a lot of code
for this, and it also serializes all forwarding of one device, like the
lock does today.
So for v8 I see two simple options:
(a) Keep seqnr_lock across the submission like today and only break
the re-entry: a frame that enters HSR transmit while this CPU is
already inside HSR forwarding is dropped and counted. Ordering is
kept, but all forwarding of one device stays serialized.
(b) Narrow the lock to the counter update, as in v6 and in Hangbin's
patch [1]. Several CPUs can forward in parallel. A receiver without
the bitmap discard can drop a reordered frame, so the patch would
carry "Cc: <[email protected]> # aae9d6b616b5: ..." as Simon
suggested there, and the TX statistics move to per-CPU counters.
I would prefer (b) because of the parallel forwarding, if the old
receiver limitation is acceptable to you. Otherwise (a).
Hangbin, Simon, your view is welcome too.
Patch 3 works with both: it segments before the lock, and each
segment is numbered and submitted under the lock.
[1]
https://lore.kernel.org/netdev/[email protected]/
--
Xin
