Date: Tue, 24 Jan 2006 13:58:49 -0800 From: Luigi Rizzo <rizzo@icir.org> To: Robert Watson <rwatson@freebsd.org> Cc: current@freebsd.org Subject: Re: if_flags usage etc. Message-ID: <20060124135849.A92938@xorpc.icir.org> In-Reply-To: <20060124214306.S48094@fledge.watson.org>; from rwatson@freebsd.org on Tue, Jan 24, 2006 at 09:47:08PM %2B0000 References: <20060124075437.B67285@xorpc.icir.org> <20060124214306.S48094@fledge.watson.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Jan 24, 2006 at 09:47:08PM +0000, Robert Watson wrote: > > On Tue, 24 Jan 2006, Luigi Rizzo wrote: > > > approx 1 month ago there was some discussion on the usage of if_flags and > > if_drv_flags in the stack As an exercise, i tried to identify the places > > these flags are used by taking a -current snapshot, splitting if_flags into > > two sets (those created at init time, those modified during normal > > operation) called if_flags_i and if_flags_n, and renaming the flags IFF_i_* > > and IFF_n_* respectively. Also, to point out where the IFF_DRV_* flags are > > used, i have renamed them IFF_d_DRV_*. > > I've spent several weeks over the past couple of months working through the > use of if_flags in the network stack. There are a number of complicating i can well imagine that given how i have spent the past 36 hours :) > issues, and I basically concluded that before we could get too much further, > we needed to catalog and normalize the use of the flags across drivers. The > mean sticking points I ran into were: > > (1) Modify drivers not to directly adjust IFF_UP. In many cases, this is > simple, but in others, it is hard. this could be done right away for the simple ones, no ? same for IFF_PROMISC, which according to my incomplete list is the other misused (in some sense) flag. > (2) Clarify semantics of driver-specific (LINKX) flags, which sometimes > reflect driver state, and sometimes control it (and sometimes both). > > (3) Introduce or reuse a mutex to protect read-modify-write of if_flags by the > stack. this i think is unnecessary - once you have split them into separate groups, basically none of the (n) flags involves races, and probably the only critical issue is define how to make the transitions to 0 of IFF_RUNNING force IFF_UP also to 0. > (4) Push if_start oactive check into device driver to allow it to lock (or > not) as it sees fit. this i am a bit unsure, too. As it is now, it is conveniently located in the handoff macro on the network stack side. I would probably try to hide it into a macro of some kind in the driver side as well, so that later we could replace the enqueue/dequeue macros with equivalent ones relying on a different form of synchronization without having to touch individual drivers. more in a day or two... cheers luigi > I have works in progress for several of these, but because there's inevitably > a snow-balling effect, I've been trickling changes in a bit as a time as I > decide what is actually the right thing to do and fix drivers to match. The > if_drv_flags breakout was all I figured was safe to get in to 6.x, but I hope > to do quite a bit more for 7.x. I have significant numbers of patches along > the above lines in p4, and will try to sort them out in the next few days. > > Robert N M Watson > > > > > > just to see how it looks, the patch that allows GENERIC to compile is at > > http://info.iet.unipi.it/~luigi/FreeBSD/if_flags.20060114.diff > > (i know it is missing netgraph, carp, vlan and atm, but > > covers the majority of drivers). > > No big suprises here but a few interesting things: > > > > - because if_drv_flags only contains RUNNING and OACTIVE, it > > could be manipulated by simple assignments by the drivers, > > instead of using bitwise ops; > > > > - ifp->if_drv_flags |= IFF_d_DRV_RUNNING; > > - ifp->if_drv_flags &= ~IFF_d_DRV_OACTIVE; > > + ifp->if_drv_flags = IFF_d_DRV_RUNNING; /* clear OACTIVE */ > > > > ... > > > > - ifp->if_drv_flags &= ~(IFF_d_DRV_RUNNING | IFF_d_DRV_OACTIVE); > > + ifp->if_drv_flags = 0; > > > > and so on; > > > > - the check for IFF_UP is written so often that i wonder if it wouldn't > > be the case to wrap it around a macro, as e.g. it is done in > > net80211/ieee80211_ioctl.c for something similar. > > > > - IFF_CANTCHANGE still has masks for IFF_DRV_RUNNING and IFF_DRV_OACTIVE, > > that are no more in if_flags so should probably be removed. > > > > - very few places outside drivers look at IFF_DRV_RUNNING. Basically. > > net/if_ethersubr.c, netinet/ip_fastfwd.c , net80211/ieee80211_ioctl.c > > in all cases checking that both IFF_UP and IFF_RUNNING are set. > > I am not sure why we need both. > > > > - some drivers try to manipulate flags that they should not e.g. > > > > if_bge > > sets IFF_UP > > > > if_ed > > sets IFF_ALTPHYS (IFF_LINK*, but it is almost a capability here) > > > > if_fwe > > sets IFF_PROMISC > > > > if_ie > > resets IFF_UP > > sets IFF_UP > > sets IFF_ALLMULTI (commented as broken) > > > > if_plip > > sets IFF_UP > > > > if_re > > resets IFF_PROMISC > > sets IFF_PROMISC > > resets IFF_UP > > if_vge > > resets IFF_UP > > if_faith > > sets IFF_UP > > if_gif > > sets IFF_UP > > if_loop > > sets IFF_UP > > > > if_ppp > > resets IFF_UP > > if_sl > > resets IFF_UP > > sets IFF_UP > > if_tun > > sets IFF_UP > > > > if_de > > resets IFF_UP > > > > this can be probably fixedin many cases. > > > > - in most drivers, the logic for SIOCSIFFLAGS is very > > convoluted and could be cleaned up a lot. if_dc.c has a > > reasonable version. if_xl.c has a bad example, unfortunately > > copied a few times around. > > > > cheers > > luigi > > _______________________________________________ > > freebsd-current@freebsd.org mailing list > > http://lists.freebsd.org/mailman/listinfo/freebsd-current > > To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org" > > > _______________________________________________ > freebsd-current@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-current > To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20060124135849.A92938>