Message-ID: <6d26bec0-689c-2130-7e95-ff5fd2383de3@quicinc.com>
Date:   Fri, 17 Feb 2023 20:10:33 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.12.0
Subject: Re: [RFC] usb: gadget: ncm: Add support to configure wMaSegmentSize
Content-Language: en-US
From:   Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>
To:     =?UTF-8?Q?Maciej_=c5=bbenczykowski?= <maze@google.com>
CC:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        <linux-usb@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <quic_ppratap@quicinc.com>, <quic_jackp@quicinc.com>
References: <20230212175659.4480-1-quic_kriskura@quicinc.com>
 <CANP3RGerSsUPhzdExrok_sc2jst2firjsEkaLepHDgt_sFmO4w@mail.gmail.com>
 <57a5133d-4eeb-7c0b-e4ad-fb10d60157e6@quicinc.com>
In-Reply-To: <57a5133d-4eeb-7c0b-e4ad-fb10d60157e6@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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
>>>