Daniel Braniss wrote:
> 
> > On 22 Aug 2015, at 14:59, Rick Macklem <rmack...@uoguelph.ca> wrote:
> > 
> > Daniel Braniss wrote:
> >> 
> >>> On Aug 22, 2015, at 12:46 AM, Rick Macklem <rmack...@uoguelph.ca> wrote:
> >>> 
> >>> Yonghyeon PYUN wrote:
> >>>> On Wed, Aug 19, 2015 at 09:00:35AM -0400, Rick Macklem wrote:
> >>>>> Hans Petter Selasky wrote:
> >>>>>> On 08/19/15 09:42, Yonghyeon PYUN wrote:
> >>>>>>> On Wed, Aug 19, 2015 at 09:00:52AM +0200, Hans Petter Selasky wrote:
> >>>>>>>> On 08/18/15 23:54, Rick Macklem wrote:
> >>>>>>>>> Ouch! Yes, I now see that the code that counts the # of mbufs is
> >>>>>>>>> before
> >>>>>>>>> the
> >>>>>>>>> code that adds the tcp/ip header mbuf.
> >>>>>>>>> 
> >>>>>>>>> In my opinion, this should be fixed by setting if_hw_tsomaxsegcount
> >>>>>>>>> to
> >>>>>>>>> whatever
> >>>>>>>>> the driver provides - 1. It is not the driver's responsibility to
> >>>>>>>>> know if
> >>>>>>>>> a tcp/ip
> >>>>>>>>> header mbuf will be added and is a lot less confusing that
> >>>>>>>>> expecting
> >>>>>>>>> the
> >>>>>>>>> driver
> >>>>>>>>> author to know to subtract one. (I had mistakenly thought that
> >>>>>>>>> tcp_output() had
> >>>>>>>>> added the tc/ip header mbuf before the loop that counts mbufs in
> >>>>>>>>> the
> >>>>>>>>> list.
> >>>>>>>>> Btw,
> >>>>>>>>> this tcp/ip header mbuf also has leading space for the MAC layer
> >>>>>>>>> header.)
> >>>>>>>>> 
> >>>>>>>> 
> >>>>>>>> Hi Rick,
> >>>>>>>> 
> >>>>>>>> Your question is good. With the Mellanox hardware we have separate
> >>>>>>>> so-called inline data space for the TCP/IP headers, so if the TCP
> >>>>>>>> stack
> >>>>>>>> subtracts something, then we would need to add something to the
> >>>>>>>> limit,
> >>>>>>>> because then the scatter gather list is only used for the data part.
> >>>>>>>> 
> >>>>>>> 
> >>>>>>> I think all drivers in tree don't subtract 1 for
> >>>>>>> if_hw_tsomaxsegcount.  Probably touching Mellanox driver would be
> >>>>>>> simpler than fixing all other drivers in tree.
> >>>>>>> 
> >>>>>>>> Maybe it can be controlled by some kind of flag, if all the three
> >>>>>>>> TSO
> >>>>>>>> limits should include the TCP/IP/ethernet headers too. I'm pretty
> >>>>>>>> sure
> >>>>>>>> we want both versions.
> >>>>>>>> 
> >>>>>>> 
> >>>>>>> Hmm, I'm afraid it's already complex.  Drivers have to tell almost
> >>>>>>> the same information to both bus_dma(9) and network stack.
> >>>>>> 
> >>>>>> Don't forget that not all drivers in the tree set the TSO limits
> >>>>>> before
> >>>>>> if_attach(), so possibly the subtraction of one TSO fragment needs to
> >>>>>> go
> >>>>>> into ip_output() ....
> >>>>>> 
> >>>>> Ok, I realized that some drivers may not know the answers before
> >>>>> ether_ifattach(),
> >>>>> due to the way they are configured/written (I saw the use of
> >>>>> if_hw_tsomax_update()
> >>>>> in the patch).
> >>>> 
> >>>> I was not able to find an interface that configures TSO parameters
> >>>> after if_t conversion.  I'm under the impression
> >>>> if_hw_tsomax_update() is not designed to use this way.  Probably we
> >>>> need a better one?(CCed to Gleb).
> >>>> 
> >>>>> 
> >>>>> If it is subtracted as a part of the assignment to if_hw_tsomaxsegcount
> >>>>> in
> >>>>> tcp_output()
> >>>>> at line#791 in tcp_output() like the following, I don't think it should
> >>>>> matter if the
> >>>>> values are set before ether_ifattach()?
> >>>>>                         /*
> >>>>>                          * Subtract 1 for the tcp/ip header mbuf that
> >>>>>                          * will be prepended to the mbuf chain in this
> >>>>>                          * function in the code below this block.
> >>>>>                          */
> >>>>>                         if_hw_tsomaxsegcount = tp->t_tsomaxsegcount - 1;
> >>>>> 
> >>>>> I don't have a good solution for the case where a driver doesn't plan
> >>>>> on
> >>>>> using the
> >>>>> tcp/ip header provided by tcp_output() except to say the driver can add
> >>>>> one
> >>>>> to the
> >>>>> setting to compensate for that (and if they fail to do so, it still
> >>>>> works,
> >>>>> although
> >>>>> somewhat suboptimally). When I now read the comment in sys/net/if_var.h
> >>>>> it
> >>>>> is clear
> >>>>> what it means, but for some reason I didn't read it that way before? (I
> >>>>> think it was
> >>>>> the part that said the driver didn't have to subtract for the headers
> >>>>> that
> >>>>> confused me?)
> >>>>> In any case, we need to try and come up with a clear definition of what
> >>>>> they need to
> >>>>> be set to.
> >>>>> 
> >>>>> I can now think of two ways to deal with this:
> >>>>> 1 - Leave tcp_output() as is, but provide a macro for the device driver
> >>>>> authors to use
> >>>>>   that sets if_hw_tsomaxsegcount with a flag for "driver uses tcp/ip
> >>>>>   header mbuf",
> >>>>>   documenting that this flag should normally be true.
> >>>>> OR
> >>>>> 2 - Change tcp_output() as above, noting that this is a workaround for
> >>>>> confusion w.r.t.
> >>>>>   whether or not if_hw_tsomaxsegcount should include the tcp/ip header
> >>>>>   mbuf and
> >>>>>   update the comment in if_var.h to reflect this. Then drivers that
> >>>>>   don't
> >>>>>   use the
> >>>>>   tcp/ip header mbuf can increase their value for if_hw_tsomaxsegcount
> >>>>>   by
> >>>>>   1.
> >>>>>   (The comment should also mention that a value of 35 or greater is
> >>>>>   much
> >>>>>   preferred to
> >>>>>    32 if the hardware will support that.)
> >>>>> 
> >>>> 
> >>>> Both works for me.  My preference is 2 just because it's very
> >>>> common for most drivers that use tcp/ip header mbuf.
> >>> Thanks for this comment. I tend to agree, both for the reason you state
> >>> and
> >>> also
> >>> because the patch is simple enough that it might qualify as an errata for
> >>> 10.2.
> >>> 
> >>> I am hoping Daniel Braniss will be able to test the patch and let us know
> >>> if it
> >>> improves performance with TSO enabled?
> >> 
> >> send me the patch and I’ll test it ASAP.
> >>    danny
> >> 
> > Patch is attached. The one for head will also include an update to the
> > comment
> > in sys/net/if_var.h, but that isn't needed for testing.
> 
> 
> well, the plot thickens.
> 
> Yesterday, before running the new kernel, I decided to re run my test, and to
> my surprise
> i was getting good numbers, about 300MGB/s with and without TSO.
> 
> this morning, the numbers were again bad, around 70MGB/s,what the ^%$#@!
> 
> so, after some coffee, I run some more tests, and some conclusions:
> using a netapp(*) as the nfs client:
>   - doing
>       ifconfig ix0 tso or -tso
>     does some magic and numbers are back to normal - for a while
> 
> using another Fbsd/zfs as client all is nifty, actually a bit faster than the
> netapp (not a fair
> comparison, since the zfs client is not heavily used) and I can’t see any
> degradation.
>  
I assume you meant "server" and not "client" above.

