RE: [ndctl PATCH v5 6/6] libndctl,papr_scm: Implement support for PAPR_PDSM_HEALTH

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

"Williams, Dan J" <dan.j.williams@intel.com> writes:

>> -----Original Message-----
>> From: Vaibhav Jain <vaibhav@linux.ibm.com>
>> Sent: Friday, May 29, 2020 3:06 PM
>> To: linux-nvdimm@lists.01.org
>> Cc: Vaibhav Jain <vaibhav@linux.ibm.com>; Williams, Dan J
>> <dan.j.williams@intel.com>; Verma, Vishal L <vishal.l.verma@intel.com>;
>> Aneesh Kumar K . V <aneesh.kumar@linux.ibm.com>; Jeff Moyer
>> <jmoyer@redhat.com>; Oliver O'Halloran <oohall@gmail.com>; Santosh
>> Sivaraj <santosh@fossix.org>; Weiny, Ira <ira.weiny@intel.com>
>> Subject: [ndctl PATCH v5 6/6] libndctl,papr_scm: Implement support for
>> PAPR_PDSM_HEALTH
>> 
>> Add support for reporting DIMM health and shutdown state by issuing
>> PAPR_PDSM_HEALTH request to papr_scm module. It returns an instance of
>> 'struct nd_papr_pdsm_health' as defined in 'papr_pdsm.h'. The patch
>> provides support for dimm-ops 'new_smart', 'smart_get_health' &
>> 'smart_get_shutdown_state' as newly introduced functions
>> papr_new_smart_health(), papr_smart_get_health() &
>> papr_smart_get_shutdown_state() respectively. These callbacks should
>> enable ndctl to report DIMM health.
>> 
>> Also a new member 'struct dimm_priv.health' is introduced which holds the
>> current health status of the dimm. This member is set inside newly added
>> function 'update_dimm_health_v1()' which parses the v1 payload returned
>> by the kernel after servicing PAPR_PDSM_HEALTH. The function will also
>> update dimm-flags viz 'struct ndctl_dimm.flags.f_*'
>> based on the flags set in the returned payload.
>> 
>> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
>> ---
>> Changelog:
>> 
>> v4..v5:
>> * Updated patch description to reflect updated names of struct and
>>   defines that have the term 'scm' removed.
>> 
>> v3..v4:
>> * None
>> 
>> v2..v3:
>> * None
>> 
>> v1..v2:
>> * Squashed patch to report nvdimm bad shutdown state with this patch.
>> * Switched to new structs/enums as defined in papr_scm_pdsm.h
>> ---
>>  ndctl/lib/papr.c | 90
>> ++++++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 87 insertions(+), 3 deletions(-)
>> 
>> diff --git a/ndctl/lib/papr.c b/ndctl/lib/papr.c index
>> 1b7870beb631..cb7ff9e0d5bd 100644
>> --- a/ndctl/lib/papr.c
>> +++ b/ndctl/lib/papr.c
>> @@ -42,7 +42,9 @@
>> 
>>  /* Per dimm data. Holds per-dimm data parsed from the cmd_pkgs */  struct
>> dimm_priv {
>> -	/* Empty for now */
>> +
>> +	/* Cache the dimm health status */
>> +	struct nd_papr_pdsm_health health;
>
[.]
> I don't understand this. The kernel is caching this, why does libndctl
> need to cache it?

Was caching it here as the returned nvdimm health payload from kernel
might be of different version than what was requested and reconstituting
'struct nd_papr_pdsm_health' from 'struct ndctl_cmd' in each dimm-ops
callback would have be costly. 

However with payload versioning scheme not used any more this caching
wont be needed anymore.

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