| Message ID | 20200529220600.225320-4-vaibhav@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for reporting papr nvdimm health | expand |
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'. 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? 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. > > 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;
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;
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;
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'. 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(+)