Re: if_flags usage etc.

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_at_freebsd.org mailing list
> > http://lists.freebsd.org/mailman/listinfo/freebsd-current
> > To unsubscribe, send any mail to "freebsd-current-unsubscribe_at_freebsd.org"
> >
> _______________________________________________
> freebsd-current_at_freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-current
> To unsubscribe, send any mail to "freebsd-current-unsubscribe_at_freebsd.org"