From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4139CC388F9 for ; Thu, 19 Nov 2020 22:58:48 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id EDF3321D40 for ; Thu, 19 Nov 2020 22:58:47 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726387AbgKSW6r (ORCPT ); Thu, 19 Nov 2020 17:58:47 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57896 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726313AbgKSW6q (ORCPT ); Thu, 19 Nov 2020 17:58:46 -0500 Received: from Chamillionaire.breakpoint.cc (Chamillionaire.breakpoint.cc [IPv6:2a0a:51c0:0:12e:520::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id AD8B0C0613CF for ; Thu, 19 Nov 2020 14:58:46 -0800 (PST) Received: from fw by Chamillionaire.breakpoint.cc with local (Exim 4.92) (envelope-from ) id 1kfst0-0005DD-0a; Thu, 19 Nov 2020 23:58:42 +0100 Date: Thu, 19 Nov 2020 23:58:42 +0100 From: Florian Westphal To: "Ramsay, Lincoln" Cc: Florian Westphal , Igor Russkikh , "David S. Miller" , Jakub Kicinski , "netdev@vger.kernel.org" , Dmitry Bogdanov Subject: Re: [PATCH v3] aquantia: Remove the build_skb path Message-ID: <20201119225842.GK15137@breakpoint.cc> References: <2b392026-c077-2871-3492-eb5ddd582422@marvell.com> <20201119221510.GI15137@breakpoint.cc> <20201119222800.GJ15137@breakpoint.cc> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Ramsay, Lincoln wrote: > When performing IPv6 forwarding, there is an expectation that SKBs > will have some headroom. When forwarding a packet from the aquantia > driver, this does not always happen, triggering a kernel warning. > > The build_skb path fails to allow for an SKB header, but the hardware For build_skb path to work the buffer scheme would need to be changed to reserve headroom, so yes, I think that the proposed patch is the most convenient solution. > buffer it is built around won't allow for this anyway. Just always use the > slower codepath that copies memory into an allocated SKB. I thought this changes the driver to always copy the entire packet, but thats not true, see below. > It seems that skb_headroom is only 14, when it is expected to be >= 16. Yes, kernel expects to have some headroom in skbs. > aq_ring.c has this code (edited slightly for brevity): > > if (buff->is_eop && buff->len <= AQ_CFG_RX_FRAME_MAX - AQ_SKB_ALIGN) { > skb = build_skb(aq_buf_vaddr(&buff->rxdata), AQ_CFG_RX_FRAME_MAX); > skb_put(skb, buff->len); > } else { > skb = napi_alloc_skb(napi, AQ_CFG_RX_HDR_SIZE); > > There is a significant difference between the SKB produced by these 2 code paths. When napi_alloc_skb creates an SKB, there is a certain amount of headroom reserved. The same pattern appears to be used in all of the other ethernet drivers I have looked at. However, this is not done in the build_skb codepath. I think the above should be part of the commit message rather than this meta-space (which gets removed by git-am). > + skb = napi_alloc_skb(napi, AQ_CFG_RX_HDR_SIZE); > + if (unlikely(!skb)) { AQ_CFG_RX_HDR_SIZE is 256 byte, so for larger packets ... > + memcpy(__skb_put(skb, hdr_len), aq_buf_vaddr(&buff->rxdata), > + ALIGN(hdr_len, sizeof(long))); This only copies the initial part and then... > + if (buff->len - hdr_len > 0) { > + skb_add_rx_frag(skb, 0, buff->rxdata.page, > + buff->rxdata.pg_off + hdr_len, > + buff->len - hdr_len, > AQ_CFG_RX_FRAME_MAX); The rest is added as a frag. IOW, this patch looks good to me, but could you update the commit message so it becomes clear that this doesn't result in a full copy? Perhaps something like: 'Just always use the napi_alloc_skb() code path that passes the buffer as a page fragment', or similar. Thanks.