[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] xen-netback: maybe_pull_tail questions



On 28/11/13 16:57, Paul Durrant wrote:
-----Original Message-----
From: Zoltan Kiss
Sent: 28 November 2013 16:40
To: xen-devel@xxxxxxxxxxxxxxxxxxxx; Wei Liu; Ian Campbell; Paul Durrant
Subject: xen-netback: maybe_pull_tail questions

Hi,

I've found some things in the newest checksum handling code which I
don't fully get. Maybe it's my fault, or maybe it's a problem indeed.
More exactly, the header_size variable we pass to maybe_pull_tail()
seems odd. In checksum_setup_ip() we start with:

        struct iphdr *iph = (void *)skb->data;
...
        off = sizeof(struct iphdr);

        header_size = skb->network_header + off + MAX_IPOPTLEN;

I agree the MAX_IPOPTLEN is not needed here. We only need the base header at 
this stage.
Good, I will delete that in a subsequent patch.


        maybe_pull_tail(skb, header_size);

        off = iph->ihl * 4;

First, why don't we set off to the real IP header length at the first
place if we already know that?

We don't know it yet. You have the read the field out of the header to know how 
long the header is, so you'd better have at least the base header in the linear 
area before trying to deference iph.
Indeed, I missed the unlikely case when the linear buffer do not have the IP header yet.


Second, skb->network_header was just reset in xenvif_tx_submit() before
we called this function, and it contains the size of the headroom (32
bytes, if I'm correct, set by skb_reserve(skb, NET_SKB_PAD +
NET_IP_ALIGN) in xenvif_tx_build_gops()). I think we need sizeof(struct
ethhdr) here, or something like that, and no MAX_IPOPTLEN, as off should
contain the right size already.

 From my reading the call to eth_type_trans() will pull in the mac header and then 
skb_reset_network_header() should make sure that skb->network_header points 
just after that i.e. the start of the IP header.
Oh, I missed that pulling. So it moved skb->data and decreased len, that explains why this pull_tail triggered.


And this applies to other places where we set header_size based on
skb->network_header.
I noticed this thing when I checked some ftrace outputs on 3.12, and
I've found that maybe_pull_tail cause pulling despite in
xenvif_tx_submit we already checked if the linear buffer need more data
to reach PKT_PROT_LEN ( = 128).

Hmm, looking at it again I wonder whether header_size should be factoring in 
skb_headroom()?
I don't think so. skb->len doesn't include the headroom (skb_pull decrease it when increasing headroom), so we should just remove skb->network_header from header_size calculations. I will send a patch shortly



   Paul

Here skb->network_header + off +
MAX_IPOPTLEN should be 32 + 20 + 40 = 92, so we shouldn't do it. Or am I
missing something?
As a workaround, I've commented out this maybe_pull_tail(), and works
fine.

Regards,

Zoli


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.