Thanks dlg@ for the hint with the ixl(4) fixes in current. I will
try so solve my 6.6 problem with that.
On Wed, Jun 16, 2021 at 10:19:03AM +0200, Martin Pieuchot wrote:
> The diff discussed in this thread reduces the scope of the splnet/splx()
> dance to only surround the modification of `if_flags'. How is this
> related to what you said? Is it because `if_flags' is read in interrupt
> handlers and it isn't modified atomically? Does that imply that every
> modification of `if_flags' should be done at IPL_NET? Does that mean
> some love is needed to ensure reading `if_flags' is coherent?
I have checked some drivers: hme, gem, em, ix.
They read if_flags in the interrupt handler, but never set it there.
So it seems to be save to change it without splnet(). We can remove
it from ifnewlladdr().
And most changes seem to happen from ioctl path, there we have net
lock. But I see also places where if_flags is changed with kernel
lock. This is in carp_carpdev_state() task and e.g. ixgbe_watchdog().
As we also have kernel lock in the ioctl path, I think this is the
lock which protects if_flags modifications. Look at if_watchdog_task()
and carp_carpdev_state().
> Does that change also imply that it is safe to issue a SIOCSIFFLAGS on
> a legacy drivers without blocking interrupts? If the IPL needs to be
> raised, this is left to the driver, right?
The drivers I have checked do splnet() in their ioctl function.
ifioctl() calls driver ioctl with net lock, but without splnet().
ixl does not use splnet() at all.
> Was the current splnet/splx() dance an easy way to block packet reception
> between multiple SIOCSIFFLAGS ioctls? This might have been a way to not
> receive packets on a DOWN interface. This is no longer the case on MP
> kernels as there's a window between the UP and DOWN ioctls. Do we care?
> Is this down/up/down dance fine for legacy a modern drivers?
I guess, if we allow interrupts to happen, all packets should have
been processed after we return from setting the interface down.
Is this the correct version of the diff? Not tested yet.
bluhm
Index: net/if.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/net/if.c,v
retrieving revision 1.641
diff -u -p -r1.641 if.c
--- net/if.c 25 May 2021 22:45:09 -0000 1.641
+++ net/if.c 16 Jun 2021 23:46:42 -0000
@@ -3107,9 +3107,10 @@ ifnewlladdr(struct ifnet *ifp)
#endif
struct ifreq ifrq;
short up;
- int s;
- s = splnet();
+ NET_ASSERT_LOCKED(); /* for ioctl and in6 */
+ KERNEL_ASSERT_LOCKED(); /* for if_flags */
+
up = ifp->if_flags & IFF_UP;
if (up) {
@@ -3143,7 +3144,6 @@ ifnewlladdr(struct ifnet *ifp)
ifrq.ifr_flags = ifp->if_flags;
(*ifp->if_ioctl)(ifp, SIOCSIFFLAGS, (caddr_t)&ifrq);
}
- splx(s);
}
void
