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

[Zoltan Kiss <zoltan.kiss@xxxxxxxxxx>]

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