On 03/24, Jakub Kicinski wrote: > On Tue, 24 Mar 2026 11:13:04 -0700 Stanislav Fomichev wrote: > > > > + netif_addr_lock_bh(dev); > > > > + > > > > + err = __hw_addr_list_snapshot(&uc_snap, &dev->uc, > > > > + dev->addr_len); > > > > + if (!err) > > > > + err = __hw_addr_list_snapshot(&uc_ref, &dev->uc, > > > > + dev->addr_len); > > > > + if (!err) > > > > + err = __hw_addr_list_snapshot(&mc_snap, > > > > &dev->mc, > > > > + dev->addr_len); > > > > + if (!err) > > > > + err = __hw_addr_list_snapshot(&mc_ref, &dev->mc, > > > > + dev->addr_len); > > > > > > This doesn't get slow with a few thousands of addresses? > > > > I can add kunit benchmark and attach the output? Although not sure where > > to go from that. The alternative to this is allocating an array of entries. > > I started with that initially but __hw_addr_sync_dev wants to kfree the > > individual entries and I decided not to have a separate helpers to > > manage the snapshots. > > Let's see what the benchmark says. Hopefully it's fast enough and > we don't have to worry. Is keeping these lists around between the > invocations of the work tricky?
Yeah, that sounds doable, don't think it's too tricky, just extra list_head on net_device + change the alloc/free to use it. And then we keep this cache around until unregister? I will try to add it as a separate patch to cache these entries to keep it simple for review.. > > > Can we give the work a reference on the netdev (at init time) and > > > cancel + release it here instead of flushing / waiting? > > > > Not sure why cancel+release, maybe you're thinking about the unregister > > path? This is rtnl_unlock -> netdev_run_todo -> __rtnl_unlock + some > > extras. > > > > And the flush is here to plumb the addresses to the real devices > > before we return to the callers. Mostly because of the following > > things we have in the tests: > > > > # TEST: team cleanup mode lacp [FAIL] > > # macvlan unicast address not found on a slave > > > > Can you explain a bit more on the suggestion? > > Oh, I thought it's here for unregister! Feels like it'd be cleaner to > add the flush in dev_*c_add() and friends? How hard would it be to > identify the callers in atomic context? Not sure we can do it in dev_xc_add because it runs under rtnl :-( I currently do flush in netdev_run_todo because that's the place that doesn't hold rtnl. Otherwise flush will get stuck because the work handler grabs it...
