xen-netback: maybe_pull_tail questions

xen-netback: maybe_pull_tail questions

[Zoltan Kiss]

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;
	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?
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.
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). 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

Re: xen-netback: maybe_pull_tail questions

[Paul Durrant]

> -----Original Message-----
> From: Zoltan Kiss
> Sent: 28 November 2013 16:40
> To: xen-devel@lists.xenproject.org; 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

Re: xen-netback: maybe_pull_tail questions

[Zoltan Kiss]

On 28/11/13 16:57, Paul Durrant wrote:
>> -----Original Message-----
>> From: Zoltan Kiss
>> Sent: 28 November 2013 16:40
>> To: xen-devel@lists.xenproject.org; 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

Re: xen-netback: maybe_pull_tail questions

[Paul Durrant]

> -----Original Message-----
> From: Zoltan Kiss
> Sent: 28 November 2013 18:05
> To: Paul Durrant; xen-devel@lists.xenproject.org; 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@lists.xenproject.org; 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