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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----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.

>       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.

> 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.

> 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()?

  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