Re: [ndctl PATCH v5 3/6] libndctl: Introduce new dimm-ops dimm_init() & dimm_uninit()

[Vaibhav Jain <vaibhav@linux.ibm.com>]

Hi Vishal,

Thanks for reviewing this patch. My responses below:

"Verma, Vishal L" <vishal.l.verma@intel.com> writes:

> On Sat, 2020-05-30 at 03:35 +0530, Vaibhav Jain wrote:
>> There are scenarios when a dimm-provider need to allocate some
>> per-dimm data that can be quickly retrieved. This data can be used to
>> cache data that spans multiple 'struct ndctl_cmd' submissions.
>> 
>> Unfortunately currently in libnvdimm there is no easy way to implement
>> this. Even if this data is some how stored in some field of 'struct
>> ndctl_dimm', managing its lifetime is a challenge.
>> 
>> To solve this problem, the patch proposes a new member 'struct
>> ndctl_dimm.dimm_user_data' to store per-dimm data interpretation of
>> which is specific to a dimm-provider. Also two new dimm-ops namely
>> dimm_init() & dimm_uninit() are introduced that can be used to manage
>> the lifetime of this per-dimm data.
>> 
>> Semantics
>> =========
>> int (*dimm_init)(struct ndctl_dimm *):
>> 
>> This callback will be called just after dimm-probe inside add_dimm()
>> is completed. Dimm-providers should use this callback to allocate
>> per-dimm data and assign it to 'struct ndctl_dimm.dimm_user_data'
>> member. In case this function returns an error, dimm initialization is
>> halted and errors out.
>> 
>> void (*dimm_uninit)(struct ndctl_dimm *):
>> 
>> This callback will be called during free_dimm() and is only called if
>> previous call to 'dimm_ops->dimm_init()' had reported no
>> error. Dimm-providers should use this callback to unallocate and
>> cleanup 'dimm_user_data'.
<snip>
>
> I'm not sure I fully understand the need for this whole paradigm - of
> creating a private caching area in ndctl_dimm, and having these
> init/uninit functions to set it up.
>
> Looking ahead at subsequent patches, the data you're stashing there is
> already cached in the kernel or the command payloads. It seems the
> caching is maybe just avoiding some ioctl round trips - is that
> correct?
Yes, that was the real motivation behind introducing these new
dimm-ops. The primary problem was with fetching the
'life_used_percentage' which in case of papr_scm would have required a
separate ioctl apart from one to fetch nvdimm-health.

With per-dimm data to hold the dimm-health and 'life_used_percentage',
once the ndctl_cmd for fetching nvdimm health is complete I would issue
another ndctl_cmd to fetch the 'life_used_percentage' store these value
in per-dimm data and when 'smart_get_life_used' is called would return
cached value.

>
> If so , why not just make all the data retrieval synchronous to the
> operation that's requesting it? Health retrieval is generally not a fast
> path of any sort, and doing everything synchronously seems much simpler,
> and is also what existing nvdimm families do.
I can probably issue the ndctl_cmd to fetch 'life_used_percentage'
synchronously in 'smart_get_life_used' but introducing per-dimm data
seemed a cleaner approach as it may have wider usefulness.

>
>> 
>> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
>> ---
>> Changelog:
>> 
>> v4..v5:
>> * None
>> 
>> v3..v4:
>> * None
>> 
>> v2..v3:
>> * None
>> 
>> v1..v2:
>> * Changed the patch order
>> ---
>>  ndctl/lib/libndctl.c | 11 +++++++++++
>>  ndctl/lib/private.h  |  5 +++++
>>  2 files changed, 16 insertions(+)
>> 
>> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
>> index a52c2e208fbe..8d226abf7495 100644
>> --- a/ndctl/lib/libndctl.c
>> +++ b/ndctl/lib/libndctl.c
>> @@ -598,6 +598,11 @@ static void free_dimm(struct ndctl_dimm *dimm)
>>  {
>>  	if (!dimm)
>>  		return;
>> +
>> +	/* If needed call the dimm uninitialization function */
>> +	if (dimm->ops && dimm->ops->dimm_uninit)
>> +		dimm->ops->dimm_uninit(dimm);
>> +
>>  	free(dimm->unique_id);
>>  	free(dimm->dimm_buf);
>>  	free(dimm->dimm_path);
>> @@ -1714,8 +1719,14 @@ static void *add_dimm(void *parent, int id, const char *dimm_base)
>>  	if (dimm->cmd_family == NVDIMM_FAMILY_PAPR)
>>  		dimm->ops = papr_dimm_ops;
>>  
>> +	/* Call the dimm initialization function if needed */
>> +	if (!rc && dimm->ops && dimm->ops->dimm_init)
>> +		rc = dimm->ops->dimm_init(dimm);
>> +
>>   out:
>>  	if (rc) {
>> +		/* Ensure dimm_uninit() is not called during free_dimm() */
>> +		dimm->ops = NULL;
>>  		err(ctx, "Unable to probe dimm:%d. Err:%d\n", id, rc);
>>  		goto err_read;
>>  	}
>> diff --git a/ndctl/lib/private.h b/ndctl/lib/private.h
>> index d90236b1f98b..d0188a97d673 100644
>> --- a/ndctl/lib/private.h
>> +++ b/ndctl/lib/private.h
>> @@ -99,6 +99,7 @@ struct ndctl_dimm {
>>  	} flags;
>>  	int locked;
>>  	int aliased;
>> +	void *dimm_user_data;
>>  	struct list_node list;
>>  	int formats;
>>  	int format[0];
>> @@ -347,6 +348,10 @@ struct ndctl_dimm_ops {
>>  	int (*fw_update_supported)(struct ndctl_dimm *);
>>  	int (*xlat_firmware_status)(struct ndctl_cmd *);
>>  	u32 (*get_firmware_status)(struct ndctl_cmd *);
>> +	/* Called just after dimm is initialized and probed */
>> +	int (*dimm_init)(struct ndctl_dimm *);
>> +	/* Called just before struct ndctl_dimm is de-allocated */
>> +	void (*dimm_uninit)(struct ndctl_dimm *);
>>  };
>>  
>>  extern struct ndctl_dimm_ops * const intel_dimm_ops;

-- 
Cheers
~ Vaibhav
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org