smsc75xx & smsc95xx, setting skb->truesize correctly?

smsc75xx & smsc95xx, setting skb->truesize correctly?

[Jussi Kivilinna]

Hello,

Is setting skb->truesize in smsc75xx and smsc95xx correct?
In smsc75xx/smsc95xx_rx_fixup(), input skb containing multiple packets  
is cloned and truesize for each clone is set to packet-size +  
sizeof(struct sk_buff), but input skb has minimum allocation size of  
9000 bytes (MAX_SINGLE_PACKET_SIZE) and maximum of 18944 bytes  
(DEFAULT_HS_BURST_CAP_SIZE) (+ NET_IP_ALIGN). Doesn't this cause  
truesize to be underestimated?

-Jussi

Re: smsc75xx & smsc95xx, setting skb->truesize correctly?

[Eric Dumazet]

On Mon, 2012-08-20 at 17:57 +0300, Jussi Kivilinna wrote:
> Hello,
> 
> Is setting skb->truesize in smsc75xx and smsc95xx correct?
> In smsc75xx/smsc95xx_rx_fixup(), input skb containing multiple packets  
> is cloned and truesize for each clone is set to packet-size +  
> sizeof(struct sk_buff), but input skb has minimum allocation size of  
> 9000 bytes (MAX_SINGLE_PACKET_SIZE) and maximum of 18944 bytes  
> (DEFAULT_HS_BURST_CAP_SIZE) (+ NET_IP_ALIGN). Doesn't this cause  
> truesize to be underestimated?

This has been discussed in a "TCP transmit performance regression"
thread some weeks ago.

More generally, skb_clone() is not a good idea in rx path.

I dont have the hardware so cannot send a formal patch.

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index b1112e7..3d9566f 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -1080,30 +1080,17 @@ static int smsc95xx_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
 				return 0;
 			}
 
-			/* last frame in this batch */
-			if (skb->len == size) {
-				if (dev->net->features & NETIF_F_RXCSUM)
-					smsc95xx_rx_csum_offload(skb);
-				skb_trim(skb, skb->len - 4); /* remove fcs */
-				skb->truesize = size + sizeof(struct sk_buff);
-
-				return 1;
-			}
-
-			ax_skb = skb_clone(skb, GFP_ATOMIC);
+			ax_skb = netdev_alloc_skb_ip_align(dev->net, size);
 			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);
+			memcpy(skb_put(ax_skb, size), packet, size);
 
 			if (dev->net->features & NETIF_F_RXCSUM)
 				smsc95xx_rx_csum_offload(ax_skb);
-			skb_trim(ax_skb, ax_skb->len - 4); /* remove fcs */
-			ax_skb->truesize = size + sizeof(struct sk_buff);
+			__skb_trim(ax_skb, ax_skb->len - 4); /* remove fcs */
 
 			usbnet_skb_return(dev, ax_skb);
 		}

Re: smsc75xx & smsc95xx, setting skb->truesize correctly?

[Jussi Kivilinna]

Quoting Eric Dumazet <eric.dumazet@gmail.com>:

> On Mon, 2012-08-20 at 17:57 +0300, Jussi Kivilinna wrote:
>> Hello,
>>
>> Is setting skb->truesize in smsc75xx and smsc95xx correct?
>> In smsc75xx/smsc95xx_rx_fixup(), input skb containing multiple packets
>> is cloned and truesize for each clone is set to packet-size +
>> sizeof(struct sk_buff), but input skb has minimum allocation size of
>> 9000 bytes (MAX_SINGLE_PACKET_SIZE) and maximum of 18944 bytes
>> (DEFAULT_HS_BURST_CAP_SIZE) (+ NET_IP_ALIGN). Doesn't this cause
>> truesize to be underestimated?
>
> This has been discussed in a "TCP transmit performance regression"
> thread some weeks ago.
>
> More generally, skb_clone() is not a good idea in rx path.

So all skb_clone use in drivers/net/usb/ should be removed/replaced  
with following?

>
> I dont have the hardware so cannot send a formal patch.

Neither do I, was looking for gigabit-usb dongle to buy and ended up  
checking various drivers.

I guess I could do and test this for rndis_host/rndis_wlan and add  
rx-skb recycling to usbnet core for drivers that do copy-break in  
rx_fixup.

-Jussi

>
> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
> index b1112e7..3d9566f 100644
> --- a/drivers/net/usb/smsc95xx.c
> +++ b/drivers/net/usb/smsc95xx.c
> @@ -1080,30 +1080,17 @@ static int smsc95xx_rx_fixup(struct usbnet  
> *dev, struct sk_buff *skb)
>  				return 0;
>  			}
>
> -			/* last frame in this batch */
> -			if (skb->len == size) {
> -				if (dev->net->features & NETIF_F_RXCSUM)
> -					smsc95xx_rx_csum_offload(skb);
> -				skb_trim(skb, skb->len - 4); /* remove fcs */
> -				skb->truesize = size + sizeof(struct sk_buff);
> -
> -				return 1;
> -			}
> -
> -			ax_skb = skb_clone(skb, GFP_ATOMIC);
> +			ax_skb = netdev_alloc_skb_ip_align(dev->net, size);
>  			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);
> +			memcpy(skb_put(ax_skb, size), packet, size);
>
>  			if (dev->net->features & NETIF_F_RXCSUM)
>  				smsc95xx_rx_csum_offload(ax_skb);
> -			skb_trim(ax_skb, ax_skb->len - 4); /* remove fcs */
> -			ax_skb->truesize = size + sizeof(struct sk_buff);
> +			__skb_trim(ax_skb, ax_skb->len - 4); /* remove fcs */
>
>  			usbnet_skb_return(dev, ax_skb);
>  		}
>
>
>
>

Re: smsc75xx & smsc95xx, setting skb->truesize correctly?

[Eric Dumazet]

On Wed, 2012-08-22 at 08:35 +0300, Jussi Kivilinna wrote:
> Quoting Eric Dumazet <eric.dumazet@gmail.com>:
> 
> > On Mon, 2012-08-20 at 17:57 +0300, Jussi Kivilinna wrote:
> >> Hello,
> >>
> >> Is setting skb->truesize in smsc75xx and smsc95xx correct?
> >> In smsc75xx/smsc95xx_rx_fixup(), input skb containing multiple packets
> >> is cloned and truesize for each clone is set to packet-size +
> >> sizeof(struct sk_buff), but input skb has minimum allocation size of
> >> 9000 bytes (MAX_SINGLE_PACKET_SIZE) and maximum of 18944 bytes
> >> (DEFAULT_HS_BURST_CAP_SIZE) (+ NET_IP_ALIGN). Doesn't this cause
> >> truesize to be underestimated?
> >
> > This has been discussed in a "TCP transmit performance regression"
> > thread some weeks ago.
> >
> > More generally, skb_clone() is not a good idea in rx path.
> 
> So all skb_clone use in drivers/net/usb/ should be removed/replaced  
> with following?

Doing a copy might be expensive on some low end hardware, so I can
understand why this skb_clone() idea was deployed years ago.

Gigabit r8169 has to perform the copy because of security issue, and so
far nobody complained of performance impact.

Best thing would be to not use large buffers from the beginning,
and switch to a frag idea.

(A large frame would needs 2 or 3 medium buffers, as done in ath9k)

Check https://gerrit.chromium.org/gerrit/#/c/18412/ 
and commit 0d95521ea74735826cb2e28bebf6a07392c75bfa (ath9k: use split rx
buffers to get rid of order-1 skb allocations)