Linux network development

Jussi Kivilinna | 20 Aug 2012 16:57

smsc75xx & smsc95xx, setting skb->truesize correctly?

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

headers

Eric Dumazet | 21 Aug 2012 11:46

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

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
 <at>  <at>  -1080,30 +1080,17  <at>  <at>  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);

(Continue reading)

headers

Jussi Kivilinna | 22 Aug 2012 07:35

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

Quoting Eric Dumazet <eric.dumazet <at> 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.

(Continue reading)

headers

Eric Dumazet | 22 Aug 2012 08:34

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

On Wed, 2012-08-22 at 08:35 +0300, Jussi Kivilinna wrote:
> Quoting Eric Dumazet <eric.dumazet <at> 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.

(Continue reading)