On 22. 12. 18. 17:55, Greg KH wrote:
On Sun, Dec 18, 2022 at 01:18:51AM +0900, Leesoo Ahn wrote:
The current source pushes skb into dev->done queue by calling
skb_queue_tail() and then, call skb_dequeue() to pop for rx_cleanup state
to free urb and skb next in usbnet_bh().
It wastes CPU resource with extra instructions. Instead, use return values
jumping to rx_cleanup case directly to free them. Therefore calling
skb_queue_tail() and skb_dequeue() is not necessary.
The follows are just showing difference between calling skb_queue_tail()
and using return values jumping to rx_cleanup state directly in usbnet_bh()
in Arm64 instructions with perf tool.
----------- calling skb_queue_tail() -----------
│ if (!(dev->driver_info->flags & FLAG_RX_ASSEMBLE))
7.58 │248: ldr x0, [x20, #16]
2.46 │24c: ldr w0, [x0, #8]
1.64 │250: ↑ tbnz w0, #14, 16c
│ dev->net->stats.rx_errors++;
0.57 │254: ldr x1, [x20, #184]
1.64 │258: ldr x0, [x1, #336]
2.65 │25c: add x0, x0, #0x1
│260: str x0, [x1, #336]
│ skb_queue_tail(&dev->done, skb);
0.38 │264: mov x1, x19
│268: mov x0, x21
2.27 │26c: → bl skb_queue_tail
0.57 │270: ↑ b 44 // branch to call skb_dequeue()
----------- jumping to rx_cleanup state -----------
│ if (!(dev->driver_info->flags & FLAG_RX_ASSEMBLE))
1.69 │25c: ldr x0, [x21, #16]
4.78 │260: ldr w0, [x0, #8]
3.28 │264: ↑ tbnz w0, #14, e4 // jump to 'rx_cleanup' state
│ dev->net->stats.rx_errors++;
0.09 │268: ldr x1, [x21, #184]
2.72 │26c: ldr x0, [x1, #336]
3.37 │270: add x0, x0, #0x1
0.09 │274: str x0, [x1, #336]
0.66 │278: ↑ b e4 // branch to 'rx_cleanup' state
Interesting, but does this even really matter given the slow speed of
the USB hardware?
It doesn't if USB hardware has slow speed but in software view, it's
still worth avoiding calling skb_queue_tail() and skb_dequeue() which
work with spinlock, if possible.
Signed-off-by: Leesoo Ahn <lsahn@xxxxxxxxxx>
---
drivers/net/usb/usbnet.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 64a9a80b2309..924392a37297 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -555,7 +555,7 @@ static int rx_submit (struct usbnet *dev, struct urb *urb, gfp_t flags)
/*-------------------------------------------------------------------------*/
-static inline void rx_process (struct usbnet *dev, struct sk_buff *skb)
+static inline int rx_process(struct usbnet *dev, struct sk_buff *skb)
{
if (dev->driver_info->rx_fixup &&
!dev->driver_info->rx_fixup (dev, skb)) {
@@ -576,11 +576,11 @@ static inline void rx_process (struct usbnet *dev, struct sk_buff *skb)
netif_dbg(dev, rx_err, dev->net, "rx length %d\n", skb->len);
} else {
usbnet_skb_return(dev, skb);
- return;
+ return 0;
}
done:
- skb_queue_tail(&dev->done, skb);
+ return -1;
Don't make up error numbers, this makes it look like this failed, not
succeeded. And if this failed, give it a real error value.
thanks,
greg k-h
Best regards,
Leesoo
