Date:   Wed, 9 Sep 2020 23:10:39 +0200
From:   Daniele Palmas <dnlplm@...il.com>
To:     Bjørn Mork <bjorn@...k.no>
Cc:     Carl Yin(殷张成) <carl.yin@...ctel.com>,
        Kristian Evensen <kristian.evensen@...il.com>,
        Paul Gildea <paul.gildea@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>
Subject: Re: [PATCH net-next 1/1] net: usb: qmi_wwan: add default rx_urb_size

Hi Bjørn,

Il giorno mer 9 set 2020 alle ore 14:49 Bjørn Mork <bjorn@...k.no> ha scritto:
>
> Daniele Palmas <dnlplm@...il.com> writes:
> > Il giorno mer 9 set 2020 alle ore 13:09 Carl Yin(殷张成)
> > <carl.yin@...ctel.com> ha scritto:
> >>
> >> Hi Deniele:
> >>
> >>         I have an idea, by now in order to use QMAP,
> >>         must execute shell command 'echo mux_id > /sys/class/net/<iface>/add_mux' in user space,
> >>         maybe we can expand usage of sys attribute 'add_mux', like next:
> >>         'echo mux_id mux_size mux_version > /sys/class/net/<iface>/add_mux'.
> >>         Users can set correct 'mux_size and mux_version' according to the response of 'QMI_WDA_SET_DATA_FORMAT '.
> >>         If 'mux_size and mux_version' miss, qmi_wwan can use default values.
> >>
> >
> > not sure this is something acceptable, let's wait for Bjørn to comment.
>
> This breaks the "one value per file" rule.  Ref
> https://www.kernel.org/doc/Documentation/filesystems/sysfs.txt
>
> And I really think this userspace ABI is complicated enough already
> without adding yet another variable governed by strict rules.
>
> I'd prefer this to just work, if possible.  I liked the simplicity of
> your patch.  If it is necessary to have a different value when QMAP is
> disabled, then I'd much rather see two fixed values, depending on
> QMI_WWAN_FLAG_MUX.
>

Maybe to have a more cautious approach we can use 2048 for normal mode
(suggested by Qualcomm for the babble issue) and 16384 when using
QMAP, as done by the Windows driver, adding a comment that higher
values (that should be only related to 5G chipsets) are currently not
supported.

I do not currently have the equipment to test with 5G, but using 16384
could not even be a problem since I doubt that the bottleneck for the
real throughput is the bus.

Moreover, we can face it once we are capable of performing throughput
tests and have indications on the behavior according to the different
sizes of the rx urb.

Sounds reasonable?

>
> >>         If fixed set as 32KB, but MDM9x07 chips only support 4KB, or uses do not enable QMAP,
> >>         Maybe cannot reach max throughput for no enough rx urbs.
> >>
> >
> > I did not test for performance, but you could be right since for
> > example, if I'm not wrong, rx_qlen for an high-speed device would be 2
> > with the changed rx_urb_size.
>
> That's correct.  But I believe 2 URBs might be enough.  Still, would be
> nice if some of you with a fast enough modem could test this.  At least
> if throughput issues are going to be an argument for a more complicated
> ABI.
>

With 16384 we'll have 5 URBs with an high-speed modem and QMAP
enabled. That would usually be a configuration for low-cat modems,
while high-cat use super-speed or super-speed-plus, for which I
already tested that value reaching 1.1Gbit/s.

I can try to perform some additional tests with rx_urb_size=16384,
QMAP and low-cat, but it will require more time.

> > So, we'll probably need to modify also usbnet_update_max_qlen, but not
> > sure about the direction (maybe fallback to a default value to
> > guarantee a minimum number if rx_qlen is < than a threshold?). And
> > this is also a change affecting all the drivers using usbnet, so it
> > requires additional care.
>
> I'm not sure we want to do that. We haven't done it for other
> aggregating usbnet protocols.  There is a reason we have the hard limit.
>

Ok, understood.

Thanks,
Daniele

>
>
>
> Bjørn
>
> >> > -----邮件原件-----
> >> > 发件人: Daniele Palmas [mailto:dnlplm@...il.com]
> >> > 发送时间: Wednesday, September 09, 2020 5:13 PM
> >> > 收件人: Bjørn Mork <bjorn@...k.no>
> >> > 抄送: Kristian Evensen <kristian.evensen@...il.com>; Paul Gildea
> >> > <paul.gildea@...il.com>; Carl Yin(殷张成) <carl.yin@...ctel.com>; David S .
> >> > Miller <davem@...emloft.net>; Jakub Kicinski <kuba@...nel.org>;
> >> > netdev@...r.kernel.org; linux-usb@...r.kernel.org; Daniele Palmas
> >> > <dnlplm@...il.com>
> >> > 主题: [PATCH net-next 1/1] net: usb: qmi_wwan: add default rx_urb_size
> >> >
> >> > Add default rx_urb_size to support QMAP download data aggregation without
> >> > needing additional setup steps in userspace.
> >> >
> >> > The value chosen is the current highest one seen in available modems.
> >> >
> >> > The patch has the side-effect of fixing a babble issue in raw-ip mode reported by
> >> > multiple users.
> >> >
> >> > Signed-off-by: Daniele Palmas <dnlplm@...il.com>
> >> > ---
> >> > Resending with mailing lists added: sorry for the noise.
> >> >
> >> > Hi Bjørn and all,
> >> >
> >> > this patch tries to address the issue reported in the following threads
> >> >
> >> > https://www.spinics.net/lists/netdev/msg635944.html
> >> > https://www.spinics.net/lists/linux-usb/msg198846.html
> >> > https://www.spinics.net/lists/linux-usb/msg198025.html
> >> >
> >> > so I'm adding the people involved, maybe you can give it a try to double check if
> >> > this is good for you.
> >> >
> >> > On my side, I performed tests with different QC chipsets without experiencing
> >> > problems.
> >> >
> >> > Thanks,
> >> > Daniele
> >> > ---
> >> >  drivers/net/usb/qmi_wwan.c | 4 ++++
> >> >  1 file changed, 4 insertions(+)
> >> >
> >> > diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c index
> >> > 07c42c0719f5..92d568f982b6 100644
> >> > --- a/drivers/net/usb/qmi_wwan.c
> >> > +++ b/drivers/net/usb/qmi_wwan.c
> >> > @@ -815,6 +815,10 @@ static int qmi_wwan_bind(struct usbnet *dev, struct
> >> > usb_interface *intf)
> >> >       }
> >> >       dev->net->netdev_ops = &qmi_wwan_netdev_ops;
> >> >       dev->net->sysfs_groups[0] = &qmi_wwan_sysfs_attr_group;
> >> > +
> >> > +     /* Set rx_urb_size to allow QMAP rx data aggregation */
> >> > +     dev->rx_urb_size = 32768;
> >> > +
> >> >  err:
> >> >       return status;
> >> >  }
> >> > --
> >> > 2.17.1
> >>

Powered by blists - more mailing lists