Messages in this thread | | | Date | Fri, 17 Feb 2023 20:10:33 +0530 | Subject | Re: [RFC] usb: gadget: ncm: Add support to configure wMaSegmentSize | From | Krishna Kurapati PSSNV <> |
| |
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 >>>
| |