if_flags usage etc.
Luigi Rizzo
rizzo at icir.org
Tue Jan 24 13:58:50 PST 2006
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"
More information about the freebsd-current
mailing list