smsc95xx truesize badness

Hi all,

It seems smsc95xx is doing bad things with truesize in our receive loop.

Simplified loop with some bits removed for clarity (handles the
possible case of receiving multiple frames in one USB transaction):


while (skb->len > 0) {

    /* last frame in this batch */
    if (skb->len == size) {
        skb_trim(skb, skb->len - 4); /* remove fcs */
        skb->truesize = size + sizeof(struct sk_buff);
        return 1;
    }

    ax_skb = skb_clone(skb, GFP_ATOMIC);
    if (unlikely(!ax_skb)) {
        netdev_warn(dev->net, "Error allocating skb\n");
        return 0;
    }

    ax_skb->len = size;
    ax_skb->data = packet;
    skb_set_tail_pointer(ax_skb, size);
    skb_trim(ax_skb, ax_skb->len - 4); /* remove fcs */
    ax_skb->truesize = size + sizeof(struct sk_buff);

    usbnet_skb_return(dev, ax_skb);
}


This is triggering warnings for many Raspberry Pi users due to Eric's
patch to catch truesize offenders.  It's a fair cop.

Before I submit a patch to remove these two truesize lines, I believe
it's safe to just do so but can anyone confirm if removing them will
have any adverse impact?

Thanks!
-- 
Steve Glendinning

On Thu, 2015-02-19 at 18:58 +0000, Steve Glendinning wrote:

This is triggering warnings for many Raspberry Pi users due to Eric's
patch to catch truesize offenders.  It's a fair cop.

Before I submit a patch to remove these two truesize lines, I believe
it's safe to just do so but can anyone confirm if removing them will
have any adverse impact?

If you simply remove the lines, skb->truesize might be big enough to
slow down TCP noticeably.

I would rather use something like commit
a9e0aca4b37885b5599e52211f098bd7f565e749
("asix: asix_rx_fixup surgery to reduce skb truesizes")

Idea is to 'copybreak' tiny frames.

From: Steve Glendinning

It seems smsc95xx is doing bad things with truesize in our receive loop.

IIRC the same is true of a lot of the usbnet drivers.

Really the way usbnet works needs a complete revist.

Allocating (eg) 16k skb for receive is just plain stupid in the first place.
Provided the URB are multiples of the USB buffer size what the card sends
as a single bulk message can be received into multiple URB.

I'm not entirely convinced that trying to receive into skb is a good idea.
Copybreak of short frames and passing up references to rx buffer fragments
for long frames is probably a better idea.
Even for 100M 'short' might be 1500!

For USB3 things are even worse.
A USB3 ethernet driver ought to be almost as fast as a normal driver.
After all the transmit buffer fragments just need writing into the
usb descriptor ring (with some non-obvious constraints the xhci driver
tends to ignore as well).
However the code paths through usbnet are horribly long.

	David

On Thu, 2015-02-19 at 14:14 -0800, Eric Dumazet wrote:

On Thu, 2015-02-19 at 18:58 +0000, Steve Glendinning wrote:

quoted

This is triggering warnings for many Raspberry Pi users due to Eric's
patch to catch truesize offenders.  It's a fair cop.

Before I submit a patch to remove these two truesize lines, I believe
it's safe to just do so but can anyone confirm if removing them will
have any adverse impact?

If you simply remove the lines, skb->truesize might be big enough to
slow down TCP noticeably.

I would rather use something like commit
a9e0aca4b37885b5599e52211f098bd7f565e749
("asix: asix_rx_fixup surgery to reduce skb truesizes")

Idea is to 'copybreak' tiny frames.

On smsc95xx.c, rx_urb_size is set to :

MAX_SINGLE_PACKET_SIZE (2048) on non turbo mode
DEFAULT_HS_BURST_CAP_SIZE (more than 18 KB) on USB_SPEED_HIGH mode
DEFAULT_FS_BURST_CAP_SIZE  (more than 7 KB)

So only non turbo mode would be okay with non copybreak strategy.