From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754116AbcISKhG (ORCPT ); Mon, 19 Sep 2016 06:37:06 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33728 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750745AbcISKg6 (ORCPT ); Mon, 19 Sep 2016 06:36:58 -0400 From: Vitaly Kuznetsov To: David Vrabel Cc: David Miller , , , , , , Subject: Re: [PATCH net-next RESEND] xen-netfront: avoid packet loss when ethernet header crosses page boundary References: <1474023554-24520-1-git-send-email-vkuznets@redhat.com> <20160918.222607.1930094684278444086.davem@davemloft.net> <87mvj4xk8i.fsf@vitty.brq.redhat.com> <57DFBCB9.1090500@citrix.com> Date: Mon, 19 Sep 2016 12:36:55 +0200 In-Reply-To: <57DFBCB9.1090500@citrix.com> (David Vrabel's message of "Mon, 19 Sep 2016 11:23:53 +0100") Message-ID: <87intsxjjs.fsf@vitty.brq.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Mon, 19 Sep 2016 10:36:58 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org David Vrabel writes: > On 19/09/16 11:22, Vitaly Kuznetsov wrote: >> David Miller writes: >> >>> From: Vitaly Kuznetsov >>> Date: Fri, 16 Sep 2016 12:59:14 +0200 >>> >>>> @@ -595,6 +596,19 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) >>>> offset = offset_in_page(skb->data); >>>> len = skb_headlen(skb); >>>> >>>> + /* The first req should be at least ETH_HLEN size or the packet will be >>>> + * dropped by netback. >>>> + */ >>>> + if (unlikely(PAGE_SIZE - offset < ETH_HLEN)) { >>>> + nskb = skb_copy(skb, GFP_ATOMIC); >>>> + if (!nskb) >>>> + goto drop; >>>> + dev_kfree_skb_any(skb); >>>> + skb = nskb; >>>> + page = virt_to_page(skb->data); >>>> + offset = offset_in_page(skb->data); >>>> + } >>>> + >>>> spin_lock_irqsave(&queue->tx_lock, flags); >>> >>> I think you also have to recalculate 'len' in this case too, as >>> skb_headlen() will definitely be different for nskb. >>> >>> In fact, I can't see how this code can work properly without that fix. >> >> Thank you for your feedback David, >> >> in my testing (even when I tried doing skb_copy() for all skbs >> unconditionally) skb_headlen(nskb) always equals 'len' so I was under an >> impression that both 'skb->len' and 'skb->data_len' remain the same when >> we do skb_copy(). However, in case you think there are cases when >> headlen changes, I see no problem with re-calculating 'len' as it won't >> bring any significant performace penalty compared to the already added >> skb_copy(). > > I think you can move the len = skb_headlen(skb) after the if, no need to > recalculate it. Sorry, I was too fast with sending 'v2' and did the other way around. I'll do v3. -- Vitaly