[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <a9efdc23-0417-48dc-aa17-ef7b1459b85a@quicinc.com>
Date: Mon, 9 Oct 2023 21:02:32 +0530
From: Krishna Kurapati PSSNV <quic_kriskura@...cinc.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC: Maciej Żenczykowski <maze@...gle.com>,
onathan Corbet <corbet@....net>,
Linyu Yuan <quic_linyyuan@...cinc.com>,
<linux-usb@...r.kernel.org>, <linux-doc@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <quic_ppratap@...cinc.com>,
<quic_wcheng@...cinc.com>, <quic_jackp@...cinc.com>
Subject: Re: [PATCH 2/2] usb: gadget: ncm: Add support to update
wMaxSegmentSize via configfs
On 10/9/2023 8:38 PM, Greg Kroah-Hartman wrote:
> On Mon, Oct 09, 2023 at 07:50:05PM +0530, Krishna Kurapati wrote:
>> Currently the NCM driver restricts wMaxSegmentSize that indicates
>> the datagram size coming from network layer to 1514.
>
> I don't see that restriction in the existing driver, where does that
> happen?
Hi Greg,
In the ecm_desc, the following line restricts the value:
.wMaxSegmentSize = cpu_to_le16(ETH_FRAME_LEN),
>
>> However the spec doesn't have any limitation.
>
> What spec?
NCM Specification.
I didn't mention "NCM specification" as I thought the patch header would
imply it is NCM Spec. Will update the wording here.
>
>> For P2P connections over NCM, increasing MTU helps increasing
>> throughput.
>
> While increasing latency, right?
I used iperf for capturing the data. I was more focussing on throughput.
Here are some results I found:
For throughput, the increase is as follows (HS link with and an old iperf):
MTU Size Throughput
1500 145
2500 204
3500 208
4500 223
5500 227
6500 299
7500 324
8050 335
As per the latency, an internal test application I was using didn't show
much difference in latency as MTU was increasing. Then again, it could
be application specific.
>
>> Add support to configure this value before configfs symlink is
>> created. Also since the NTB Out/In buffer sizes are fixed at 16384
>> bytes, limit the segment size to an upper cap of 15014. Set the
>> default MTU size for the ncm interface during function bind before
>> network interface is registered allowing MTU to be set in parity
>> with wMaxSegmentSize.
>
> Where does 15014 come from?
>
>>
>> Signed-off-by: Krishna Kurapati <quic_kriskura@...cinc.com>
>> ---
>> drivers/usb/gadget/function/f_ncm.c | 51 +++++++++++++++++++++++++++++
>> drivers/usb/gadget/function/u_ncm.h | 2 ++
>> 2 files changed, 53 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
>> index feccf4c8cc4f..eab297b22200 100644
>> --- a/drivers/usb/gadget/function/f_ncm.c
>> +++ b/drivers/usb/gadget/function/f_ncm.c
>> @@ -103,6 +103,8 @@ static inline struct f_ncm *func_to_ncm(struct usb_function *f)
>> /* Delay for the transmit to wait before sending an unfilled NTB frame. */
>> #define TX_TIMEOUT_NSECS 300000
>>
>> +#define MAX_DATAGRAM_SIZE 15014
>
> Where does this magic value come from? Please document it really really
> well.
>
Sorry for not being clear. The 14 here meant ETH_HLEN which gets
appended to MTU usually. I wanted to limit mtu to 15000 max (via
wMaxsegment size) and mentioned it here as 15014.
Will update comments here in v2.
>> +
>> #define FORMATS_SUPPORTED (USB_CDC_NCM_NTB16_SUPPORTED | \
>> USB_CDC_NCM_NTB32_SUPPORTED)
>>
>> @@ -1408,6 +1410,7 @@ static int ncm_bind(struct usb_configuration *c, struct usb_function *f)
>> ncm_opts = container_of(f->fi, struct f_ncm_opts, func_inst);
>>
>> if (cdev->use_os_string) {
>> + ncm_opts->net->mtu = (ncm_opts->max_segment_size - ETH_HLEN);
>> f->os_desc_table = kzalloc(sizeof(*f->os_desc_table),
>> GFP_KERNEL);
>> if (!f->os_desc_table)
>> @@ -1469,6 +1472,8 @@ static int ncm_bind(struct usb_configuration *c, struct usb_function *f)
>>
>> status = -ENODEV;
>>
>> + ecm_desc.wMaxSegmentSize = ncm_opts->max_segment_size;
>> +
>> /* allocate instance-specific endpoints */
>> ep = usb_ep_autoconfig(cdev->gadget, &fs_ncm_in_desc);
>> if (!ep)
>> @@ -1569,11 +1574,56 @@ USB_ETHERNET_CONFIGFS_ITEM_ATTR_QMULT(ncm);
>> /* f_ncm_opts_ifname */
>> USB_ETHERNET_CONFIGFS_ITEM_ATTR_IFNAME(ncm);
>>
>> +static ssize_t ncm_opts_max_segment_size_show(struct config_item *item,
>> + char *page)
>> +{
>> + struct f_ncm_opts *opts = to_f_ncm_opts(item);
>> + u32 segment_size;
>> +
>> + mutex_lock(&opts->lock);
>> + segment_size = opts->max_segment_size;
>> + mutex_unlock(&opts->lock);
>> +
>> + return sprintf(page, "%u\n", segment_size);
>
> sysfs_emit()?
>
Apologies.
I followed u_ether_configfs.h which used sprintf. Will replace it with
sysfs_emit in v2.
Thanks for the review.
Regards,
Krishna,
Powered by blists - more mailing lists