Re: [RFC] usb: gadget: ncm: Add support to configure wMaSegmentSize

[Krishna Kurapati PSSNV <quic_kriskura@xxxxxxxxxxx>]

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