Hi Vishal, Thanks for reviewing this patch. My responses below:
"Verma, Vishal L" <[email protected]> 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 <[email protected]> >> --- >> 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 -- [email protected] To unsubscribe send an email to [email protected]
