> 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
>
> rick
>
>> _______________________________________________
>> freebsd-sta...@freebsd.org mailing list
>> https://lists.freebsd.org/mailman/listinfo/freebsd-stable
>> To unsubscribe, send any mail to "freebsd-stable-unsubscr...@freebsd.org"
>>
_______________________________________________
freebsd-net@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
