Message-ID: <aYHXN1RvUoX6TGXM@thinkpad>
Date: Tue, 3 Feb 2026 12:08:39 +0100
From: Felix Maurer <fmaurer@...hat.com>
To: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc: netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, horms@...nel.org,
	jkarrenpalo@...il.com, tglx@...utronix.de, mingo@...nel.org,
	allison.henderson@...cle.com, petrm@...dia.com, antonio@...nvpn.net,
	Yoann Congal <yoann.congal@...le.fr>
Subject: Re: [PATCH net-next v2 6/9] hsr: Implement more robust duplicate
 discard for HSR

On Mon, Feb 02, 2026 at 06:11:23PM +0100, Sebastian Andrzej Siewior wrote:
> On 2026-01-29 17:17:04 [+0100], Felix Maurer wrote:
> > About the interlink: that's the interface where you attach devices that
> > know nothing about HSR, i.e., when we are a RedBox. I consider it very
> > similar to the master port, it's our responsibility to de-duplicate what
> > we send out there.
>
> Right. It should contain the same "seen" values as the master port.
>
> > I was thinking about exactly this while working on the patch as well and
> > I came to each conclusion (A,B are needed vs. are not needed) at least
> > once. In the end, I think we will need it. It's right that a well
> > behaving node in the ring should not forward "frames for which the node
> > is the unique destination" (5.3.2.1). But there could be frames that
> > have no unique destination in the ring at all: multicast frames or
> > frames addressed to a non-existing MAC address. We should not forward
> > such frames either to prevent them from looping forever.
>
> But multicast and non-existing destination frames do one round in the
> circle, reach the sender which will remove them from the ring and not
> forward.
>
> > Now, such frames should probably only reach back to us, if we sent them
> > (either from our stack or from the interlink port). We could track
> > sequence numbers sent to master and interlink (for de-duplication) and
> > sent by master and interlink (for loop prevention) for more clarity, but
> > then we're back at four bitmaps again.
>
> I think we could avoid tracking A/ B. The packets that we receive from
> A/ B and forward to the master after de-duplication should be the same
> as those that are forwarded to the interlink port.

I consider this an argument for removing the separate tracking of master
and interlink (and thereby saving another 1k), by changing the
forwarding code to not iterate over all ports independently.

I'll reply to the A/B tracking topic in the other mail :)


> > > I know you preserve what is already here but what is this even used for?
> > > | ip -d link show dev hsr0
> > >
> > > does not show these numbers. It shows the sequence number of the hsr0
> > > interface which I understand.
> > > But then it is also possible to show the last received sequence number
> > > of any node on either of the two interfaces?
> >
> > This is only exposed through the HSR_C_GET_NODE_STATUS netlink command
> > for the "HSR" (sic!) netlink family. I'm not aware of a userspace tool
> > that actually shows this data. It's unclear if it is of any real use.
>
> hehe. Great.
>
> > I assume that the two sequence numbers are reported because they would
> > in theory allow to detect a broken ring: if one of the numbers stops
> > increasing or the numbers generally diverge, some link in the ring is
> > broken. If we have traffic from multiple nodes, we could even detect
> > where the ring is broken. That's likely also the reason for the weird
> > cross-assinment of B->1, A->2. What we send out on B has to have arrived
> > on port A resp. 1 before.
> >
> > We can discuss removing this stuff in the future, but I'd prefer not to
> > touch userspace at this time.
>
> Sure. I was just looking who is using this and this netlink interface
> was the only user. It looks like debug code.

For which we would have the debugfs... The netlink interface is already
on my list to take a look at. I'll take that debug aspect into account.
I might start a different thread with the discussion topics that came up
with this patchset.

> > > > +}
> > > > +
> > > >  int hsr_get_node_data(struct hsr_priv *hsr,
> > > >  		      const unsigned char *addr,
> > > >  		      unsigned char addr_b[ETH_ALEN],
> > > …
> > > > --- a/net/hsr/hsr_framereg.h
> > > > +++ b/net/hsr/hsr_framereg.h
> > > > @@ -82,15 +82,18 @@ int prp_register_frame_out(struct hsr_port *port, struct hsr_frame_info *frame);
> > > >  #define hsr_seq_block_index(sequence_nr) ((sequence_nr) >> HSR_SEQ_BLOCK_SHIFT)
> > > >  #define hsr_seq_block_bit(sequence_nr) ((sequence_nr) & HSR_SEQ_BLOCK_MASK)
> > > >
> > > > +#define DECLARE_BITMAP_FLEX_ARRAY(name, bits) \
> > > > +	unsigned long name[][BITS_TO_LONGS(bits)]
> > > > +
> > > >  struct hsr_seq_block {
> > > >  	unsigned long		time;
> > > >  	u16			block_idx;
> > > > -	DECLARE_BITMAP(seq_nrs, HSR_SEQ_BLOCK_SIZE);
> > > > +	DECLARE_BITMAP_FLEX_ARRAY(seq_nrs, HSR_SEQ_BLOCK_SIZE);
> > >
> > > is there a story behind DECLARE_BITMAP_FLEX_ARRAY()? We have just this
> > > one user.
> >
> > I've added it this way to make it obvious that almost all of it is the
> > same as DECLARE_BITMAP(). But probably it's better to get rid of the
> > macro, add the definition directly to the struct, and maybe add a
> > comment that it's supposed to be similar to what DECLARE_BITMAP()
> > produces. What do you think?
>
> I would get rid of it. There the "official" DECLARE_BITMAP(). If this
> doesn't work just open code it avoiding the macro.

Alright, I did that in the v3 [1] I sent yesterday. Sorry for not
waiting with that until the end of every discussion. I understood your
reply to 0/9 as a "go" for sending a new version addressing the small
issues (without touching the A/B tracking).

Thanks,
   Felix


[1]: https://lore.kernel.org/netdev/cover.1770041682.git.fmaurer@redhat.com/

Powered by blists - more mailing lists