Received-SPF: pass (google.com: domain of linux-kernel+bounces-59880-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
References: <20240208030702.27296-1-jdamato@fastly.com>
 <8c083e6d-5fcd-4557-88dd-0f95acdbc747@gmail.com>
 <871q9mz1a0.fsf@nvidia.com> <20240209192404.GA1430@fastly.com>
User-agent: mu4e 1.10.8; emacs 28.2
From: Rahul Rameshbabu <rrameshbabu@nvidia.com>
To: Joe Damato <jdamato@fastly.com>
Cc: Tariq Toukan <ttoukan.linux@gmail.com>, linux-kernel@vger.kernel.org,
 netdev@vger.kernel.org, tariqt@nvidia.com, Saeed Mahameed
 <saeedm@nvidia.com>, Leon Romanovsky <leon@kernel.org>, "David S. Miller"
 <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski
 <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Richard Cochran
 <richardcochran@gmail.com>, Gal Pressman <gal@nvidia.com>, Vadim Fedorenko
 <vadim.fedorenko@linux.dev>, "open list:MELLANOX MLX5 core VPI driver"
 <linux-rdma@vger.kernel.org>
Subject: Re: [PATCH net-next v3] net/mlx5e: link NAPI instances to queues
 and IRQs
Date: Fri, 09 Feb 2024 11:26:33 -0800
In-reply-to: <20240209192404.GA1430@fastly.com>
Message-ID: <87jzndpham.fsf@nvidia.com>
Content-Type: text/plain
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?y9H6qvRpRA8S3RUCHObNKawYYZZr6WltqmnEucPv8ztuXH37BX3StGYuQpkx?=
 =?us-ascii?Q?N8vcMz/RUmkYfyrf2sui+YS/dnTeZ8q+LH3RYXFfkw15WiVaXq5aOtQSwSNc?=
 =?us-ascii?Q?NQoXPjv6RwnvmUY0klTnaqicH2ImPANQFn48GcynRsB+5f4urFkvxpbRzxRf?=
 =?us-ascii?Q?6DgdwlkQjuYR6N5ktlT7n44xjfEXzJRinjkbZhsgnWbJpRecPD1XU+BG3Zrg?=
 =?us-ascii?Q?PEMK5Dmyr1cesj0fdFfZK7zg5GQg4FsIWMAal1vBtR4/Cm7qD3wOY/HlOqLi?=
 =?us-ascii?Q?KbHdzcszqUSno9fiisoZuMYKYsee4T2KebAKNbxfipGzaBrJlqA7H8BrBY4a?=
 =?us-ascii?Q?62fF1WLZRYUGY3GLE5ro1WJKZRNA0nxLSwfTVO7GAI4f7OPmpjRPkFG557hA?=
 =?us-ascii?Q?BGRR0wG1IxcVC6jzbS81mJRA5yQ/feawShyOENFKzlQmkVT5EbYvFFqa/ous?=
 =?us-ascii?Q?4v8loNHiIlk4x7bEymS/k+SSug7afBPKv+2mKh/2qgdGbeLL97fzjQrI8CIS?=
 =?us-ascii?Q?23NfbZuRW6BJ/nWIcdb/nUz0++v0A8SHuuZ9U5538gS+j0U3Qa0x8GYjgBtc?=
 =?us-ascii?Q?dFdQsIF73mg5UKxitenqhkHZ87efzyqvra9BDr7a8IbUMBgkqVsSG7NN1Wqu?=
 =?us-ascii?Q?QZ/dujoJoPJy8bMxI7SWGDSzKYacoW0UXFoYm0US5o42SvVWbXjc2sRgTVJz?=
 =?us-ascii?Q?A0csuoroYT/aNzwhZ6G8zpGUThr/BUcZMj6JUf9dDiEoG2LBA+OK17KSztcW?=
 =?us-ascii?Q?NHn4fgyzFwaLhJaykWgrE0hGL97MNUFO/v7oTVycb4ErL9jOloAeairVOX94?=
 =?us-ascii?Q?TROAgcGOs2VDKsvye3KiPMbGpkix5NYwP9wHsQPo0xELQbOeOyc5lOg96aRT?=
 =?us-ascii?Q?+n6P6BesKB/AuGPY+reYH/ukRrXewQii1IFnQW2mecgW7clg/Gz5K6NW0YEn?=
 =?us-ascii?Q?JbFPoFg93O7er9BIxSEy/NOdNv3XrBcUz7hw2RQutWn5PvLy1sVQMR0/4mwV?=
 =?us-ascii?Q?20Qgj+7b8yRfzHCX28zFhLJI/2OdbJcgjGnvQK5Dcb6c0vlpaSNfbP41pR6K?=
 =?us-ascii?Q?byTSpN9epGtKDQZZZjp1+8YIovb//lsSnPronktsyxNa075SCdh/AvGf/juR?=
 =?us-ascii?Q?/WF4bJ0+bMgu2gc7reX7uhk6cSkGhWwmCrdMLPwBwUmBEG5h9+ETFdW+MkvG?=
 =?us-ascii?Q?X8/OeUQ2KVXJQZwJkItKw/3Djd2IhwffPqKryp0y6oKZUx7TlM3HpuiIbp4X?=
 =?us-ascii?Q?cTuKDL/cuRsA0WfCu/hCZtOCH4SQFQNREmU6zgAzFTrbxE/nceky/bgFIdYB?=
 =?us-ascii?Q?XirU9h/i4yqL9+YUlNNx9LGN62bykUf6TqN/UwF+Es2P2hAfn3zq63JtSFUq?=
 =?us-ascii?Q?cKjwHrlGL+90lqn8tVY+1Pu0ord6SzOgCcatCRcdlPvEbQyWofjOoYduNWXH?=
 =?us-ascii?Q?E7kpCiCJvunXZqMsI5wgKVn2mWjfQO8PrWyR1fh4lHE3dRvVsRW6QU6We3Fq?=
 =?us-ascii?Q?C1UsoNtysyOK0YyVawYAB8pdmat3oQ+AYqFdrqTFPRmSKICvg73GROjCg7uf?=
 =?us-ascii?Q?nynb4a9lgDuHiGI3T4J4M9ExgW9W0mbjVHDnl5BjcUihjEm9UTW2zXrbwYho?=
 =?us-ascii?Q?Fw=3D=3D?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: abe2a20c-ebea-4890-d766-08dc29a57e7f
X-MS-Exchange-CrossTenant-AuthSource: BYAPR12MB2743.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Feb 2024 19:29:54.9687
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: kkWQM8yj3MweenonbZgbTFe3Es+HdZJVXi//e3rpCpBjXHF+buLMlF4zvUMl6Ahk+zPxGJe7n8acoAGpeV9K8w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: LV2PR12MB5725

On Fri, 09 Feb, 2024 11:24:04 -0800 Joe Damato <jdamato@fastly.com> wrote:
> On Thu, Feb 08, 2024 at 08:43:57PM -0800, Rahul Rameshbabu wrote:
>> On Thu, 08 Feb, 2024 21:13:25 +0200 Tariq Toukan <ttoukan.linux@gmail.com> wrote:
>> > On 08/02/2024 5:07, Joe Damato wrote:
>> >> Make mlx5 compatible with the newly added netlink queue GET APIs.
>> >> Signed-off-by: Joe Damato <jdamato@fastly.com>
>> >> ---
>> 
>> Just came back from testing this code. Let me make one cosmetic point
>> here. I noticed this patch has a line that is past 90 characters. Would
>> be nice to get it wrapped in the next version. We use 90 instead of 80
>> characters for the line wrap in the mlx5 driver because of firmware
>> command interface related code would lead to very hard to read lines if
>> wrapped at 80.
>> 
>>   https://patchwork.kernel.org/project/netdevbpf/patch/20240208030702.27296-1-jdamato@fastly.com/
>
> OK, I had wrapped them in the next version I was going to send to 80, but
> I'll adjust that to 90.
>
>> >> v2 -> v3:
>> >>    - Fix commit message subject
>> >>    - call netif_queue_set_napi in mlx5e_ptp_activate_channel and
>> >>      mlx5e_ptp_deactivate_channel to enable/disable NETDEV_QUEUE_TYPE_RX for
>> >>      the PTP channel.
>> >>    - Modify mlx5e_activate_txqsq and mlx5e_deactivate_txqsq to set
>> >>      NETDEV_QUEUE_TYPE_TX which should take care of all TX queues including
>> >>      QoS/HTB and PTP.
>> >>    - Rearrange mlx5e_activate_channel and mlx5e_deactivate_channel for
>> >>      better ordering when setting and unsetting NETDEV_QUEUE_TYPE_RX NAPI
>> >>      structs
>> >> v1 -> v2:
>> >>    - Move netlink NULL code to mlx5e_deactivate_channel
>> >>    - Move netif_napi_set_irq to mlx5e_open_channel and avoid storing the
>> >>      irq, after netif_napi_add which itself sets the IRQ to -1
>> >>   drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c  | 3 +++
>> >>   drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 7 +++++++
>> >>   2 files changed, 10 insertions(+)
>> >> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c
>> >> b/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c
>> >> index 078f56a3cbb2..fbbc287d924d 100644
>> >> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c
>> >> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c
>> >> @@ -927,6 +927,8 @@ void mlx5e_ptp_activate_channel(struct mlx5e_ptp *c)
>> >>   	int tc;
>> >>     	napi_enable(&c->napi);
>> >> +	netif_queue_set_napi(c->netdev, c->rq.ix, NETDEV_QUEUE_TYPE_RX,
>> >> +			     &c->napi);
>> 
>> This should only be set if MLX5E_PTP_STATE_RX is set. Otherwise, the rq
>> is not initialized. The following callgraph should help illustrate this.
>> 
>>    mlx5e_ptp_open
>>     |_ mlx5e_ptp_open_queues
>>        |_ mlx5e_ptp_open_rq
>>           |_ mlx5e_open_rq
>
> I had made the change that Tariq had suggested in the previous message of
> using sq->netdev and sq->cq.napi, but I can also tie that into
> MLX5E_PTP_STATE_RX.

Right, Tariq's comments are needed for the TX queue associations and are
needed as well. We also need to make sure we do not create an RX queue
association for an RX queue that may not exist for this special PTP
channel.

>
> I'll add this change as well, test locally again and resend shortly.
>

--
Thanks,

Rahul Rameshbabu