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=-15.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable 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 96B69C432BE for ; Tue, 24 Aug 2021 15:48:18 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 7A70A60725 for ; Tue, 24 Aug 2021 15:48:18 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238447AbhHXPs7 (ORCPT ); Tue, 24 Aug 2021 11:48:59 -0400 Received: from mga12.intel.com ([192.55.52.136]:27746 "EHLO mga12.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S238287AbhHXPs6 (ORCPT ); Tue, 24 Aug 2021 11:48:58 -0400 X-IronPort-AV: E=McAfee;i="6200,9189,10086"; a="196914374" X-IronPort-AV: E=Sophos;i="5.84,347,1620716400"; d="scan'208";a="196914374" Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 24 Aug 2021 08:48:14 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.84,347,1620716400"; d="scan'208";a="455619246" Received: from ranger.igk.intel.com ([10.102.21.164]) by fmsmga007.fm.intel.com with ESMTP; 24 Aug 2021 08:48:06 -0700 Date: Tue, 24 Aug 2021 17:32:25 +0200 From: Maciej Fijalkowski To: Jason Xing Cc: Jesper Dangaard Brouer , Jesse Brandeburg , "Nguyen, Anthony L" , David Miller , kuba@kernel.org, ast@kernel.org, daniel@iogearbox.net, hawk@kernel.org, john.fastabend@gmail.com, andrii@kernel.org, kafai@fb.com, songliubraving@fb.com, yhs@fb.com, kpsingh@kernel.org, brouer@redhat.com, intel-wired-lan@lists.osuosl.org, netdev , LKML , bpf@vger.kernel.org, Jason Xing , Shujin Li , =?iso-8859-1?B?zfFpZ28=?= Huguet Subject: Re: [PATCH] ixgbe: let the xdpdrv work with more than 64 cpus Message-ID: <20210824153225.GA16546@ranger.igk.intel.com> References: <20210824104918.7930-1-kerneljasonxing@gmail.com> <59dff551-2d52-5ecc-14ac-4a6ada5b1275@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.12.1 (2019-06-15) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Aug 24, 2021 at 11:23:29PM +0800, Jason Xing wrote: > On Tue, Aug 24, 2021 at 9:32 PM Jesper Dangaard Brouer > wrote: > > > > > > > > On 24/08/2021 12.49, kerneljasonxing@gmail.com wrote: > > > From: Jason Xing > > > > > > Originally, ixgbe driver doesn't allow the mounting of xdpdrv if the > > > server is equipped with more than 64 cpus online. So it turns out that > > > the loading of xdpdrv causes the "NOMEM" failure. > > > > > > Actually, we can adjust the algorithm and then make it work, which has > > > no harm at all, only if we set the maxmium number of xdp queues. > > > > This is not true, it can cause harm, because XDP transmission queues are > > used without locking. See drivers ndo_xdp_xmit function ixgbe_xdp_xmit(). > > As driver assumption is that each CPU have its own XDP TX-queue. Thanks Jesper for chiming in. > > > > Point taken. I indeed miss that part which would cause bad behavior if it > happens. > > At this point, I think I should find all the allocation and use of XDP > related, say, > queues and rings, then adjust them all? > > Let's say if the server is shipped with 128 cpus, we could map 128 cpus to 64 > rings in the function ixgbe_xdp_xmit(). However, it sounds a little bit odd. > > Do you think that it makes any sense? We need a fallback path for ixgbe. I did the following for ice: https://x-lore.kernel.org/bpf/20210819120004.34392-9-maciej.fijalkowski@intel.com/T/#u > > Thanks, > Jason > > > This patch is not a proper fix. > > > > I do think we need a proper fix for this issue on ixgbe. > > > > > > > Fixes: 33fdc82f08 ("ixgbe: add support for XDP_TX action") > > > Co-developed-by: Shujin Li > > > Signed-off-by: Shujin Li > > > Signed-off-by: Jason Xing > > > --- > > > drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c | 2 +- > > > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 3 --- > > > 2 files changed, 1 insertion(+), 4 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c > > > index 0218f6c..5953996 100644 > > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c > > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c > > > @@ -299,7 +299,7 @@ static void ixgbe_cache_ring_register(struct ixgbe_adapter *adapter) > > > > > > static int ixgbe_xdp_queues(struct ixgbe_adapter *adapter) > > > { > > > - return adapter->xdp_prog ? nr_cpu_ids : 0; > > > + return adapter->xdp_prog ? min_t(int, MAX_XDP_QUEUES, nr_cpu_ids) : 0; > > > } > > > > > > #define IXGBE_RSS_64Q_MASK 0x3F > > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > > index 14aea40..b36d16b 100644 > > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > > @@ -10130,9 +10130,6 @@ static int ixgbe_xdp_setup(struct net_device *dev, struct bpf_prog *prog) > > > return -EINVAL; > > > } > > > > > > - if (nr_cpu_ids > MAX_XDP_QUEUES) > > > - return -ENOMEM; > > > - > > > old_prog = xchg(&adapter->xdp_prog, prog); > > > need_reset = (!!prog != !!old_prog); > > > > > > > > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Maciej Fijalkowski Date: Tue, 24 Aug 2021 17:32:25 +0200 Subject: [Intel-wired-lan] [PATCH] ixgbe: let the xdpdrv work with more than 64 cpus In-Reply-To: References: <20210824104918.7930-1-kerneljasonxing@gmail.com> <59dff551-2d52-5ecc-14ac-4a6ada5b1275@redhat.com> Message-ID: <20210824153225.GA16546@ranger.igk.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: intel-wired-lan@osuosl.org List-ID: On Tue, Aug 24, 2021 at 11:23:29PM +0800, Jason Xing wrote: > On Tue, Aug 24, 2021 at 9:32 PM Jesper Dangaard Brouer > wrote: > > > > > > > > On 24/08/2021 12.49, kerneljasonxing at gmail.com wrote: > > > From: Jason Xing > > > > > > Originally, ixgbe driver doesn't allow the mounting of xdpdrv if the > > > server is equipped with more than 64 cpus online. So it turns out that > > > the loading of xdpdrv causes the "NOMEM" failure. > > > > > > Actually, we can adjust the algorithm and then make it work, which has > > > no harm at all, only if we set the maxmium number of xdp queues. > > > > This is not true, it can cause harm, because XDP transmission queues are > > used without locking. See drivers ndo_xdp_xmit function ixgbe_xdp_xmit(). > > As driver assumption is that each CPU have its own XDP TX-queue. Thanks Jesper for chiming in. > > > > Point taken. I indeed miss that part which would cause bad behavior if it > happens. > > At this point, I think I should find all the allocation and use of XDP > related, say, > queues and rings, then adjust them all? > > Let's say if the server is shipped with 128 cpus, we could map 128 cpus to 64 > rings in the function ixgbe_xdp_xmit(). However, it sounds a little bit odd. > > Do you think that it makes any sense? We need a fallback path for ixgbe. I did the following for ice: https://x-lore.kernel.org/bpf/20210819120004.34392-9-maciej.fijalkowski at intel.com/T/#u > > Thanks, > Jason > > > This patch is not a proper fix. > > > > I do think we need a proper fix for this issue on ixgbe. > > > > > > > Fixes: 33fdc82f08 ("ixgbe: add support for XDP_TX action") > > > Co-developed-by: Shujin Li > > > Signed-off-by: Shujin Li > > > Signed-off-by: Jason Xing > > > --- > > > drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c | 2 +- > > > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 3 --- > > > 2 files changed, 1 insertion(+), 4 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c > > > index 0218f6c..5953996 100644 > > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c > > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c > > > @@ -299,7 +299,7 @@ static void ixgbe_cache_ring_register(struct ixgbe_adapter *adapter) > > > > > > static int ixgbe_xdp_queues(struct ixgbe_adapter *adapter) > > > { > > > - return adapter->xdp_prog ? nr_cpu_ids : 0; > > > + return adapter->xdp_prog ? min_t(int, MAX_XDP_QUEUES, nr_cpu_ids) : 0; > > > } > > > > > > #define IXGBE_RSS_64Q_MASK 0x3F > > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > > index 14aea40..b36d16b 100644 > > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > > @@ -10130,9 +10130,6 @@ static int ixgbe_xdp_setup(struct net_device *dev, struct bpf_prog *prog) > > > return -EINVAL; > > > } > > > > > > - if (nr_cpu_ids > MAX_XDP_QUEUES) > > > - return -ENOMEM; > > > - > > > old_prog = xchg(&adapter->xdp_prog, prog); > > > need_reset = (!!prog != !!old_prog); > > > > > > > >