Re: if_flags usage etc.

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 
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.

(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.

(4) Push if_start oactive check into device driver to allow it to lock (or
     not) as it sees fit.

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"
>