Re: [PATCH net v3 1/1] net: hsr: limit node table growth

[Felix Maurer <fmaurer@redhat.com>]

On Wed, Apr 22, 2026 at 10:52:42AM +0200, Sebastian Andrzej Siewior wrote:
> On 2026-04-22 10:31:39 [+0200], Felix Maurer wrote:
> > On Tue, Apr 21, 2026 at 10:50:01PM +0800, Ren Wei wrote:
> > > diff --git a/net/hsr/hsr_framereg.c b/net/hsr/hsr_framereg.c
> > > index d09875b33588..8a5a2a54a81f 100644
> > > --- a/net/hsr/hsr_framereg.c
> > > +++ b/net/hsr/hsr_framereg.c
> > > @@ -189,6 +195,7 @@ static struct hsr_node *hsr_add_node(struct hsr_priv *hsr,
> > >  				     enum hsr_port_type rx_port)
> > >  {
> > >  	struct hsr_node *new_node, *node = NULL;
> > > +	unsigned int node_count = 0;
> > >  	unsigned long now;
> > >  	size_t block_sz;
> > >  	int i;
> > > @@ -226,20 +233,31 @@ static struct hsr_node *hsr_add_node(struct hsr_priv *hsr,
> > >  	spin_lock_bh(&hsr->list_lock);
> > >  	list_for_each_entry_rcu(node, node_db, mac_list,
> > >  				lockdep_is_held(&hsr->list_lock)) {
> > > +		node_count++;
> >
> > I'm not sure if this on-the-fly node counting is the best solution here.
> > My concern is that it comes quite late in the process, i.e., after we
> > already allocated a bunch of memory, etc. As we are discussing a
> > scenario where a lot of entries are created, maybe we shouldn't even
> > allocate a new_node if the table is already full? For example by storing
> > the node_count in hsr_priv and checking it early in the function?
>
> The node is allocated upfront. Then it iterates here and we only end up
> counting through the full list if there is no match. This is under a
> lock so "many clients" are serialized. If we allocate the node later
> then we need to do it under the lock.
>
> I don't think the node count exceeds 100 in production. So having a
> counter which is incremented while adding to the list and decremented
> while removing items from the list would optimize the "worst case". So
> instead traversing the list with 1000 we would just give up.

The counter is what I had in mind. I agree that allocating under the
lock isn't what we want.

I'd argue counting through the whole list is the normal case.
hsr_add_node() is only called after the node table has been searched
already (without the lock). Here we go through the whole list again
under the lock to prevent TOCTOU-type situations.

I agree that, overall, it would be optimizing the worst case, but I
think it may be worth it to prevent the memory allocations and walking
the whole list. But I'd go along with the (current) on-the-fly counting
as well.

Thanks,
   Felix