Date:   Wed, 1 Feb 2023 19:50:08 +0100
From:   Maciej Fijalkowski <maciej.fijalkowski@intel.com>
To:     John Hickey <jjh@daedalian.us>
CC:     <anthony.l.nguyen@intel.com>,
        Jesse Brandeburg <jesse.brandeburg@intel.com>,
        "David S. Miller" <davem@davemloft.net>,
        "Eric Dumazet" <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Jesper Dangaard Brouer <hawk@kernel.org>,
        "John Fastabend" <john.fastabend@gmail.com>,
        Shujin Li <lishujin@kuaishou.com>,
        Jason Xing <xingwanli@kuaishou.com>,
        <intel-wired-lan@lists.osuosl.org>, <netdev@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <bpf@vger.kernel.org>
Subject: Re: [PATCH net v2] ixgbe: Panic during XDP_TX with > 64 CPUs
Message-ID: <Y9q0YDLVO+ndlaa8@boxer>
References: <20230131073815.181341-1-jjh@daedalian.us>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230131073815.181341-1-jjh@daedalian.us>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?3jvsb3jY/z7cFAdL1gYW7RiB9TdIOcXE6/F0fPa7g6GlEBGoUcC8wo/Z619d?=
 =?us-ascii?Q?zZDBAUDRdgipRF7+yPl4ihPl7oUtej3tnS7EOi00FWGu5TaTeUO6AyU3VqdQ?=
 =?us-ascii?Q?sSqwiZ/OuwfcWJ14JOMj5UKVwdOBsTUsa//1zEqPcwtQHK66TxajQ//69eW1?=
 =?us-ascii?Q?vAxtpraEHdA7wkuiA/S0+iQRRL45PrJgcNd+qv151m0mqSuacy3dQ0J8d7jV?=
 =?us-ascii?Q?Th9jG1ONZtjRIWF15fwK2DZfHecYTRqnmHQfQ4Hr+aYKKjQmCwnovFlfvLgG?=
 =?us-ascii?Q?n3yI6FzqNL8fRwFjUu0KLyg46H05Zjf6HHLKK9Fbmfjbl6cCsEmFQNeRZ8W5?=
 =?us-ascii?Q?gxIfIpfuTKQoujuCEC9Zjb6/o3WsaflL7Z5WU2fOX4HYfrpKBsimPwzqGzLZ?=
 =?us-ascii?Q?WnlthTNiqNlZXUp4I7E3Faany88zTlzKqGCawIsWNtdJbNsOwFCrRf09Mntw?=
 =?us-ascii?Q?AfMaAYHt1r/BUEPkXWtrhzRDVRJoHt6UujXPgjvUQIWqxdLCD6wvifypwubJ?=
 =?us-ascii?Q?AiV7iR0jrzi2ZCDbdL1SPITfuu/jL4VBsxoPI1cS233aWMX3W9TgwXq4oTMe?=
 =?us-ascii?Q?GYTbEDEKK0PecdgDztY4aqlZCxNnqpKOCeVdu4QG+OqAFW2Z7nbHJWAPxxP0?=
 =?us-ascii?Q?Ojh07m9nyLglQtPW26ujcgOwONpgAoUexasXScO8g2Qf+5lAgdltyN5mbf9D?=
 =?us-ascii?Q?zoqNWoSDMF2zOO69qwNSqZWYgcgmTXYtqDWWET0dqMRL9dOUR1DTMbiETDez?=
 =?us-ascii?Q?ZrhMC3+a0CeX9beR7Ipe4Xkoqe+OurBwPpiio0ppE6o/+GUpHF4rkjSLP6Te?=
 =?us-ascii?Q?aNEh2Y6ilmTngIrl5P9yekdInlY9eVQIvEHQmXvXY2WiG+UQ6Dan1BR4DJHr?=
 =?us-ascii?Q?pFKdskqm4vmMrU1fi7CizT4/PaeS0bxl/k98AJCy/9dFeY841nonULxY+I9E?=
 =?us-ascii?Q?O7KwdEG5CmCZeHjYvf5g967wrIsdMubX/EdokSSPNgOim1bE0nXF33Fn9VkN?=
 =?us-ascii?Q?E+sa70zCTfsl7tIGfzIR91AU9HBXXDQZY55ORHT5AOzPqwXsVJ1zc0b+89dj?=
 =?us-ascii?Q?eUvSvjgjx9CqHGXjxaZ3H7bQFgpxfpa6oy23/Dg2/dFgmqjmRFIQ+nNCSbFt?=
 =?us-ascii?Q?98j36noSMsqZBqKRtO8KanmvACJZ/hRMWhjMoSOAQ84ccostKUaOki8OKYik?=
 =?us-ascii?Q?85+EEBi8MG1n/T4dt66ZAtN+N7It8fZnrB4nL0fRMItwD0lKTG2G0il9p6fA?=
 =?us-ascii?Q?BK61QnvxVzOAqdIJQrYtNLE5cE+bAkplllpanR1Gpwkp0HRyzwwBNYsn/jtI?=
 =?us-ascii?Q?UqIL1EisE5acekoZTIcT0k9rZgCRDreFBQ3X+Kic9H1NWnxMU2TEwJwvhzQn?=
 =?us-ascii?Q?4zoV6nRp6yOlxuW/46dmnZh42ofuynWPtp7tPtez3+ynJCvoNYIDJTYoDV3e?=
 =?us-ascii?Q?3PkNL1uhDrz+TMLf5utCIOb9cWizAVjHQ3Tx+fP1UQJDTfUr37f8YShtyG0c?=
 =?us-ascii?Q?OxAT6OnG3X83Pm23P+eza/MssaKYxvnps6zBk2EmOUk1NTJCPWLaXbGLcgrS?=
 =?us-ascii?Q?8tg8F+PHbezzp+rz7tN6Sc4outnijRDUlaDoh1DnHiduKqqHQAOTM3zln2As?=
 =?us-ascii?Q?OTEvE6eLJfZmaDxWwZ+AZGc=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 62e3d168-3b2b-47c8-5aac-08db04852849
