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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Zoltan Kiss
> Sent: 28 November 2013 18:05
> To: Paul Durrant; xen-devel@xxxxxxxxxxxxxxxxxxxx; Wei Liu; Ian Campbell
> Subject: Re: 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
> 

Ok. Yes, I had the misconception that skb_headlen() was equivalent to skb->tail 
- skb->head, but it appears to be equivalent to skb->tail - skb_data.

  Paul

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