Re: [PATCH net-next v2 4/9] hsr: Implement more robust duplicate discard for PRP

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2026-02-03 11:23:57 [+0100], Felix Maurer wrote:
> On Mon, Feb 02, 2026 at 05:57:02PM +0100, Sebastian Andrzej Siewior wrote:
> > On 2026-01-28 19:37:23 [+0100], Felix Maurer wrote:
> > > > > +	if (!block) {
> > > > > +		block = &node->block_buf[node->next_block];
> > > > > +		hsr_forget_seq_block(node, block);
> > > > > +
> > > > > +		memset(block, 0, sizeof(*block));
> > > > > +		block->time = jiffies;
> > > > > +		block->block_idx = block_idx;
> > > > > +
> > > > > +		res = xa_store(&node->seq_blocks, block_idx, block, GFP_ATOMIC);
> > > > > +		if (xa_is_err(res))
> > > >
> > > > Hi Felix,
> > > >
> > > > I ran Claude Code over this with review-prompts [1] and it flags
> > > > that in the error path above, the following is needed so that the
> > > > block can be re-used.
> > > >
> > > > 			block->time = 0;
> > >
> > > I agree. It would nonetheless be reused at some point, but not setting
> > > time = 0 may lead to an unexpected block getting removed when it is
> > > reused (or at least an attempt to do so). I'll fix this in v3.
> >
> > Not sure I follow. If xa_store() fails then the block is not added to
> > the "sequence-blocks" and it will be attempted once the next packet is
> > received, right?. Otherwise node->next_block is not updated at which
> > point this block will be added twice which sounds worse.
> 
> Yes, it will be attempted again on the next frame that needs a new
> block. But every attempt to recycle the next_block starts with calling
> hsr_forget_seq_block(), which tries to xa_erase() the block if time!=0.
> Generally, time==0 is the marker that a block is not stored in the
> xarray. So for consistency it's correct to set time=0 if the block can't
> be added to the xarray, even though I don't think it would cause any
> trouble at the moment if the time is not reset.

My point is that the block will not be recycled if it failed to be added
here. It remains the "next" block to be used and added to the list if it
fails that xa_store().

> Thanks,
>    Felix

Sebastian