Re: [PATCH v3 02/10] powerpc/pseries/vas: Add notifier for DLPAR core removal/add

[Nicholas Piggin <npiggin@gmail.com>]

Excerpts from Haren Myneni's message of January 22, 2022 5:54 am:
> 
> The hypervisor assigns credits for each LPAR based on number of
> cores configured in that system. So expects to release credits
> (means windows) when the core is removed. This patch adds notifier
> for core removal/add so that the OS closes windows if the system
> looses credits due to core removal and reopen windows when the
> credits available later.

This could be improved. As far as I can tell,

 The hypervisor assigns vas credits (windows) for each LPAR based on the 
 number of cores configured in that system. The OS is expected to 
 release credits when cores are removed, and may allocate more when 
 cores are added.

Or can you only re-use credits that you previously lost?

> 
> Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> ---
>  arch/powerpc/platforms/pseries/vas.c | 37 ++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/arch/powerpc/platforms/pseries/vas.c b/arch/powerpc/platforms/pseries/vas.c
> index c0737379cc7b..d2c8292bfb33 100644
> --- a/arch/powerpc/platforms/pseries/vas.c
> +++ b/arch/powerpc/platforms/pseries/vas.c
> @@ -538,6 +538,39 @@ static int __init get_vas_capabilities(u8 feat, enum vas_cop_feat_type type,
>  	return 0;
>  }
>  
> +/*
> + * Total number of default credits available (target_credits)
> + * in LPAR depends on number of cores configured. It varies based on
> + * whether processors are in shared mode or dedicated mode.
> + * Get the notifier when CPU configuration is changed with DLPAR
> + * operation so that get the new target_credits (vas default capabilities)
> + * and then update the existing windows usage if needed.
> + */
> +static int pseries_vas_notifier(struct notifier_block *nb,
> +				unsigned long action, void *data)
> +{
> +	struct of_reconfig_data *rd = data;
> +	struct device_node *dn = rd->dn;
> +	const __be32 *intserv = NULL;
> +	int len, rc = 0;
> +
> +	if ((action == OF_RECONFIG_ATTACH_NODE) ||
> +		(action == OF_RECONFIG_DETACH_NODE))

I suppose the OF notifier is the way to do it (cc Nathan).

Could this patch be folded in with where it acually does something? It 
makes it easier to review and understand how the notifier is used.


> +		intserv = of_get_property(dn, "ibm,ppc-interrupt-server#s",
> +					  &len);
> +	/*
> +	 * Processor config is not changed
> +	 */
> +	if (!intserv)
> +		return NOTIFY_OK;
> +
> +	return rc;
> +}
> +
> +static struct notifier_block pseries_vas_nb = {
> +	.notifier_call = pseries_vas_notifier,
> +};
> +
>  static int __init pseries_vas_init(void)
>  {
>  	struct hv_vas_cop_feat_caps *hv_cop_caps;
> @@ -591,6 +624,10 @@ static int __init pseries_vas_init(void)
>  			goto out_cop;
>  	}
>  
> +	/* Processors can be added/removed only on LPAR */

What does this comment mean? DLPAR?

Thanks,
Nick

> +	if (copypaste_feat && firmware_has_feature(FW_FEATURE_LPAR))
> +		of_reconfig_notifier_register(&pseries_vas_nb);
> +
>  	pr_info("GZIP feature is available\n");
>  
>  out_cop:
> -- 
> 2.27.0
> 
> 
>