Re: [PATCH 2/2] usb: gadget: ncm: Add support to update wMaxSegmentSize via configfs
From: Krishna Kurapati PSSNV
Date: Mon Oct 09 2023 - 11:32:48 EST
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@xxxxxxxxxxx>
---
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,