cvs commit: src/sys/sys mbuf.h src/sys/net if_ethersubr.c src/sys/dev/mxge mxge_lro.c

[Jack Vogel]

On 6/11/07, Andre Oppermann <andre at freebsd.org> wrote:
> Andrew Gallatin wrote:
> > Andre Oppermann writes:
> >  > Scott Long wrote:
> >  > > Andre Oppermann wrote:
> >  > >> Andrew Gallatin wrote:
> >  > >>> gallatin    2007-06-11 14:59:56 UTC
> >  > >>>
> >  > >>>   FreeBSD src repository
> >  > >>>
> >  > >>>   Modified files:
> >  > >>>     sys/sys              mbuf.h     sys/net
> >  > >>> if_ethersubr.c     sys/dev/mxge         mxge_lro.c   Log:
> >  > >>>   Allow drivers, such as cxgb and mxge, which support LRO to bypass
> >  > >>>   the MTU check in ether_input() on LRO merged frames.
> >  > >>>     Discussed with: kmacy
> >  > >> Not discussed with: andre
> >  > >>
> >  > >> Your change isn't the right way to make this work.  LRO is an interface
> >  > >> capability (that should have the option to disable it) and the test in
> >  > >> ether_input() should go on that instead.  LRO is not an information
> >  > >> that is needed beyond ether_input() and thus doesn't have to be a mbuf
> >  > >> flag.
> >  > >>
> >  > >> I've indicated that I'm working in this area as well and at least
> >  > >> dropping an email or a ping IRC would have been nice.  I would have
> >  > >> told you the above right away.  My common version of LRO isn't ready
> >  > >> yet as I'm a bit short on time and I chose to concentrate on TCP it-
> >  > >> self.  We only have to make sure that we don't exclude a common LRO
> >  > >> implementation due to API/ABI issues for 7.1R.
> >  > >>
> >  > >
> >  > > Drew's commit looks simple and non-obtrusive enough that it can likely
> >  > > be replaced once your perfected LRO implementation is done and in the
> >  > > tree.  Until that happens, I can't imagine a good reason to block his
> >  > > and Kip's work.
> >  >
> >  > I'm blocking nothing.  Read again what I wrote.  I complained about
> >  > a technically wrong approach to solve a particular side problem.  In
> >  > the meantime it got backed out because someone else complained too.
> >
> > I specifically *didn't* make it an interface flag *because* of your
> > promised generic LRO support.  Ie, I didn't want to have any standard,
> > documented interface to carry around for what is nothing more than a
> > temporary hack to allow 10GbE with standard frames to not suck on
> > FreeBSD in the short term.  Per-driver LRO support is a nightmare that
> > should be ripped out when your generic support is ready.  I'm really
> > looking forward to your work, and tried to keep the per-driver stuff
> > as unobtrusive as possible.
>
> LRO actually should become an interface capability flag just like
> TSO so we can turn it individually on/off for every interface.  LRO
> must be done inside the driver, even the generic one.  Having the
> flag doesn't interfere with the common implementation drivers should
> use in the future when it's ready.  We should do the flag right now
> to have it included in the 7.0 API/ABI.
>
> > As to the other complaint, Sam complained about the scarcity of mbuf
> > flags, not about LRO in general.  We agreed that, since its basically
> > a temporary hack, the best thing to do would be to move the frame
> > length check under DIAGNOSTIC, where it should be anyway.
>
> I didn't (want to) complain about your LRO either.  I'm fine with it.
> Just it shouldn't have used the mbuf flag as Sam said too but for
> different reasons.
>
> > FWIW, LRO triples receive performance for standard frames (3.xGb/s ->
> > 9.3Gb/s) on decent hardware.
>
> Nice to see that.  The problem with LRO at the moment is that it only
> works on short RTT links (<1ms) because the TCP stack doesn't do ABC
> yet and growing the send window with a LRO receiver is going to be
> painfully slow as the RTT goes up.
>
> Lets add the interface capabilities flag for LRO including the ifconfig
> support and be done with this episode.

I agree with this approach also.

Jack