lkml.org 
[lkml]   [2023]   [Feb]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [RFC] usb: gadget: ncm: Add support to configure wMaSegmentSize
From


On 2/14/2023 6:57 AM, Krishna Kurapati PSSNV wrote:
>
>
> On 2/14/2023 3:17 AM, Maciej Żenczykowski wrote:
>> This isn't a review per say - just some loose comments.
>>
>> On Sun, Feb 12, 2023 at 9:57 AM Krishna Kurapati
>> <quic_kriskura@quicinc.com> wrote:
>>> Currently the NCM driver restricts wMasxSegmentSize that indicates
>>
>> there's a number of spelling mistakes, here, and in the commit title
>>
>>> the datagram size coming from network layer to 1514. However the
>>> spec doesn't have any limitation.
>>>
>>> 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 8192 bytes so that
>>> at least 2 packets can be aggregated.
>>
>> I've experimented with increasing mtu to boost performance before
>> (have some half-baked patches/scripts somewhere).
>> And while it did improve point-to-point performance, it wasn't
>> actually useful for any real world use cases,
>> as internet mtu is simply never above 1500.
>>
>> Note that you cannot simply receive, aggregate (lro/gro) and forward
>> aggregated packets without splitting them back up.
> One more query on this:
>
> Would this problem come up if the MTU and segment size are consistent as
> well ? i.e., if the datagrams present in NTB are  of size <= MTU so that
> they can be given back safely to network stack without any further
> aggregation/splitup before wrap/unwrap call at usb layer ?
>
> Regards,
> Krishna,
>
Hi Maciej Żenczykowski,

Any thoughts on this ?
I believe if we modify the ncm_opts->net->mtu to the datagram
length (excluding the header) before registering net device in bind
call, we wouldn't need to split/aggregate obtained data before giving it
to network layer right as they would definitely be of size <= MTU of the
net device ? I tried it out and no issue came up in iperf test.

Thanks,
Krishna,
>>
>> A change like this to be useful would require negotiating some sort of
>> gso capabilities between the two devices
>> (and thus extending the NCM standard).  I've been meaning to do
>> this... but time...
>>
>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>>> ---
>>>   drivers/usb/gadget/function/f_ncm.c | 55 +++++++++++++++++++++++++++++
>>>   drivers/usb/gadget/function/u_ncm.h |  1 +
>>>   2 files changed, 56 insertions(+)
>>>
>>> diff --git a/drivers/usb/gadget/function/f_ncm.c
>>> b/drivers/usb/gadget/function/f_ncm.c
>>> index 424bb3b666db..1969e276017f 100644
>>> --- a/drivers/usb/gadget/function/f_ncm.c
>>> +++ b/drivers/usb/gadget/function/f_ncm.c
>>> @@ -118,6 +118,12 @@ static inline unsigned ncm_bitrate(struct
>>> usb_gadget *g)
>>>   /* Delay for the transmit to wait before sending an unfilled NTB
>>> frame. */
>>>   #define TX_TIMEOUT_NSECS       300000
>>>
>>> +/*
>>> + * Currently the max NTB Buffer size is set to 16384. For atleast 2
>>> packets
>>> + * to be aggregated, the size of datagram must at max be 8192.
>>> + */
>>> +#define MAX_DATAGRAM_SIZE      8192
>>
>>  From what I recall, there's a fair bit of overhead, and 8192 x2
>> doesn't actually fit in 16384...
>> That said... is it reasonable to require 2 to fit?  why? what's wrong
>> with 15000?
>>
>>> +
>>>   #define FORMATS_SUPPORTED      (USB_CDC_NCM_NTB16_SUPPORTED |  \
>>>                                   USB_CDC_NCM_NTB32_SUPPORTED)
>>>
>>> @@ -1440,6 +1446,7 @@ static int ncm_bind(struct usb_configuration
>>> *c, struct usb_function *f)
>>>           */
>>>          if (!ncm_opts->bound) {
>>>                  mutex_lock(&ncm_opts->lock);
>>> +               ncm_opts->net->mtu = (ncm_opts->max_segment_size - 14);
>>
>> this should use a constant, ETH_HLEN probably
>>
>>>                  gether_set_gadget(ncm_opts->net, cdev->gadget);
>>>                  status = gether_register_netdev(ncm_opts->net);
>>>                  mutex_unlock(&ncm_opts->lock);
>>> @@ -1484,6 +1491,8 @@ static int ncm_bind(struct usb_configuration
>>> *c, struct usb_function *f)
>>>
>>>          status = -ENODEV;
>>>
>>> +       ecm_desc.wMaxSegmentSize = ncm_opts->max_segment_size;
>>> +
>>
>> Curious... wasn't this set previously?
>>
>>>          /* allocate instance-specific endpoints */
>>>          ep = usb_ep_autoconfig(cdev->gadget, &fs_ncm_in_desc);
>>>          if (!ep)
>>> @@ -1586,11 +1595,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);
>>> +}
>>> +
>>> +static ssize_t ncm_opts_max_segment_size_store(struct config_item
>>> *item,
>>> +                                               const char *page,
>>> size_t len)
>>> +{
>>> +       struct f_ncm_opts *opts = to_f_ncm_opts(item);
>>> +       int ret;
>>> +       u32 segment_size;
>>> +
>>> +       mutex_lock(&opts->lock);
>>> +       if (opts->refcnt) {
>>> +               ret = -EBUSY;
>>> +               goto out;
>>> +       }
>>> +
>>> +       ret = kstrtou32(page, 0, &segment_size);
>>> +       if (ret)
>>> +               goto out;
>>> +
>>> +       if (segment_size > MAX_DATAGRAM_SIZE) {
>>> +               ret = -EINVAL;
>>> +               goto out;
>>> +       }
>>> +
>>> +       opts->max_segment_size = segment_size;
>>> +       ret = len;
>>> +out:
>>> +       mutex_unlock(&opts->lock);
>>> +       return ret;
>>> +}
>>> +
>>> +CONFIGFS_ATTR(ncm_opts_, max_segment_size);
>>> +
>>>   static struct configfs_attribute *ncm_attrs[] = {
>>>          &ncm_opts_attr_dev_addr,
>>>          &ncm_opts_attr_host_addr,
>>>          &ncm_opts_attr_qmult,
>>>          &ncm_opts_attr_ifname,
>>> +       &ncm_opts_attr_max_segment_size,
>>>          NULL,
>>>   };
>>>
>>> @@ -1633,6 +1687,7 @@ static struct usb_function_instance
>>> *ncm_alloc_inst(void)
>>>                  kfree(opts);
>>>                  return ERR_CAST(net);
>>>          }
>>> +       opts->max_segment_size = cpu_to_le16(ETH_FRAME_LEN);
>>>          INIT_LIST_HEAD(&opts->ncm_os_desc.ext_prop);
>>>
>>>          descs[0] = &opts->ncm_os_desc;
>>> diff --git a/drivers/usb/gadget/function/u_ncm.h
>>> b/drivers/usb/gadget/function/u_ncm.h
>>> index 5408854d8407..fab99d997476 100644
>>> --- a/drivers/usb/gadget/function/u_ncm.h
>>> +++ b/drivers/usb/gadget/function/u_ncm.h
>>> @@ -31,6 +31,7 @@ struct f_ncm_opts {
>>>           */
>>>          struct mutex                    lock;
>>>          int                             refcnt;
>>> +       u32                             max_segment_size;
>>>   };
>>>
>>>   #endif /* U_NCM_H */
>>> --
>>> 2.39.0
>>>

\
 
 \ /
  Last update: 2023-03-27 00:27    [W:0.975 / U:0.168 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site