Re: trunk vs busy ports

[David Gwynne]

> On 20 Nov 2015, at 11:23 PM, Reyk Floeter  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.

how would you feel if i simply removed the check?

dlg

> 
> 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 -  1.124
>> +++ if_trunk.c   20 Nov 2015 05:35:07 -
>> @@ -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(>if_snd))
>>  return (EBUSY);
>> 
>>  /* Check if port has already been associated to a trunk */
> 
> -- 

Re: trunk vs busy ports

[Reyk Floeter]

On Fri, Nov 20, 2015 at 11:27:05PM +1000, David Gwynne wrote:
> 
> > On 20 Nov 2015, at 11:23 PM, Reyk Floeter  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 -  1.124
> >> +++ if_trunk.c 20 Nov 2015 05:35:07 -
> >> @@ -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(>if_snd))
> >>return (EBUSY);
> >> 
> >>/* Check if port has already been associated to a trunk */
> > 
> > -- 
> 

-- 

Re: trunk vs busy ports

[Hrvoje Popovski]

On 20.11.2015. 6:36, 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.


don't know if this is relevant other than mentioning IFF_OACTIVE :)

https://www.dragonflybsd.org/mailarchive/users/2013-01/msg0.html

Re: trunk vs busy ports

[Reyk Floeter]

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.

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.c20 Nov 2015 05:33:54 -  1.124
> +++ if_trunk.c20 Nov 2015 05:35:07 -
> @@ -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(>if_snd))
>   return (EBUSY);
>  
>   /* Check if port has already been associated to a trunk */

-- 

Re: trunk vs busy ports

[David Gwynne]

> On 20 Nov 2015, at 8:18 PM, Martin Pieuchot  wrote:
> 
> On 20/11/15(Fri) 15:36, David Gwynne wrote:
>> IFF_OACTIVE means the hardware ring is full, not if it is busy.
> 
> As discussed earlier, I'd still love to have a document (manpage?)
> describing these flags so we can agree on when to use them.
> 
>> perhaps a better check is to see whether there are pending packets
>> on the send queue?
> 
> What's the underlying problem here, checking for if_flags in ioctl()
> path?

one problem is the check doesn't mean what it is intended to mean, the other is 
i want to get rid of IFF_OACTIVE.

> 
>> i could also argue we dont need the check at all, but this is less
>> of a semantic change.
> 
> I'm also wondering why do we need this check at all.

i dont see why it matters if we wait for pending packets to send before joining 
it to a trunk, or just join it to a trunk and let the pending packets leave.

there's no consequence to having a slightly wrong temporal ordering.

Re: trunk vs busy ports

[Hrvoje Popovski]

On 20.11.2015. 13:26, Hrvoje Popovski wrote:
> On 20.11.2015. 6:36, 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.
> 
> 
> don't know if this is relevant other than mentioning IFF_OACTIVE :)
> 
> https://www.dragonflybsd.org/mailarchive/users/2013-01/msg0.html
> 


by relevant i meant interesting :)

Re: trunk vs busy ports

[Martin Pieuchot]

On 20/11/15(Fri) 15:36, David Gwynne wrote:
> IFF_OACTIVE means the hardware ring is full, not if it is busy.

As discussed earlier, I'd still love to have a document (manpage?)
describing these flags so we can agree on when to use them.

> perhaps a better check is to see whether there are pending packets
> on the send queue?

What's the underlying problem here, checking for if_flags in ioctl()
path?

> i could also argue we dont need the check at all, but this is less
> of a semantic change.

I'm also wondering why do we need this check at all.

Re: trunk vs busy ports

[sven falempin]

On Fri, Nov 20, 2015 at 9:00 AM, Reyk Floeter  wrote:

> On Fri, Nov 20, 2015 at 11:27:05PM +1000, David Gwynne wrote:
> >
> > > On 20 Nov 2015, at 11:23 PM, Reyk Floeter  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 -  1.124
> > >> +++ if_trunk.c 20 Nov 2015 05:35:07 -
> > >> @@ -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(>if_snd))
> > >>return (EBUSY);
> > >>
> > >>/* Check if port has already been associated to a trunk */
> > >
> > > --
> >
>
> --
>
>
I have a trunk usage currently, and when the trunk is created, the first
data to go through
suffer a very high latency.

After less than 5 seconds, everything runs smoothly (same latency as each
link)

Could it be related ?


-- 
-
() ascii ribbon campaign - against html e-mail
/\