Messages in this thread | | | Date | Mon, 9 Oct 2023 21:02:32 +0530 | Subject | Re: [PATCH 2/2] usb: gadget: ncm: Add support to update wMaxSegmentSize via configfs | From | Krishna Kurapati PSSNV <> |
| |
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@quicinc.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,
| |