* [PATCH] usb: gadget: u_ether: enable qmult on SuperSpeed Plus as well
@ 2020-08-06 16:16 Lorenzo Colitti
2020-08-06 17:29 ` Maciej Żenczykowski
0 siblings, 1 reply; 5+ messages in thread
From: Lorenzo Colitti @ 2020-08-06 16:16 UTC (permalink / raw)
To: linux-usb; +Cc: balbi, gregkh, zenczykowski, Lorenzo Colitti
The u_ether driver has a qmult setting that multiplies the
transmit queue length (which by default is 2).
The intent is that it should be enabled at high/super speed, but
because the code does not explicitly check for USB_SUPER_PLUS,
it is disabled at that speed.
Fix this by ensuring that the queue multiplier is enabled for any
wired link at high speed or above. Using >= for USB_SPEED_*
constants seems correct because it is what the gadget_is_xxxspeed
functions do.
The queue multiplier substantially helps performance at higher
speeds. On a direct SuperSpeed Plus link to a Linux laptop,
iperf3 single TCP stream:
Before (qmult=1): 1.3 Gbps
After (qmult=5): 3.2 Gbps
Signed-off-by: Lorenzo Colitti <lorenzo@google.com>
---
drivers/usb/gadget/function/u_ether.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
index c3cc6bd14e..31ea76adcc 100644
--- a/drivers/usb/gadget/function/u_ether.c
+++ b/drivers/usb/gadget/function/u_ether.c
@@ -93,7 +93,7 @@ struct eth_dev {
static inline int qlen(struct usb_gadget *gadget, unsigned qmult)
{
if (gadget_is_dualspeed(gadget) && (gadget->speed == USB_SPEED_HIGH ||
- gadget->speed == USB_SPEED_SUPER))
+ gadget->speed >= USB_SPEED_SUPER))
return qmult * DEFAULT_QLEN;
else
return DEFAULT_QLEN;
--
2.28.0.163.g6104cc2f0b6-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] usb: gadget: u_ether: enable qmult on SuperSpeed Plus as well
2020-08-06 16:16 [PATCH] usb: gadget: u_ether: enable qmult on SuperSpeed Plus as well Lorenzo Colitti
@ 2020-08-06 17:29 ` Maciej Żenczykowski
2020-08-06 17:41 ` Maciej Żenczykowski
2020-08-18 16:25 ` Lorenzo Colitti
0 siblings, 2 replies; 5+ messages in thread
From: Maciej Żenczykowski @ 2020-08-06 17:29 UTC (permalink / raw)
To: Lorenzo Colitti; +Cc: linux-usb, balbi, gregkh
On Thu, Aug 6, 2020 at 9:17 AM Lorenzo Colitti <lorenzo@google.com> wrote:
>
> The u_ether driver has a qmult setting that multiplies the
> transmit queue length (which by default is 2).
>
> The intent is that it should be enabled at high/super speed, but
> because the code does not explicitly check for USB_SUPER_PLUS,
> it is disabled at that speed.
>
> Fix this by ensuring that the queue multiplier is enabled for any
> wired link at high speed or above. Using >= for USB_SPEED_*
> constants seems correct because it is what the gadget_is_xxxspeed
> functions do.
>
> The queue multiplier substantially helps performance at higher
> speeds. On a direct SuperSpeed Plus link to a Linux laptop,
> iperf3 single TCP stream:
>
> Before (qmult=1): 1.3 Gbps
> After (qmult=5): 3.2 Gbps
>
> Signed-off-by: Lorenzo Colitti <lorenzo@google.com>
> ---
> drivers/usb/gadget/function/u_ether.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
> index c3cc6bd14e..31ea76adcc 100644
> --- a/drivers/usb/gadget/function/u_ether.c
> +++ b/drivers/usb/gadget/function/u_ether.c
> @@ -93,7 +93,7 @@ struct eth_dev {
> static inline int qlen(struct usb_gadget *gadget, unsigned qmult)
> {
> if (gadget_is_dualspeed(gadget) && (gadget->speed == USB_SPEED_HIGH ||
> - gadget->speed == USB_SPEED_SUPER))
> + gadget->speed >= USB_SPEED_SUPER))
> return qmult * DEFAULT_QLEN;
> else
> return DEFAULT_QLEN;
> --
> 2.28.0.163.g6104cc2f0b6-goog
>
Reviewed-by: Maciej Żenczykowski <maze@google.com>
Though I think this probably deserves a fixes tag of some sort.
Probably:
Fixes: 04617db7aa68 ("usb: gadget: add SS descriptors to Ethernet gadget")
since that's the commit that did:
-MODULE_PARM_DESC(qmult, "queue length multiplier at high speed");
+MODULE_PARM_DESC(qmult, "queue length multiplier at high/super speed");
...
-/* for dual-speed hardware, use deeper queues at highspeed */
+/* for dual-speed hardware, use deeper queues at high/super speed */
static inline int qlen(struct usb_gadget *gadget)
{
- if (gadget_is_dualspeed(gadget) && gadget->speed == USB_SPEED_HIGH)
+ if (gadget_is_dualspeed(gadget) && (gadget->speed == USB_SPEED_HIGH ||
+ gadget->speed == USB_SPEED_SUPER))
return qmult * DEFAULT_QLEN;
else
return DEFAULT_QLEN;
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] usb: gadget: u_ether: enable qmult on SuperSpeed Plus as well
2020-08-06 17:29 ` Maciej Żenczykowski
@ 2020-08-06 17:41 ` Maciej Żenczykowski
2020-08-18 16:26 ` Lorenzo Colitti
2020-08-18 16:25 ` Lorenzo Colitti
1 sibling, 1 reply; 5+ messages in thread
From: Maciej Żenczykowski @ 2020-08-06 17:41 UTC (permalink / raw)
To: Lorenzo Colitti; +Cc: linux-usb, balbi, gregkh
btw. it looks like irq throttling in the same file:
@@ -598,9 +599,10 @@ static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
- /* throttle highspeed IRQ rate back slightly */
+ /* throttle high/super speed IRQ rate back slightly */
if (gadget_is_dualspeed(dev->gadget))
- req->no_interrupt = (dev->gadget->speed == USB_SPEED_HIGH)
+ req->no_interrupt = (dev->gadget->speed == USB_SPEED_HIGH ||
+ dev->gadget->speed == USB_SPEED_SUPER)
should also be fixed to be >= SUPER and not ==.
On Thu, Aug 6, 2020 at 10:29 AM Maciej Żenczykowski
<zenczykowski@gmail.com> wrote:
>
> On Thu, Aug 6, 2020 at 9:17 AM Lorenzo Colitti <lorenzo@google.com> wrote:
> >
> > The u_ether driver has a qmult setting that multiplies the
> > transmit queue length (which by default is 2).
> >
> > The intent is that it should be enabled at high/super speed, but
> > because the code does not explicitly check for USB_SUPER_PLUS,
> > it is disabled at that speed.
> >
> > Fix this by ensuring that the queue multiplier is enabled for any
> > wired link at high speed or above. Using >= for USB_SPEED_*
> > constants seems correct because it is what the gadget_is_xxxspeed
> > functions do.
> >
> > The queue multiplier substantially helps performance at higher
> > speeds. On a direct SuperSpeed Plus link to a Linux laptop,
> > iperf3 single TCP stream:
> >
> > Before (qmult=1): 1.3 Gbps
> > After (qmult=5): 3.2 Gbps
> >
> > Signed-off-by: Lorenzo Colitti <lorenzo@google.com>
> > ---
> > drivers/usb/gadget/function/u_ether.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
> > index c3cc6bd14e..31ea76adcc 100644
> > --- a/drivers/usb/gadget/function/u_ether.c
> > +++ b/drivers/usb/gadget/function/u_ether.c
> > @@ -93,7 +93,7 @@ struct eth_dev {
> > static inline int qlen(struct usb_gadget *gadget, unsigned qmult)
> > {
> > if (gadget_is_dualspeed(gadget) && (gadget->speed == USB_SPEED_HIGH ||
> > - gadget->speed == USB_SPEED_SUPER))
> > + gadget->speed >= USB_SPEED_SUPER))
> > return qmult * DEFAULT_QLEN;
> > else
> > return DEFAULT_QLEN;
> > --
> > 2.28.0.163.g6104cc2f0b6-goog
> >
>
> Reviewed-by: Maciej Żenczykowski <maze@google.com>
>
> Though I think this probably deserves a fixes tag of some sort.
> Probably:
>
> Fixes: 04617db7aa68 ("usb: gadget: add SS descriptors to Ethernet gadget")
>
> since that's the commit that did:
>
> -MODULE_PARM_DESC(qmult, "queue length multiplier at high speed");
> +MODULE_PARM_DESC(qmult, "queue length multiplier at high/super speed");
>
> ...
>
> -/* for dual-speed hardware, use deeper queues at highspeed */
> +/* for dual-speed hardware, use deeper queues at high/super speed */
> static inline int qlen(struct usb_gadget *gadget)
> {
> - if (gadget_is_dualspeed(gadget) && gadget->speed == USB_SPEED_HIGH)
> + if (gadget_is_dualspeed(gadget) && (gadget->speed == USB_SPEED_HIGH ||
> + gadget->speed == USB_SPEED_SUPER))
> return qmult * DEFAULT_QLEN;
> else
> return DEFAULT_QLEN;
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] usb: gadget: u_ether: enable qmult on SuperSpeed Plus as well
2020-08-06 17:29 ` Maciej Żenczykowski
2020-08-06 17:41 ` Maciej Żenczykowski
@ 2020-08-18 16:25 ` Lorenzo Colitti
1 sibling, 0 replies; 5+ messages in thread
From: Lorenzo Colitti @ 2020-08-18 16:25 UTC (permalink / raw)
To: Maciej Żenczykowski; +Cc: linux-usb, Felipe Balbi, Greg KH
> Reviewed-by: Maciej Żenczykowski <maze@google.com>
>
> Though I think this probably deserves a fixes tag of some sort.
> Probably:
>
> Fixes: 04617db7aa68 ("usb: gadget: add SS descriptors to Ethernet gadget")
>
> since that's the commit that did:
>
> -MODULE_PARM_DESC(qmult, "queue length multiplier at high speed");
> +MODULE_PARM_DESC(qmult, "queue length multiplier at high/super speed");
Thanks for the review. Added Fixes: tag in v2.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] usb: gadget: u_ether: enable qmult on SuperSpeed Plus as well
2020-08-06 17:41 ` Maciej Żenczykowski
@ 2020-08-18 16:26 ` Lorenzo Colitti
0 siblings, 0 replies; 5+ messages in thread
From: Lorenzo Colitti @ 2020-08-18 16:26 UTC (permalink / raw)
To: Maciej Żenczykowski; +Cc: linux-usb, Felipe Balbi, Greg KH
On Fri, Aug 7, 2020 at 2:41 AM Maciej Żenczykowski
<zenczykowski@gmail.com> wrote:
> btw. it looks like irq throttling in the same file:
>
> @@ -598,9 +599,10 @@ static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
> - /* throttle highspeed IRQ rate back slightly */
> + /* throttle high/super speed IRQ rate back slightly */
> if (gadget_is_dualspeed(dev->gadget))
> - req->no_interrupt = (dev->gadget->speed == USB_SPEED_HIGH)
> + req->no_interrupt = (dev->gadget->speed == USB_SPEED_HIGH ||
> + dev->gadget->speed == USB_SPEED_SUPER)
>
> should also be fixed to be >= SUPER and not ==.
That code was removed by fd9afd3cbe40 ("usb: gadget: u_ether: remove
interrupt throttling").
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-08-18 16:27 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-06 16:16 [PATCH] usb: gadget: u_ether: enable qmult on SuperSpeed Plus as well Lorenzo Colitti
2020-08-06 17:29 ` Maciej Żenczykowski
2020-08-06 17:41 ` Maciej Żenczykowski
2020-08-18 16:26 ` Lorenzo Colitti
2020-08-18 16:25 ` Lorenzo Colitti
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).