Re: [PATCH net v4 2/4] net: hsr: shrink seqnr_lock to sequence counter updates

[Hangbin Liu <liuhangbin@gmail.com>]

Hi Xie Xin,
On Tue, Aug 04, 2026 at 12:22:09AM +0200, Xin Xie wrote:
> hsr->seqnr_lock is currently held across entire hsr_forward_skb()
> calls: master TX (hsr_dev_xmit()), interlink RX (hsr_handle_frame()),
> and both supervision frame builders hold it while frames are built,
> classified, duplicated and forwarded on every port. The only state
> that actually needs the lock is the sequence counters themselves
> (hsr->sequence_nr / hsr->sup_sequence_nr).
> 
> Shrink the locking to the individual counter updates:
> handle_std_frame() now takes the lock around its sequence number
> allocation (replacing the lockdep assertion), the master TX and
> interlink RX paths drop their outer lock, and the supervision
> builders release the lock right after updating their counter instead
> of holding it across frame construction and forwarding.
> 
> Statistics: the outer seqnr_lock also incidentally serialized the
> tx_packets/tx_bytes/tx_dropped updates in hsr_forward_skb(), which are
> reachable from the concurrently callable master-TX, supervision and
> interlink-RX paths. Those updates now use DEV_STATS_INC()/
> DEV_STATS_ADD(), the atomic legacy-stat helpers, so narrowing the lock
> does not turn them into lossy plain read-modify-writes.
> 
> Ordering: with IFF_NO_QUEUE and dev->lltx, hsr_dev_xmit() is
> concurrently callable, and this change removes the old
> allocation-through-forward ordering guarantee there: master-TX frames
> may now be emitted out of sequence-allocation order. On the current
> tree this is safe because duplicate discard tracks individual
> sequence numbers in sparse bitmaps (commit aae9d6b616b5 ("hsr:
> Implement more robust duplicate discard for HSR") and
> commit 415e6367512b ("hsr: Implement more robust duplicate discard
> for PRP")) rather than requiring monotonic arrival. Sequence numbers
> remain unique and monotonically allocated per counter.
> 
> This is a latency/critical-section prerequisite for unfolding GSO
> super-packets at the forward entry (not a functional prerequisite):
> the segmentation work should not extend the global sequence lock's
> critical section. Patch 3 depends on this change; their automatic
> stable selection is limited to 7.0 and newer, where sparse-bitmap
> duplicate discard accepts out-of-order arrival. Older stable branches
> require an adapted backport.
> 
> History of the lock being narrowed: it was introduced by
> commit 06afd2c31d33 ("hsr: Synchronize sending frames to have always
> incremented outgoing seq nr.") and briefly removed by
> commit b3c9e65eb227 ("net: hsr: remove seqnr_lock") in net. Merge
> commit 46ae4d0a4897 ("Merge git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net")
> reverted that removal because
> commit 430d67bdcb04 ("net: hsr: Use the seqnr lock for frames
> received via interlink port.") in net-next had already superseded it
> by adding locking for the interlink RX path. All sequence counter
> updates remain protected; only the forwarding work moves out of the
> critical section.
> 
> Cc: <stable@vger.kernel.org> # 7.0.x
> Signed-off-by: Xin Xie <xiexinet@gmail.com>

Maybe use a shorter commit description.

[ ... ]
> @@ -746,8 +747,8 @@ void hsr_forward_skb(struct sk_buff *skb, struct hsr_port *port)
>  	 * So check and increment stats for master port only here.
>  	 */
>  	if (port->type == HSR_PT_MASTER || port->type == HSR_PT_INTERLINK) {
> -		port->dev->stats.tx_packets++;
> -		port->dev->stats.tx_bytes += skb->len;
> +		DEV_STATS_INC(port->dev, tx_packets);
> +		DEV_STATS_ADD(port->dev, tx_bytes, skb->len);

Note: Avoid these macros in fast path, prefer per-cpu or per-queue counters.

And the counters in hsr_deliver_master() also need to protected. Especially
multicast.

Thanks
Hangbin