On Sat, Apr 23, 2022 at 06:07:23PM +0200, Lukas Wunner wrote:
> On Thu, Apr 21, 2022 at 10:02:43AM +0200, Paolo Abeni wrote:
> > On Sun, 2022-04-17 at 09:04 +0200, Lukas Wunner wrote:
> > > --- a/net/core/link_watch.c
> > > +++ b/net/core/link_watch.c
> > > @@ -107,7 +107,8 @@ static void linkwatch_add_event(struct net_device *dev)
> > > unsigned long flags;
> > >
> > > spin_lock_irqsave(&lweventlist_lock, flags);
> > > - if (list_empty(&dev->link_watch_list)) {
> > > + if (list_empty(&dev->link_watch_list) &&
> > > + dev->reg_state < NETREG_UNREGISTERED) {
> > > list_add_tail(&dev->link_watch_list, &lweventlist);
> > > dev_hold_track(dev, &dev->linkwatch_dev_tracker, GFP_ATOMIC);
> > >
> >
> > What about testing dev->reg_state in linkwatch_fire_event() before
> > setting the __LINK_STATE_LINKWATCH_PENDING bit, so that we don't leave
> > the device in an unexpected state?
About __LINK_STATE_LINKWATCH_PENDING being set even though the netdev
is not on link_watch_list:
After this patch (which removes one user of __LINK_STATE_LINKWATCH_PENDING)
the only purpose of the flag is a small speed-up of linkwatch_fire_event():
If the netdev is already on link_watch_list, the function skips acquiring
lweventlist_lock.
I don't think this is a hotpath, so the small speed-up is probably not worth
it and the flag could be removed completely in a follow-up patch.
There is a single other (somewhat oddball) user of the flag in
bond_should_notify_peers() in drivers/net/bonding/bond_main.c.
It would be possible to replace it with "!list_empty(&dev->link_watch_list)".
I don't think acquiring lweventlist_lock is necessary for that because
test_bit() is unordered (per Documentation/atomic_bitops.txt) and the
check is racy anyway.
Thanks,
Lukas
