Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Message-ID: <70c15867-ccce-4788-a0dd-38a73decb785@quicinc.com>
Date:   Sat, 14 Oct 2023 12:32:59 +0530
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 2/2] usb: gadget: ncm: Add support to update
 wMaxSegmentSize via configfs
To:     =?UTF-8?Q?Maciej_=C5=BBenczykowski?= <maze@google.com>
CC:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        onathan Corbet <corbet@lwn.net>,
        Linyu Yuan <quic_linyyuan@quicinc.com>,
        <linux-usb@vger.kernel.org>, <linux-doc@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <quic_ppratap@quicinc.com>,
        <quic_wcheng@quicinc.com>, <quic_jackp@quicinc.com>
References: <20231009142005.21338-1-quic_kriskura@quicinc.com>
 <20231009142005.21338-2-quic_kriskura@quicinc.com>
 <CANP3RGfEk2DqZ3biyN78ycQYbDxCEG+H1me2vnEYuwXkNdXnTA@mail.gmail.com>
 <CANP3RGcCpNOuVpdV9n0AFxZo-wsfwi8OfYgBk1WHNHaEd-4V-Q@mail.gmail.com>
 <CANP3RGdY4LsOA6U5kuccApHCzL0_jBnY=pLOYrUuYtMZFTvnbw@mail.gmail.com>
 <d19d9d08-c119-4991-b460-49925f601d15@quicinc.com>
 <fad5a7fb-cce1-46bc-a0af-72405c76d107@quicinc.com>
 <CANP3RGcqWBYd9FqAX47rE9pFgBTB8=0CGdwkScm-OH1epHcVWQ@mail.gmail.com>
 <8ff92053-52ff-4950-95c8-0e986f6a028a@quicinc.com>
 <CANP3RGd4G4dkMOyg6wSX29NYP2mp=LhMhmZpoG=rgoCz=bh1=w@mail.gmail.com>
 <b12eb7b1-54e7-406f-8c19-0046555b82d3@quicinc.com>
 <CANP3RGcUrFTaFL8V3tpuh+qQoEi84O0Dy9ie+XD=-H01c2btAw@mail.gmail.com>
