Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 25 May 2023 10:21:55 +0200
From:   Simon Horman <simon.horman@corigine.com>
To:     Shradha Gupta <shradhagupta@linux.microsoft.com>
Cc:     linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
        netdev@vger.kernel.org, Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        "K. Y. Srinivasan" <kys@microsoft.com>,
        Haiyang Zhang <haiyangz@microsoft.com>,
        Wei Liu <wei.liu@kernel.org>, Dexuan Cui <decui@microsoft.com>,
        Long Li <longli@microsoft.com>,
        Michael Kelley <mikelley@microsoft.com>,
        "David S. Miller" <davem@davemloft.net>,
        Steen Hegelund <steen.hegelund@microchip.com>
Subject: Re: [PATCH v2] hv_netvsc: Allocate rx indirection table size
 dynamically
Message-ID: <ZG8ao4SnUCyZO6jH@corigine.com>
References: <1684922230-24073-1-git-send-email-shradhagupta@linux.microsoft.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1684922230-24073-1-git-send-email-shradhagupta@linux.microsoft.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?tE1CNSsRRHuKnczWxu/KQnBg0BusoCPyq8+8Esh2UjUAzVkk594A0SRq/xEM?=
 =?us-ascii?Q?u22l2aXcclMN6JPqkPQfEJOv8GMqUfcXE9SnLuH5bpLLPInVlm9PefBo9/u2?=
 =?us-ascii?Q?2HVBjdmUSQsIXFSOnw0uZllNvkC9JlpkRpG4Lr7wQD7vRJHnNgBNxVU/Z5zF?=
 =?us-ascii?Q?tSybSKX9hi5I9MSR9A11j2qsr90mzy1jNJ3yhNU0JK3nhkSbjcacxHxobXqg?=
 =?us-ascii?Q?e9AXS4Fb9LCBS1QwXKF7CHI6tuQc1KT3Sf/9BFcr9fp5YKOA+qnZsUN8nwX3?=
 =?us-ascii?Q?gTT+MCfdYsrr2cmScwuqayrQlTUTRsDyxdjSI2GlopaBhVpkzqVqsfxgGIaN?=
 =?us-ascii?Q?oiOcOj277Wl8eDoEoZZ34IFXfGBD7/W6kZ0Uslj3jYUgH5TaZLxNYg4OIEsV?=
 =?us-ascii?Q?gSdBF5dwxRquk6aZA1r3lZDPdgUDtk4PWwyWcmN4e80foCtfnVQJLSJ5bXZs?=
 =?us-ascii?Q?kNU+0Xw7sbiwQMjOXRcVopP86EFq/hoGNr6QQjMmZ748F1MmdeNtdOqD/fS+?=
 =?us-ascii?Q?SqXK4qboumNRw4uW32nupMjIut1Q51BAZab9uN5b+T1JJ1Hpb/o7PViearT/?=
 =?us-ascii?Q?7VuaFhQ5ZWx8WATx3NWCQ/ZsqnLGZBkVW9N4VVtjGS3fmBkT3X1ol/PDWCaw?=
 =?us-ascii?Q?KOIYraDnml85PiVtESx3Y2NjqD6BQxMMHPvUcUdp9wMODeUk7ghr7EvuoVkw?=
 =?us-ascii?Q?wicFlkVAMT2FVZEGtrkigyrDF+YJKLwu33SRrUDoNVX6J9i+kzNtKP6mgiE3?=
 =?us-ascii?Q?NRfMQRgRMe0PdLoulBiz6lygw+xUqwiOw7a24OvJhuDm5T4aUEiS9BTmnYYI?=
 =?us-ascii?Q?2RoDJEpMsj4kL+rFFYEmPZl+CMkvHxJaaILpF0vLkOVf9w7cHEgP116xZbPJ?=
 =?us-ascii?Q?TCVdjJlbGIue8S5DsIUz0E9/6FvGsdZRj7M9l7WB7ZlddQNcSOBQlNgppTqx?=
 =?us-ascii?Q?BVdcgFJ+rliNMfejggr4tyoYL8IkwwNFZrJgy/syIuo7DlaCXQBxVd/OB2WP?=
 =?us-ascii?Q?+ykIn7ViJ8LhSZedvXz8KjijcEtH/H3LSpPHNYe0vJcRCShkmL/AXyXdP0ln?=
 =?us-ascii?Q?J9B+wtP9tkR0cI0L/9h2NhQlibyJC7SGvG+8UlR/iP/NDxI7wTnNrz9bWI9G?=
 =?us-ascii?Q?ZpAQV0BLop2yuLM2Zjh+7mrGdBSJkMY3VCggX57n8SIfd79SsFo06zmnp8yM?=
 =?us-ascii?Q?emD2+aXwh2sG8oSrnh+FRdwn4OWAzaXZqwt2PHNUNCzyZzc8Aw7K6hKOYz9q?=
 =?us-ascii?Q?Fv1rFUExbtUUW2QViNuZHgHZDSolGgmbQt2fPEfUuLCn/aUfrhHZUqc1fk1p?=
 =?us-ascii?Q?Kz5+GNVsEIAPZp/CNqvqdNUBEFoPIe0ZrZokcF81Jhq9sBJlNNJea3yComzb?=
 =?us-ascii?Q?yfOYHC3EAdhNNt48dwZ3NMWQHadnmh9DVXpb+7uUivJ063ujAbvfAuaC7c9X?=
 =?us-ascii?Q?2SYz92KS+f2j5TmkuyVud4Dxz/FC97aw+/0X9uzFvLtO/BK3kmKBlHJjuLjr?=
 =?us-ascii?Q?3sYSolPU89yr0J7MhmOraW5YHqtLytomgxjHNPuff3ftgK84nulNzvy2tWK4?=
 =?us-ascii?Q?GzWnAJ0IedjopDUQrV7Ehm24O+LSC9Rg7McUkxvmT1ndRIlzQZIyAcrhd5yq?=
 =?us-ascii?Q?HIa68JGJkmlzDDXzs5/zog/ixMljAX/jXjB5iInENEBFM1nF95a89Qxmkzdj?=
 =?us-ascii?Q?TVO/Ww=3D=3D?=
