From mboxrd@z Thu Jan 1 00:00:00 1970 From: Zoltan Kiss Subject: Re: xen-netback: maybe_pull_tail questions Date: Thu, 28 Nov 2013 18:04:52 +0000 Message-ID: <529785C4.3040602@citrix.com> References: <529771D1.3060100@citrix.com> <9AAE0902D5BC7E449B7C8E4E778ABCD0195121@AMSPEX01CL01.citrite.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Vm5xp-0007KF-7b for xen-devel@lists.xenproject.org; Thu, 28 Nov 2013 18:05:21 +0000 In-Reply-To: <9AAE0902D5BC7E449B7C8E4E778ABCD0195121@AMSPEX01CL01.citrite.net> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Paul Durrant , "xen-devel@lists.xenproject.org" , Wei Liu , Ian Campbell List-Id: xen-devel@lists.xenproject.org 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