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
Signed-off-by: Leesoo Ahn <[email protected]>
---
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;
}
/*-------------------------------------------------------------------------*/
@@ -1528,13 +1528,14 @@ static void usbnet_bh (struct timer_list *t)
entry = (struct skb_data *) skb->cb;
switch (entry->state) {
case rx_done:
- entry->state = rx_cleanup;
- rx_process (dev, skb);
+ if (rx_process(dev, skb))
+ goto cleanup;
continue;
case tx_done:
kfree(entry->urb->sg);
fallthrough;
case rx_cleanup:
+cleanup:
usb_free_urb (entry->urb);
dev_kfree_skb (skb);
continue;
--
2.34.1
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?
> Signed-off-by: Leesoo Ahn <[email protected]>
> ---
> 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
On Sun, Dec 18, 2022 at 09:55:44AM +0100, 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?
On the other side, it is pretty nice optimization and a proof someone
read the code really carefully.
> > Signed-off-by: Leesoo Ahn <[email protected]>
> > ---
> > 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.
Note that jumps to 'done' label can be avoided now, so eventual v2 version
of that patch doesn't increase total goto entropy.
l.
> thanks,
>
> greg k-h
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 <[email protected]>
>> ---
>> 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
On Mon, Dec 19, 2022 at 04:41:16PM +0900, Leesoo Ahn wrote:
>
> 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.
But can you actually measure that in either CPU load or in increased
transfer speeds?
thanks,
greg k-h
On 22. 12. 19. 16:50, Greg KH wrote:
> On Mon, Dec 19, 2022 at 04:41:16PM +0900, Leesoo Ahn wrote:
>> 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.
> But can you actually measure that in either CPU load or in increased
> transfer speeds?
>
> thanks,
>
> greg k-h
I think the follows are maybe what you would be interested in. I have
tested both case with perf on the same machine and environments, also
modified driver code a bit to go to rx_cleanup case, not to net stack in
a specific packet.
----- calling skb_queue_tail() -----
- 11.58% 0.26% swapper [k] usbnet_bh
- 11.32% usbnet_bh
- 6.43% skb_dequeue
6.34% _raw_spin_unlock_irqrestore
- 2.21% skb_queue_tail
2.19% _raw_spin_unlock_irqrestore
- 1.68% consume_skb
- 0.97% kfree_skbmem
0.80% kmem_cache_free
0.53% skb_release_data
----- jump to rx_cleanup directly -----
- 7.62% 0.18% swapper [k] usbnet_bh
- 7.44% usbnet_bh
- 4.63% skb_dequeue
4.57% _raw_spin_unlock_irqrestore
- 1.76% consume_skb
- 1.03% kfree_skbmem
0.86% kmem_cache_free
0.56% skb_release_data
0.54% smsc95xx_rx_fixup
The first case takes CPU resource a bit much by the result.
Thank you for reviewing, by the way.
Best regards,
Leesoo
On Mon, Dec 19, 2022 at 05:09:21PM +0900, Leesoo Ahn wrote:
>
> On 22. 12. 19. 16:50, Greg KH wrote:
> > On Mon, Dec 19, 2022 at 04:41:16PM +0900, Leesoo Ahn wrote:
> > > 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.
> > But can you actually measure that in either CPU load or in increased
> > transfer speeds?
> >
> > thanks,
> >
> > greg k-h
>
> I think the follows are maybe what you would be interested in. I have tested
> both case with perf on the same machine and environments, also modified
> driver code a bit to go to rx_cleanup case, not to net stack in a specific
> packet.
>
> ----- calling skb_queue_tail() -----
> - 11.58% 0.26% swapper [k] usbnet_bh
> - 11.32% usbnet_bh
> - 6.43% skb_dequeue
> 6.34% _raw_spin_unlock_irqrestore
> - 2.21% skb_queue_tail
> 2.19% _raw_spin_unlock_irqrestore
> - 1.68% consume_skb
> - 0.97% kfree_skbmem
> 0.80% kmem_cache_free
> 0.53% skb_release_data
>
> ----- jump to rx_cleanup directly -----
> - 7.62% 0.18% swapper [k] usbnet_bh
> - 7.44% usbnet_bh
> - 4.63% skb_dequeue
> 4.57% _raw_spin_unlock_irqrestore
> - 1.76% consume_skb
> - 1.03% kfree_skbmem
> 0.86% kmem_cache_free
> 0.56% skb_release_data
> 0.54% smsc95xx_rx_fixup
>
> The first case takes CPU resource a bit much by the result.
Ok, great! Fix up the patch based on the review comments and add this
information to the changelog as well.
thanks,
greg k-h