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