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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 412DFFA3741 for ; Mon, 31 Oct 2022 20:18:29 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230039AbiJaUS1 (ORCPT ); Mon, 31 Oct 2022 16:18:27 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51400 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229721AbiJaUSY (ORCPT ); Mon, 31 Oct 2022 16:18:24 -0400 Received: from vps0.lunn.ch (vps0.lunn.ch [156.67.10.101]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 587C6DD; Mon, 31 Oct 2022 13:18:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lunn.ch; s=20171124; h=In-Reply-To:Content-Disposition:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:From:Sender:Reply-To:Subject: Date:Message-ID:To:Cc:MIME-Version:Content-Type:Content-Transfer-Encoding: Content-ID:Content-Description:Content-Disposition:In-Reply-To:References; bh=k76g8mPvFlEsezniA/wfzLnncWnwoBf8Gjp4kw6Bjfs=; b=gPhd/zBywntCe7Ln240c4GIlxd R5bz4c6omlPxFAvMBH0RerjVxsZHIqkzcpoo0otBjauOSEhJCrxSx6xnq1Qy1Tjo58NwOVIH5z9O7 hYNrxQfWOmWnlSffTqerNBJGgpMf9TW+uAZEm2B7XskYuElv2Bf8Ok9xo8+iJv/fxovE=; Received: from andrew by vps0.lunn.ch with local (Exim 4.94.2) (envelope-from ) id 1opbDY-00137a-EL; Mon, 31 Oct 2022 21:17:08 +0100 Date: Mon, 31 Oct 2022 21:17:08 +0100 From: Andrew Lunn To: Shenwei Wang Cc: "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Alexei Starovoitov , Daniel Borkmann , Jesper Dangaard Brouer , John Fastabend , "netdev@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "imx@lists.linux.dev" , kernel test robot Subject: Re: [EXT] Re: [PATCH v2 1/1] net: fec: add initial XDP support Message-ID: References: <20221031162200.1997788-1-shenwei.wang@nxp.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > > > + cbd_base = rxq->bd.base; > > > + if (bpf->prog) > > > + rxq->bd.ring_size = XDP_RX_RING_SIZE; > > > + else > > > + rxq->bd.ring_size = RX_RING_SIZE; > > > + size = dsize * rxq->bd.ring_size; > > > + cbd_base = (struct bufdesc *)(((void *)cbd_base) + size); > > > + rxq->bd.last = (struct bufdesc *)(((void > > > + *)cbd_base) - dsize); > > > > This does not look safe. netif_tx_disable(dev) will stop new transmissions, but > > the hardware can be busy receiving, DMAing frames, using the descriptors, etc. > > Modifying rxq->bd.last in particular seems risky. I think you need to disable the > > receiver, wait for it to indicate it really has stopped, and only then make these > > modifications. > > > > Sounds reasonable. How about moving the codes of updating ring size to the place > right after the enet reset inside the fec_restart? This should clear those risky corner > cases. That sounds reasonable. But please add some comments. The driver has RX_RING_SIZE elements allocated, but you are only using a subset. This needs to be clear for when somebody implements the ethtool --rings option. And i still think it would be good to implement that code. As your numbers show, the ring size does affect performance, and it is hard to know if your hard coded XDP_RX_RING_SIZE is the right value, depending on what the eBPF program is doing. If the ethtool option was provided, it allows users to benchmark different ring sizes for their workload. Andrew