Content-Language: en-US
From:   Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>
In-Reply-To: <CANP3RGcUrFTaFL8V3tpuh+qQoEi84O0Dy9ie+XD=-H01c2btAw@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 10/14/2023 4:05 AM, Maciej Żenczykowski wrote:
>>>> The intent of posting the diff was two fold:
>>>>
>>>> 1. The question Greg asked regarding why the max segment size was
>>>> limited to 15014 was valid. When I thought about it, I actually wanted
>>>> to limit the max MTU to 15000, so the max segment size automatically
>>>> needs to be limited to 15014.
>>>
>>> Note that this is a *very* abstract value.
>>> I get you want L3 MTU of 10 * 1500, but this value is not actually meaningful.
>>>
>>> IPv4/IPv6 fragmentation and IPv4/IPv6 TCP segmentation
>>> do not result in a trivial multiplication of the standard 1500 byte
>>> ethernet L3 MTU.
>>> Indeed aggregating 2 1500 L3 mtu frames results in *different* sized
>>> frames depending on which type of aggregation you do.
>>> (and for tcp it even depends on the number and size of tcp options,
>>> though it is often assumed that those take up 12 bytes, since that's the
>>> normal for Linux-to-Linux tcp connections)
>>>
>>> For example if you aggregate N standard Linux ipv6/tcp L3 1500 mtu frames,
>>> this means you have
>>> N frames: ethernet (14) + ipv6 (40) + tcp (20) + tcp options (12) +
>>> payload (1500-12-20-40=1500-72=1428)
>>> post aggregation:
>>> 1 frame: ethernet (14) + ipv6 (40) + tcp (20) + tcp options (12) +
>>> payload (N*1428)
>>>
>>> so N * 1500 == N * (72 + 1428) --> 1 * (72 + N * 1428)
>>>
>>> That value of 72 is instead 52 for 'standard Linux ipv4/tcp),
>>> it's 40/60 if there's no tcp options (which I think happens when
>>> talking to windows)
>>> it's different still with ipv4 fragmentation... and again different
>>> with ipv6 fragmentation...
>>> etc.
>>>
>>> ie. 15000 L3 mtu is exactly as meaningless as 14000 L3 mtu.
>>> Either way you don't get full frames.
>>>
>>> As such I'd recommend going with whatever is the largest mtu that can
>>> be meaningfully made to fit in 16K with all the NCM header overhead.
>>> That's likely closer to 15500-16000 (though I have *not* checked).
>>>
>>>> But my commit text didn't mention this
>>>> properly which was a mistake on my behalf. But when I looked at the
>>>> code, limiting the max segment size 15014 would force the practical
>>>> max_mtu to not cross 15000 although theoretical max_mtu was set to:
>>>> (GETHER_MAX_MTU_SIZE - 15412) during registration of net device.
>>>>
>>>> So my assumption of limiting it to 15000 was wrong. It must be limited
>>>> to 15412 as mentioned in u_ether.c  This inturn means we must limit
>>>> max_segment_size to:
>>>> GETHER_MAX_ETH_FRAME_LEN (GETHER_MAX_MTU_SIZE + ETH_HLEN)
>>>> as mentioned in u_ether.c.
>>>>
>>>> I wanted to confirm that setting MAX_DATAGRAM_SIZE to
>>>> GETHER_MAX_ETH_FRAME_LEN was correct.
>>>>
>>>> 2. I am not actually able to test with MTU beyond 15000. When my host
>>>> device is a linux machine, the cdc_ncm.c limits max_segment_size to:
>>>> CDC_NCM_MAX_DATAGRAM_SIZE               8192    /* bytes */
>>>
>>> In practice you get 50% of the benefits of infinitely large mtu by
>>> going from 1500 to ~2980.
>>> you get 75% of the benefits by going to ~6K
>>> you get 87.5% of the benefits by going to ~12K
>>> the benefits of going even higher are smaller and smaller...
>>>   > If the host side is limited to 8192, maybe we should match that here too?
>>
>> Hi Maciej,
>>
>>    Thanks for the detailed explanation. I agree with you on setting
>> device side also to 8192 instead of what max_mtu is present in u_ether
>> or practical max segment size possible.
>>
>>>
>>> But the host side limitation of 8192 doesn't seem particularly sane either...
>>> Maybe we should relax that instead?
>>>
>> I really didn't understand why it was set to 8192 in first place.
>>
>>> (especially since for things like tcp zero copy you want an mtu which
>>> is slighly more then N * 4096,
>>> ie. around 4.5KB, 8.5KB, 12.5KB or something like that)
>>>
>>
>> I am not sure about host mode completely. If we want to increase though,
>> just increasing the MAX_DATAGRAM_SIZE to some bigger value help ? (I
>> don't know the entire code of cdc_ncm, so I might be wrong).
>>
>> Regards,
>> Krishna,
> 
> Hmm, I'm not sure.  I know I've experimented with high mtu ncm in the past
> (around 2.5 years ago).  I got it working between my Linux desktop (host)
> and a Pixel 6 (device/gadget) with absolutely no problems.
> 
> I'm pretty sure I didn't change my desktop kernel, so I was probably
> limited to 8192 there
> (and I do more or less remember that).
>  From what I vaguely remember, it wasn't difficult (at all) to hit
> upwards of 7gbps for iperf tests.
> I don't remember how close to the theoretical USB 10gbps maximum of
> 9.7gbps I could get...
> [this was never the real bottleneck / issue, so I didn't ever dig
> particularly deep]
> 
> I'm pretty sure my gadget side changes were non-configurable...
> Probably just bumped one or two constants...
> 
Could you share what parameters you changed to get this high value of 
iperf throughput.

> I do *very* *vaguely* recall there being some funkiness though, where 8192 was
> *less* efficient than some slightly smaller value.
> 
> If I recall correctly the issue is that 8192 + ethernet overhead + NCM
> overhead only fits *once* into 16384, which leaves a lot of space
> wasted.
> While ~7.5 kb + overhead fits twice and is thus a fair bit better.
>Right, same goes for using 5K vs 5.5K MTU. If MTU is 5K, 3 packets can 
conveniently fit into an NTB but if its 5.5, at max only two (5.5k) 
packets can fit in (essentially filling ~11k of the 16384 bytes and 
wasting the rest)

And whether its Ipv4/Ipv6 like you mentioned on [1], the MTU is what NCM 
layer receives and we append the Ethernet header and add NCM headers and 
send it out after aggregation. Why can't we set the MAX_DATAGRAM_SIZE to 
~8050 or ~8100 ? The reason I say this value is, obviously setting it to 
8192 would not efficiently use the NTB buffer. We need to fill as much 
space in buffer as possible and assuming that each packet received on 
ncm layer is of MTU size set (not less that that), we can assume that 
even if only 2 packets are aggregated (minimum aggregation possible), we 
would be filling (2 * (8050 + ETH_HLEN) + (room for NCM headers)) would 
almost be close to 16384 ep max packet size. I already check 8050 MTU 
and it works. We can add a comment in code detailing the above 
explanation and why we chose to use 8050 or 8100 as MAX_DATAGRAM_SIZE.

Hope my reasoning of why we can chose 8.1K or 8.05K makes sense. Let me 
know your thoughts on this.

Regards,
Krishna,