On Fri, Nov 20, 2015 at 11:27:05PM +1000, David Gwynne wrote: > > > On 20 Nov 2015, at 11:23 PM, Reyk Floeter <r...@openbsd.org> wrote: > > > > On Fri, Nov 20, 2015 at 03:36:40PM +1000, David Gwynne wrote: > >> IFF_OACTIVE means the hardware ring is full, not if it is busy. > >> > >> perhaps a better check is to see whether there are pending packets > >> on the send queue? > >> > >> i could also argue we dont need the check at all, but this is less > >> of a semantic change. > >> > >> ok? > >> > > > > OK > > > > I added this check in the initial commit of trunk(4) more than 10 > > years ago. I don't remember a particular reason - there wasn't even a > > production use yet. I initially wrote trunk to use it for failover on > > some firewalls, but it was not going into production before it was > > committed to OpenBSD. > > > > So the reason for the flag might just be a historical one: back in the > > days, I heard that 10 years is a long time in IT, there was not much > > reference about implementing such a "cloner" correctly. I must have > > looked at vlan(4) or bridge(4) and decided that it was the way to do. > > I mean, before mpi@, working in the network stack was tough and very > > different: you didn't ask, never refered to any documentation, just > > relied on the traditions and repeated what was done by the BSD heroes > > in the past. > > thats exactly right. please dont take this as an accusation of negligence, > this is just mopping up some of that inherited code. > > im upset about my own use of IFQ_POLL() in a bunch of my own drivers, but > really i am only guilty of what you described above too. ie, we're not > guilty, we were just following best practices at the time. >
Hehe, i'm not feeling accused. I think it is a lot of fun to see all the progress and cleanup happening. I just wanted to tell a story ... and to point out that some of the obvious mistakes of the past weren't so obvious to us back then. > how would you feel if i simply removed the check? > fine! Reyk > >> Index: if_trunk.c > >> =================================================================== > >> RCS file: /cvs/src/sys/net/if_trunk.c,v > >> retrieving revision 1.124 > >> diff -u -p -r1.124 if_trunk.c > >> --- if_trunk.c 20 Nov 2015 05:33:54 -0000 1.124 > >> +++ if_trunk.c 20 Nov 2015 05:35:07 -0000 > >> @@ -296,7 +296,7 @@ trunk_port_create(struct trunk_softc *tr > >> return (ENOSPC); > >> > >> /* New trunk port has to be in an idle state */ > >> - if (ifp->if_flags & IFF_OACTIVE) > >> + if (!ifq_empty(&ifp->if_snd)) > >> return (EBUSY); > >> > >> /* Check if port has already been associated to a trunk */ > > > > -- > --