X-OriginatorOrg: corigine.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ed85cd04-50e3-4cb5-8e3f-08db5cf91f9c
X-MS-Exchange-CrossTenant-AuthSource: PH0PR13MB4842.namprd13.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 May 2023 08:22:05.1487
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: fe128f2c-073b-4c20-818e-7246a585940c
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: kKgoiYr4W5Q+Ub0Bw0KTB07vvIH8TspRWhQvHX//hy0CUEXxbvYcUjyy97a6k4nIb3FI+H3G3O1VZMHi6eJztG/OqlvPFUCrqr4W1d+Zl70=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY3PR13MB4931
X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, May 24, 2023 at 02:57:10AM -0700, Shradha Gupta wrote:
> Allocate the size of rx indirection table dynamically in netvsc
> from the value of size provided by OID_GEN_RECEIVE_SCALE_CAPABILITIES
> query instead of using a constant value of ITAB_NUM.
> 
> Signed-off-by: Shradha Gupta <shradhagupta@linux.microsoft.com>

Hi Shradha,

thanks for your patch.

Some nits from my side.
And some friendly advice: please consider using checkpatch

...

> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index 0103ff914024..ab791e4ca63c 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -1040,6 +1040,13 @@ static int netvsc_detach(struct net_device *ndev,
>  
>  	rndis_filter_device_remove(hdev, nvdev);
>  
> +	/* 
> +	 * Free the rx indirection table and reset the table size to 0.
> +	 * With the netvsc_attach call it will get allocated again.
> +	 */

nit: In Networking code multi-line comments look like this:

	/* Free the rx indirection table and reset the table size to 0.
	 * With the netvsc_attach call it will get allocated again.
	 */

> +	ndev_ctx->rx_table_sz = 0;
> +	kfree(ndev_ctx->rx_table);
> +
>  	return 0;
>  }
>  

...

> @@ -1548,6 +1548,20 @@ struct netvsc_device *rndis_filter_device_add(struct hv_device *dev,
>  	if (ret || rsscap.num_recv_que < 2)
>  		goto out;
>  
> +	if (rsscap.num_indirect_tabent &&
> +	    rsscap.num_indirect_tabent <= ITAB_NUM_MAX)
> +		ndc->rx_table_sz = rsscap.num_indirect_tabent;
> +	else
> +		ndc->rx_table_sz = ITAB_NUM;
> +
> +	ndc->rx_table = kzalloc(sizeof(u16) * ndc->rx_table_sz,
> +				GFP_KERNEL);

nit: kcalloc seems appropriate here.

> +	if (!ndc->rx_table) {
> +		netdev_err(net, "Error in allocating rx indirection table of size %d\n",
> +				ndc->rx_table_sz);

nit: No need to log memory allocation errors, the mm core does that.

     Also, the alignment of the line above should match of the opening '('
     of the preceding line.

	f(a,
	  b);

> +		goto out;
> +	}
> +
>  	/* This guarantees that num_possible_rss_qs <= num_online_cpus */
>  	num_possible_rss_qs = min_t(u32, num_online_cpus(),