X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB6117.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Feb 2023 18:50:15.7989
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: rGCffO51ENve1UT1P+85e76TxdE571vJx3bYXLYvzkn8gALXr1n3QtafPTu7CItFun/r3clCFWIsWTOXUPgYlu8IOePlVJxZ+/jTDX4ZX/I=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN0PR11MB6160
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Mon, Jan 30, 2023 at 11:38:15PM -0800, John Hickey wrote:
> In commit 'ixgbe: let the xdpdrv work with more than 64 cpus'
> (4fe815850bdc), support was added to allow XDP programs to run on systems
> with more than 64 CPUs by locking the XDP TX rings and indexing them
> using cpu % 64 (IXGBE_MAX_XDP_QS).
> 
> Upon trying this out patch via the Intel 5.18.6 out of tree driver
> on a system with more than 64 cores, the kernel paniced with an
> array-index-out-of-bounds at the return in ixgbe_determine_xdp_ring in
> ixgbe.h, which means ixgbe_determine_xdp_q_idx was just returning the
> cpu instead of cpu % IXGBE_MAX_XDP_QS.

I'd like to ask you to include the splat you got in the commit message.

> 
> I think this is how it happens:
> 
> Upon loading the first XDP program on a system with more than 64 CPUs,
> ixgbe_xdp_locking_key is incremented in ixgbe_xdp_setup.  However,
> immediately after this, the rings are reconfigured by ixgbe_setup_tc.
> ixgbe_setup_tc calls ixgbe_clear_interrupt_scheme which calls
> ixgbe_free_q_vectors which calls ixgbe_free_q_vector in a loop.
> ixgbe_free_q_vector decrements ixgbe_xdp_locking_key once per call if
> it is non-zero.  Commenting out the decrement in ixgbe_free_q_vector
> stopped my system from panicing.
> 
> I suspect to make the original patch work, I would need to load an XDP
> program and then replace it in order to get ixgbe_xdp_locking_key back
> above 0 since ixgbe_setup_tc is only called when transitioning between
> XDP and non-XDP ring configurations, while ixgbe_xdp_locking_key is
> incremented every time ixgbe_xdp_setup is called.
> 
> Also, ixgbe_setup_tc can be called via ethtool --set-channels, so this
> becomes another path to decrement ixgbe_xdp_locking_key to 0 on systems
> with greater than 64 CPUs.
> 
> For this patch, I have changed static_branch_inc to static_branch_enable
> in ixgbe_setup_xdp.  We aren't counting references and I don't see any
> reason to turn it off, since all the locking appears to be in the XDP_TX
> path, which isn't run if a XDP program isn't loaded.
> 
> Fixes: 4fe815850bdc ("ixgbe: let the xdpdrv work with more than 64 cpus")
> Signed-off-by: John Hickey <jjh@daedalian.us>
> ---
> v1 -> v2:
> 	Added Fixes and net tag.  No code changes.
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c  | 3 ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 +-
>  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 f8156fe4b1dc..0ee943db3dc9 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c
> @@ -1035,9 +1035,6 @@ static void ixgbe_free_q_vector(struct ixgbe_adapter *adapter, int v_idx)
>  	adapter->q_vector[v_idx] = NULL;
>  	__netif_napi_del(&q_vector->napi);
>  
> -	if (static_key_enabled(&ixgbe_xdp_locking_key))
> -		static_branch_dec(&ixgbe_xdp_locking_key);

Yeah calling this per each qvector is *very* unbalanced approach whereas
you bump it single time when loading xdp prog.

> -
>  	/*
>  	 * after a call to __netif_napi_del() napi may still be used and
>  	 * ixgbe_get_stats64() might access the rings on this vector,
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index ab8370c413f3..cd2fb72c67be 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -10283,7 +10283,7 @@ static int ixgbe_xdp_setup(struct net_device *dev, struct bpf_prog *prog)
>  	if (nr_cpu_ids > IXGBE_MAX_XDP_QS * 2)
>  		return -ENOMEM;
>  	else if (nr_cpu_ids > IXGBE_MAX_XDP_QS)
> -		static_branch_inc(&ixgbe_xdp_locking_key);
> +		static_branch_enable(&ixgbe_xdp_locking_key);

Now that you removed static_branch_dec you probably need a counter part
(static_branch_disable) at appriopriate place.

>  
>  	old_prog = xchg(&adapter->xdp_prog, prog);
>  	need_reset = (!!prog != !!old_prog);
> -- 
> 2.37.2
>