Re: [RFC] usb: gadget: ncm: Add support to configure wMaSegmentSize
From: Krishna Kurapati PSSNV
Date: Fri Feb 17 2023 - 09:40:52 EST
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@xxxxxxxxxxx> 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@xxxxxxxxxxx>
---
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