netdev - Re: [PATCH net-next v4] ibmvnic: Increase max subcrq indirect entries with fallback

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20250807124744.GJ61519@horms.kernel.org>
Date: Thu, 7 Aug 2025 13:47:44 +0100
From: Simon Horman <horms@...nel.org>
To: Mingming Cao <mmc@...ux.ibm.com>
Cc: netdev@...r.kernel.org, bjking1@...ux.ibm.com, haren@...ux.ibm.com,
	ricklind@...ux.ibm.com, kuba@...nel.org, edumazet@...gle.com,
	pabeni@...hat.com, linuxppc-dev@...ts.ozlabs.org,
	maddy@...ux.ibm.com, mpe@...erman.id.au
Subject: Re: [PATCH net-next v4] ibmvnic: Increase max subcrq indirect
 entries with fallback

On Wed, Aug 06, 2025 at 11:44:49AM -0700, Mingming Cao wrote:
> POWER8 support a maximum of 16 subcrq indirect descriptor entries per
>  H_SEND_SUB_CRQ_INDIRECT call, while POWER9 and newer hypervisors
>  support up to 128 entries. Increasing the max number of indirect
> descriptor entries improves batching efficiency and reduces
> hcall overhead, which enhances throughput under large workload on POWER9+.
> 
> Currently, ibmvnic driver always uses a fixed number of max indirect
> descriptor entries (16). send_subcrq_indirect() treats all hypervisor
> errors the same:
>  - Cleanup and Drop the entire batch of descriptors.
>  - Return an error to the caller.
>  - Rely on TCP/IP retransmissions to recover.
>  - If the hypervisor returns H_PARAMETER (e.g., because 128
>    entries are not supported on POWER8), the driver will continue
>    to drop batches, resulting in unnecessary packet loss.
> 
> In this patch:
> Raise the default maximum indirect entries to 128 to improve ibmvnic
> batching on morden platform. But also gracefully fall back to
> 16 entries for Power 8 systems.
> 
> Since there is no VIO interface to query the hypervisor’s supported
> limit, vnic handles send_subcrq_indirect() H_PARAMETER errors:
>  - On first H_PARAMETER failure, log the failure context
>  - Reduce max_indirect_entries to 16 and allow the single batch to drop.
>  - Subsequent calls automatically use the correct lower limit,
>     avoiding repeated drops.
> 
> The goal is to  optimizes performance on modern systems while handles
> falling back for older POWER8 hypervisors.
> 
> Performance shows 40% improvements with MTU (1500) on largework load.
> 
> --------------------------------------
> Changes since v3:
> Link to v3: https://www.spinics.net/lists/netdev/msg1112828.html
> - consolidate H_PARAMTER handling & subcrq ind desc limit reset for RX/TX
>   into a helper function
> - Cleanup and clarify comments in post migration case
> - Renamed the limits to be a clear and simple name

Thanks for the updates.

I'm sorry for not mentioning this in my review of v3, but net-next
is currently closed for the merge window. Could you please repost,
or post a v4, once it re-opens. That should happen once v6.17-rc1
has been released. Probably early next week (week of 11th August).

My minor nits below notwithstanding this looks good to me.
So feel free to include.

Reviewed-by: Simon Horman <horms@...nel.org>

N.b.: I will be on a break when net-next reopens.
      So please don't wait for feedback from me then.

> 
> Changes since v2:
> link to v2: https://www.spinics.net/lists/netdev/msg1104669.html
> 
> -- was Patch 4 from a patch series v2. v2 introduced a module parameter
> for backward compatibility. Based on review feedback, This patch handles
> older systems fall back case without adding a module parameter.
> 
> Signed-off-by: Mingming Cao <mmc@...ux.ibm.com>
> Reviewed-by: Brian King <bjking1@...ux.ibm.com>
> Reviewed-by: Haren Myneni <haren@...ux.ibm.com>
> ---

These days it is preferable to put the revision history here.
Rather than above your Signed-off-by line, as is currently the case.

>  drivers/net/ethernet/ibm/ibmvnic.c | 59 ++++++++++++++++++++++++++----
>  drivers/net/ethernet/ibm/ibmvnic.h |  6 ++-
>  2 files changed, 56 insertions(+), 9 deletions(-)

Or here.

> 
> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c

...

> @@ -6369,6 +6400,19 @@ static int ibmvnic_reset_init(struct ibmvnic_adapter *adapter, bool reset)
>  			rc = reset_sub_crq_queues(adapter);
>  		}
>  	} else {
> +		if (adapter->reset_reason == VNIC_RESET_MOBILITY) {
> +			/* After an LPM, reset the max number of indirect
> +			 * subcrq descriptors per H_SEND_SUB_CRQ_INDIRECT
> +			 * hcall to the default max (e.g POWER8 -> POWER10)
> +			 *
> +			 * If the new destination platform does not support
> +			 * the higher limit max (e.g. POWER10-> POWER8 LPM)
> +			 * H_PARAMETER will trigger automatic fallback to the
> +			 * safe minimium limit.

minimum

> +			 */
> +			adapter->cur_max_ind_descs = IBMVNIC_MAX_IND_DESCS;
> +		}
> +
>  		rc = init_sub_crqs(adapter);
>  	}

...

> diff --git a/drivers/net/ethernet/ibm/ibmvnic.h b/drivers/net/ethernet/ibm/ibmvnic.h

> index 246ddce753f9..480dc587078f 100644
> --- a/drivers/net/ethernet/ibm/ibmvnic.h
> +++ b/drivers/net/ethernet/ibm/ibmvnic.h
> @@ -29,8 +29,9 @@
>  #define IBMVNIC_BUFFS_PER_POOL	100
>  #define IBMVNIC_MAX_QUEUES	16
>  #define IBMVNIC_MAX_QUEUE_SZ   4096
> -#define IBMVNIC_MAX_IND_DESCS  16
> -#define IBMVNIC_IND_ARR_SZ	(IBMVNIC_MAX_IND_DESCS * 32)
> +#define IBMVNIC_MAX_IND_DESCS 128
> +#define IBMVNIC_SAFE_IND_DESC 16
> +#define IBMVNIC_IND_MAX_ARR_SZ (IBMVNIC_MAX_IND_DESCS * 32)

nit: maybe move towards using tabs before the values here?

+#define IBMVNIC_MAX_IND_DESCS	128
+#define IBMVNIC_SAFE_IND_DESC	16
+#define IBMVNIC_IND_MAX_ARR_SZ	(IBMVNIC_MAX_IND_DESCS * 32)

...

-- 
pw-bot: deferred