| Message ID | 20200909091302.20992-1-dnlplm@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [net-next,1/1] net: usb: qmi_wwan: add default rx_urb_size | expand |
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. 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. > -----邮件原件----- > 发件人: 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 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 >
On Wed, Sep 09, 2020 at 11:13:02AM +0200, Daniele Palmas 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> Any specific kernel commit that this "fixes:"? > --- > 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; Where did this "magic number" come from? And making an urb size that big can keep some pipelines full, it also comes at the expense of other potential issues, have you tested this to see that it really does help in throughput? And if it does, does this size really need to be that big? What is it set to today, the endpoint size? If so, that's a huge jump... thanks, greg k-h
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. >> 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. > 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. 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 Greg, Il giorno mer 9 set 2020 alle ore 14:28 Greg KH <gregkh@linuxfoundation.org> ha scritto: > > On Wed, Sep 09, 2020 at 11:13:02AM +0200, Daniele Palmas 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> > > Any specific kernel commit that this "fixes:"? > Related to the aggregation, if I have to find a commit that would be c6adf77953bcec0ad63d7782479452464e50f7a3 "net: usb: qmi_wwan: add qmap mux protocol support", but I feel that this is an improvement rather than a fix, since it avoids having userspace to configure the rx_urb_size through changing the MTU of the master interface. Related to the babble issue, my understanding is that it's not a qmi_wwan issue, but a workaround for some chipsets' behavior. > > --- > > 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; > > Where did this "magic number" come from? > This is coming from the modem and it's the highest value I'm aware of (Qualcomm SDX55 chipset). The problem is that different chipsets have different maximum values, so while other modems have lower values (e.g. 4096), if rx_urb_size can't be configured in user-space, the maximum known value should be used to support all currently available chipsets. > And making an urb size that big can keep some pipelines full, it also > comes at the expense of other potential issues, have you tested this to > see that it really does help in throughput? > Yes, I had doubts about this: I could not test throughput for SDX55, since at the moment I do not have the needed equipment. In the past I tested a different value (16384) with 4G modems and I'm able to reach the max dl throughput (1.1Gbit/s). Actually, enabling dl aggregation is the only way to reach that throughput. > And if it does, does this size really need to be that big? What is it > set to today, the endpoint size? If so, that's a huge jump... > Since 16384 seems also the size used by the Windows driver, a possibility is to use that value and add a comment in the driver stating that higher values are not supported. Today the default size is 1500, but it can be "configured" in user space as a side-effect of changing the MTU. To me that would still be acceptable since proved to be working fine until now, but the problem is that to solve a babble issue seen with recent chipsets the default rx_urb_size should be changed to an higher value (QC suggested 2048) and this breaks the MTU workaround (see function usbnet_change_mtu in usbnet.c). Thanks, Daniele > thanks, > > greg k-h
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 Kristian, Il giorno mer 4 nov 2020 alle ore 18:01 Kristian Evensen <kristian.evensen@gmail.com> ha scritto: > > 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. 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. > 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. Is the dongle driver based on usbnet? Besides the aggregated datagram size, did you also try different datagram max numbers? > 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. > Sorry, I'm not expert enough to give you any good hint. 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. Regards, Daniele > 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
Hi Kristian, For openwrt device, the ' Performance bottleneck ' usually is NAT, not usbnet. As I remember: MT7621 have dual core, and support Hardware acceleration of 'NAT'. It seems r8152 is a pure Ethernet card, does it can use the ' Hardware acceleration ' And do you use 'mpstat -P ALL 2' to monitor each core's loading? Generally USB interrupt occurs at cpu0, and the 'NAT' is also on cpu0. You can try to use "echo 2 > /sys/class/net/wwan0/ /queues/rx-0/rps_cpus " to move NAT to cpu1. X55 max support 31KB, there are benefit from 16KB -> 31KB. Maybe your X55's FW version is old, only generates 16KB data. And URB size is 32KB, but X55 only output 16KB, so maybe there are not enough number of URBs? On November 13, 2020 3:37, Kristian Evensen wrote: > 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
Hi Carl, Thanks a lot for your reply. On Fri, Nov 13, 2020 at 9:37 AM Carl Yin(殷张成) <carl.yin@quectel.com> wrote: > For openwrt device, the ' Performance bottleneck ' usually is NAT, not usbnet. > As I remember: MT7621 have dual core, and support Hardware acceleration of 'NAT'. Yes, you are right in that NAT can have a large effect on performance, especially when you start being CPU-limited. However,when using perf to profile the kernel during my tests, no function related to netfilter/conntrack appeared very high on the list. I would also expect the modem to at least reach the performance of the dongle, with offloading being switched off. However, there could be some detail I missed. > It seems r8152 is a pure Ethernet card, does it can use the ' Hardware acceleration ' I will do some experiments with hardware NAT, thanks for reminding me of this feature. However, my experience with it is not very good, so I would ideally like to find a solution that does not rely on this feature. > And do you use 'mpstat -P ALL 2' to monitor each core's loading? > Generally USB interrupt occurs at cpu0, and the 'NAT' is also on cpu0. > You can try to use "echo 2 > /sys/class/net/wwan0/ /queues/rx-0/rps_cpus " to move NAT to cpu1. I use htop to monitor the load on each core. rx_cpus is set to "e" to balance traffic better across all cores, locking rx to one core gave a much worse result (something like 170 Mbit/s). > X55 max support 31KB, there are benefit from 16KB -> 31KB. > Maybe your X55's FW version is old, only generates 16KB data. > And URB size is 32KB, but X55 only output 16KB, so maybe there are not enough number of URBs? I had the same theory and asked my FAE if a more recent firmware is available for my device (some of my tests were done with Quectel RM500Q). I do not know what is the trigger for the device to generate 32KB URBs is? The fastest network I have access to gives a speed of 700-800 Mbit/s, but I do not know if that is enough? Kristian
Hi, On Fri, Nov 13, 2020 at 9:50 AM Kristian Evensen <kristian.evensen@gmail.com> wrote: > Yes, you are right in that NAT can have a large effect on performance, > especially when you start being CPU-limited. However,when using perf > to profile the kernel during my tests, no function related to > netfilter/conntrack appeared very high on the list. I would also > expect the modem to at least reach the performance of the dongle, with > offloading being switched off. However, there could be some detail I > missed. I continued working on this issue today and I believe I have found at least one reason for my performance problems. My initial attempts at profiling resulted in quite noisy perf files and this caused me to look in the wrong places. Today I figured out how to get a cleaner file, and I noticed that a lot of resources were spent on pskb_expand_head() + support functions. My MT7621 devices are used as routers, so before the packets are sent out on the LAN additional headers have to be added. The current code in qmimux_rx_fixup() allocates an SKB for each aggregated packet and copies the data from the URB. The newly allocated SKB has too little headroom, so when we get to ip_forward() then the check in skb_cow() fails and the SKB is reallocated. After increasing the amount of data allocated to also include the required headroom + reserving headroom amount of bytes, I see a huge performance increase. I go from around 230 Mbit/s and to 280Mbit/s, with significantly less CPU usage. 280 Mbit/s is the same speed as I get from my phone connected to the same network, so it seems to be the max of the network right now. I do not know what would be an acceptable way (if any) to get this fix upstreamed. I currently add an additional "safe" amount of data, but I am pretty sure ETH_HLEN + 2 is not an acceptable solution :) 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(+)