| Message ID | 20200909091302.20992-1-dnlplm@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [net-next,1/1] net: usb: qmi_wwan: add default rx_urb_size | expand |
Hi Carl, Il giorno mer 9 set 2020 alle ore 13:09 Carl Yin(殷张成) <carl.yin@quectel.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. > 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. 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. Let's wait for the maintainers' advice also on this. Regards, Daniele > > > -----邮件原件----- > > 发件人: Daniele Palmas [mailto:dnlplm@gmail.com] > > 发送时间: Wednesday, September 09, 2020 5:13 PM > > 收件人: Bjørn Mork <bjorn@mork.no> > > 抄送: Kristian Evensen <kristian.evensen@gmail.com>; Paul Gildea > > <paul.gildea@gmail.com>; Carl Yin(殷张成) <carl.yin@quectel.com>; David S . > > Miller <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; > > netdev@vger.kernel.org; linux-usb@vger.kernel.org; Daniele Palmas > > <dnlplm@gmail.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@gmail.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 >
Hi Bjørn, Il giorno mer 9 set 2020 alle ore 14:49 Bjørn Mork <bjorn@mork.no> ha scritto: > > Daniele Palmas <dnlplm@gmail.com> writes: > > Il giorno mer 9 set 2020 alle ore 13:09 Carl Yin(殷张成) > > <carl.yin@quectel.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@gmail.com] > >> > 发送时间: Wednesday, September 09, 2020 5:13 PM > >> > 收件人: Bjørn Mork <bjorn@mork.no> > >> > 抄送: Kristian Evensen <kristian.evensen@gmail.com>; Paul Gildea > >> > <paul.gildea@gmail.com>; Carl Yin(殷张成) <carl.yin@quectel.com>; David S . > >> > Miller <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; > >> > netdev@vger.kernel.org; linux-usb@vger.kernel.org; Daniele Palmas > >> > <dnlplm@gmail.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@gmail.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 > >>
Hi, On Wed, Sep 9, 2020 at 11:14 AM Daniele Palmas <dnlplm@gmail.com> wrote: > > 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@gmail.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 First of all, I am very sorry for not providing any feedback earlier. I applied your patch and have been running it on my devices more or less since it was submitted. My devices are equipped with different generations of modems (cat. 4, cat. 6, cat. 12, 5G NSA), and I haven't noticed any problems and the babble-issue is gone. Over the last couple of days I also finally had a chance to experiment with QMAP, using an SDX55-based modem (i..e,32KB datagram support). Increasing the datagram size to 32KB gives a nice performance boost over for example 16KB. When measuring using iperf3 (on the same device), the throughput goes from around 210 Mbit/s and to 230 Mbit/s. The CPU was more or less saturated during all of my experiments, so the main performance gain was from the increased aggregated datagram size. As a side question, and perhaps this should be a separate thread, does anyone have any suggestion on how to improve QMI performance further? The device that I used for my iperf3-tests is mt7621-based, and using for example an Ethernet dongle I am able to reach somere between 400 and 500 Mbit/s over USB. The Ethernet dongle is able to make use of for example scatter-gather, but I would still expect at least a bit more using QMI. I tried to replace the alloc()/put() in the qmimux_rx_fixup() function with clone() and then doing push()/pull(), but this resulted in a decrease in performance. I have probably overlooked something, but I think at least my use of the functions was correct. The packets looked correct when adding some debug output, error counters did not increase, etc., etc. The mobile network is not the bottleneck, on my phone I reliably get around 400 Mbit/s. BR, Kristian
Hi Daniele, On Thu, Nov 12, 2020 at 7:29 PM Daniele Palmas <dnlplm@gmail.com> wrote: > thanks for testing. Still thinking it could be better to differentiate > between raw-ip and qmap, but not yet able to find the time to perform > some tests on my own. I agree that separating between qmap and non-qmap would be nice. However, with my modules I have not noticed any issues when using 32KB as the URB size. Still, the results show that there is no gain in increasing the aggregation size from 16 to 32KB. Capturing traffic from the modem reveals that the hardware still only generates 16KB URBs (even in high-speed networks). I also see that for example the r8152 driver uses a static URB size of 16384. > Is the dongle driver based on usbnet? Besides the aggregated datagram > size, did you also try different datagram max numbers? The dongle driver is not based on usbnet, it is r8152. I tried to increase the maximum datagrams from 32 to 64 (as well as some other values), but it had no effect on the perfrormance. > The only advice I can give you is to check if other drivers are > performing better, e.g. did you try the MBIM composition? not sure it > will make much difference, since it's based on usbnet, but could be > worth trying. I tried to use MBIM, but the performance was the same as with QMI. I will take a look at r8152 and experiment with implementing some of the differences in usbnet/qmi_wwan. I see for example that r8152 uses NAPI, which while not a perfect fit for USB could be worth a try. Based on some discussions I found on the mailing list from 2011, implementing NAPI in usbnet could be worthwhile. Thanks for the reply! BR, Kristian
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; }
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@gmail.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(+)