[PATCH net-next 1/1] net: usb: qmi_wwan: add default rx_urb_size

[PATCH net-next 1/1] net: usb: qmi_wwan: add default rx_urb_size

[Daniele Palmas]

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

答复: [PATCH net-next 1/1] net: usb: qmi_wwan: add default rx_urb_size

[Carl Yin(殷张成)]

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

Re: [PATCH net-next 1/1] net: usb: qmi_wwan: add default rx_urb_size

[Daniele Palmas]

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
>

Re: [PATCH net-next 1/1] net: usb: qmi_wwan: add default rx_urb_size

[Greg KH]

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

Re: [PATCH net-next 1/1] net: usb: qmi_wwan: add default rx_urb_size

[Bjørn Mork]

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

Re: [PATCH net-next 1/1] net: usb: qmi_wwan: add default rx_urb_size

[Daniele Palmas]

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

Re: [PATCH net-next 1/1] net: usb: qmi_wwan: add default rx_urb_size

[Daniele Palmas]

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

Re: [PATCH net-next 1/1] net: usb: qmi_wwan: add default rx_urb_size

[Kristian Evensen]

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

Re: [PATCH net-next 1/1] net: usb: qmi_wwan: add default rx_urb_size

[Daniele Palmas]

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

Re: [PATCH net-next 1/1] net: usb: qmi_wwan: add default rx_urb_size

[Kristian Evensen]

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

答复: [PATCH net-next 1/1] net: usb: qmi_wwan: add default rx_urb_size

[Carl Yin(殷张成)]

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

Re: [PATCH net-next 1/1] net: usb: qmi_wwan: add default rx_urb_size

[Kristian Evensen]

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

Re: [PATCH net-next 1/1] net: usb: qmi_wwan: add default rx_urb_size

[Kristian Evensen]

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