> btw, this is with the patch applied, but was seeing similar numbers before
> the patch.
> 
> running with tso, initially I get around 300MGB/s, but after a while(sorry
> can’t be more scientific)
> it drops down to about half,  and finally to a pathetic 70MGB/s
> 
Ok, so it sounds like tso isn't the issue. (At least it seems the patch,
which I believe is needed, doesn't cause a regression.)

All I can suggest is:
- looking at the ix stats (I know nothing about them), but if you post them
  maybe someone conversant with the chip can help? (Before and after 
degredation.)
- if you captured packets for a short period of time when degraded and then
  after doing "ifconfig", looking at the packet capture in wireshark might give
  some indication of what changes?
  - For this I'd be focused on the TCP layer (window sizes, etc) and timing of
    packets.
--> I don't know if there is a packet capture tool like tcpdump on a Netapp, but
    that might be better than capturing them on the client, in case tcpdump 
affects
    the outcome. However, tcpdump run on the client would be a fallback, I 
think.

The other thing is the degradation seems to cut the rate by about half each 
time.
300-->150-->70 I have no idea if this helps to explain it.

Have fun with it, rick

> *: while running the tests I monitored the Netapp, and nothing out of the
> ordinary there.
> 
> cheers,
>       danny
> 
> _______________________________________________
> freebsd-stable@freebsd.org mailing list
> https://lists.freebsd.org/mailman/listinfo/freebsd-stable
> To unsubscribe, send any mail to "freebsd-stable-unsubscr...@freebsd.org"
_______________________________________________
freebsd-stable@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-stable
To unsubscribe, send any mail to "freebsd-stable-unsubscr...@freebsd.org"

Reply via email to