| Message ID | PU1P153MB01691E80896E9EAA0C8245EDBF7D0@PU1P153MB0169.APCP153.PROD.OUTLOOK.COM (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add the support for NVDIMM_FAMILY_HYPERV | expand |
> -----Original Message----- > From: Dexuan Cui [mailto:decui@microsoft.com] > Sent: Wednesday, February 20, 2019 2:12 PM > To: Dave Jiang <dave.jiang@intel.com>; Vishal Verma > <vishal.l.verma@intel.com>; Dan Williams <dan.j.williams@intel.com>; > linux-nvdimm@lists.01.org; Michael Kelley <mikelley@microsoft.com>; Qi, > Fuli/斉 福利 <qi.fuli@fujitsu.com>; Johannes Thumshirn <jthumshirn@suse.de> > Subject: [ndctl PATCH v2 4/4] ndctl, monitor: support NVDIMM_FAMILY_HYPERV > > > Currently "ndctl monitor" fails for NVDIMM_FAMILY_HYPERV due to > "no smart support". > > NVDIMM_FAMILY_HYPERV doesn't use ND_CMD_SMART to get the health info. > Instead, it uses ND_CMD_CALL, so the checking here can't apply,and it > doesn't support threshold alarms -- actually it only supports one event: > ND_EVENT_HEALTH_STATE. > > See http://www.uefi.org/RFIC_LIST ("Virtual NVDIMM 0x1901"). > > Let's skip the unnecessary checking for NVDIMM_FAMILY_HYPERV, and make > sure we only monitor the "dimm-health-state" event and ignore the others. > > With the patch, when an error happens, we log it with such a message: > > {"timestamp":"1550547497.431731497","pid":1571,"event": > {"dimm-health-state":true},"dimm":{"dev":"nmem1", > "id":"04d5-01-1701-01000000","handle":1,"phys_id":0, > "health":{"health_state":"fatal","shutdown_count":8}}} > > Here the meaningful info is: > "health":{"health_state":"fatal","shutdown_count":8} > > Signed-off-by: Dexuan Cui <decui@microsoft.com> > --- > ndctl/monitor.c | 42 +++++++++++++++++++++++++++++++++++------- > 1 file changed, 35 insertions(+), 7 deletions(-) > > diff --git a/ndctl/monitor.c b/ndctl/monitor.c > index 43b2abe..43beb06 100644 > --- a/ndctl/monitor.c > +++ b/ndctl/monitor.c > @@ -265,31 +265,59 @@ static bool filter_region(struct ndctl_region *region, > return true; > } > > -static void filter_dimm(struct ndctl_dimm *dimm, struct util_filter_ctx > *fctx) > +static bool ndctl_dimm_test_and_enable_notification(struct ndctl_dimm > *dimm) > { > - struct monitor_dimm *mdimm; > - struct monitor_filter_arg *mfa = fctx->monitor; > const char *name = ndctl_dimm_get_devname(dimm); > > + /* > + * Hyper-V Virtual NVDIMM doesn't use ND_CMD_SMART to get the health > + * info. Instead, it uses ND_CMD_CALL, so the checking here can't > + * apply, and it doesn't support threshold alarms -- actually it only > + * supports one event: ND_EVENT_HEALTH_STATE. > + */ > + if (ndctl_dimm_get_cmd_family(dimm) == NVDIMM_FAMILY_HYPERV) { > + if (monitor.event_flags != ND_EVENT_HEALTH_STATE) { > + monitor.event_flags = ND_EVENT_HEALTH_STATE; > + Hi Dexuan, I think that "monitor" should be a "read-only" command and can't force the users to change their options. The monitor.event_flags will work for all NVDIMMs to be monitored. I don't know whether a physical NVDIMM could be mounted on the OS running inside of a virtual machine. If yes, this forced changing may cause an exception of monitoring smart threshold events on NVDIMM. Thanks, Qi > + notice(&monitor, > + "%s: only dimm-health-state can be > monitored\n", > + name); > + } > + return true; > + } > + > if (!ndctl_dimm_is_cmd_supported(dimm, ND_CMD_SMART)) { > err(&monitor, "%s: no smart support\n", name); > - return; > + return false; > } > if (!ndctl_dimm_is_cmd_supported(dimm, ND_CMD_SMART_THRESHOLD)) { > err(&monitor, "%s: no smart threshold support\n", name); > - return; > + return false; > } > > if (!ndctl_dimm_is_flag_supported(dimm, ND_SMART_ALARM_VALID)) { > err(&monitor, "%s: smart alarm invalid\n", name); > - return; > + return false; > } > > if (enable_dimm_supported_threshold_alarms(dimm)) { > err(&monitor, "%s: enable supported threshold alarms > failed\n", name); > - return; > + return false; > } > > + return true; > +} > + > +static void filter_dimm(struct ndctl_dimm *dimm, struct util_filter_ctx > *fctx) > +{ > + struct monitor_dimm *mdimm; > + struct monitor_filter_arg *mfa = fctx->monitor; > + const char *name = ndctl_dimm_get_devname(dimm); > + > + > + if (!ndctl_dimm_test_and_enable_notification(dimm)) > + return; > + > mdimm = calloc(1, sizeof(struct monitor_dimm)); > if (!mdimm) { > err(&monitor, "%s: calloc for monitor dimm failed\n", name); > -- > 2.19.1
> From: qi.fuli@fujitsu.com <qi.fuli@fujitsu.com> > Sent: Thursday, February 21, 2019 12:40 AM > > ... > > + /* > > + * Hyper-V Virtual NVDIMM doesn't use ND_CMD_SMART to get the > > health info. Instead, it uses ND_CMD_CALL, so the checking here can't > > + * apply, and it doesn't support threshold alarms -- actually it only > > + * supports one event: ND_EVENT_HEALTH_STATE. > > + */ > > + if (ndctl_dimm_get_cmd_family(dimm) == NVDIMM_FAMILY_HYPERV) { > > + if (monitor.event_flags != ND_EVENT_HEALTH_STATE) { > > + monitor.event_flags = ND_EVENT_HEALTH_STATE; > > + > > + notice(&monitor, "%s: only dimm-health-state can be > > monitored\n", name); > Hi Dexuan, > I think that "monitor" should be a "read-only" command and can't force the > users to change their options. > The monitor.event_flags will work for all NVDIMMs to be monitored. > I don't know whether a physical NVDIMM could be mounted on the OS > running inside of a virtual machine. > If yes, this forced changing may cause an exception of monitoring smart > threshold events on NVDIMM. > > Thanks, > Qi Hi Qi, Generally speaking, I agree with you, but here the fact is that we can only monitor this one single event "dimm-health-state" for Hyper-V Virtual NVDIMM, and the other events are meaningless for Hyper-V Virtual NVDIMM, as they're not supported by Hyper-V Virtual NVDIMM. So, if the user specifies more events than just "dimm-health-state", or the user doesn't specify "dimm-health-state", it's reasonable to force monitor.event_flags to equal to ND_EVENT_HEALTH_STATE, and I explicitly print a warning. The line "monitor.event_flags = ND_EVENT_HEALTH_STATE" only runs in a Linux VM running on Hyper-V. It can't adversely affect bare metal cases. In a Linux VM on Hyper-V, all the NVDIMMs appearing in the VM are Hyper-V Virtual NVDIMMs. A physical NVDIMM can't be directly passed through to a VM and the hypervisors (e.g., Xen, KVM, Hyper-V, Qemu, etc.) have to virtualize it to make it available to a VM. In a VM, the NVDIMM_FAMILY is NVDIMM_FAMILY_HYPERV only when the VM runs on Hyper-V. So I think we should be good. :-) Thanks, -- Dexuan
> > Hi Qi, > Generally speaking, I agree with you, but here the fact is that we can only monitor this > one single event "dimm-health-state" for Hyper-V Virtual NVDIMM, and the other > events are meaningless for Hyper-V Virtual NVDIMM, as they're not supported by > Hyper-V Virtual NVDIMM. > > So, if the user specifies more events than just "dimm-health-state", or the user doesn't > specify "dimm-health-state", it's reasonable to force monitor.event_flags to equal to > ND_EVENT_HEALTH_STATE, and I explicitly print a warning. > > The line "monitor.event_flags = ND_EVENT_HEALTH_STATE" only runs in a Linux VM > running on Hyper-V. It can't adversely affect bare metal cases. In a Linux VM on > Hyper-V, all the NVDIMMs appearing in the VM are Hyper-V Virtual NVDIMMs. > > A physical NVDIMM can't be directly passed through to a VM and the hypervisors (e.g., > Xen, KVM, Hyper-V, Qemu, etc.) have to virtualize it to make it available to a VM. In a > VM, the NVDIMM_FAMILY is NVDIMM_FAMILY_HYPERV only when the VM runs on > Hyper-V. > > So I think we should be good. :-) > > Thanks, > -- Dexuan Hi Dexuan, Thanks for your explanation. I think at least we should document it into man page, so that the users could know that their options may be modified by monitor. Though I have a little concern about that there may be other events for Hyper-V Virtual NVDIMM can be monitored in future... Thanks Qi
> From: qi.fuli@fujitsu.com <qi.fuli@fujitsu.com> > Sent: Sunday, February 24, 2019 8:48 PM > To: Dexuan Cui <decui@microsoft.com>; Dave Jiang <dave.jiang@intel.com>; > Vishal Verma <vishal.l.verma@intel.com>; Dan Williams > <dan.j.williams@intel.com>; linux-nvdimm@lists.01.org; Michael Kelley > <mikelley@microsoft.com>; Johannes Thumshirn <jthumshirn@suse.de> > Subject: RE: [ndctl PATCH v2 4/4] ndctl, monitor: support > NVDIMM_FAMILY_HYPERV > > > > > Hi Qi, > > Generally speaking, I agree with you, but here the fact is that we can only > monitor this > > one single event "dimm-health-state" for Hyper-V Virtual NVDIMM, and the > other > > events are meaningless for Hyper-V Virtual NVDIMM, as they're not > supported by > > Hyper-V Virtual NVDIMM. > > > > So, if the user specifies more events than just "dimm-health-state", or the > user doesn't > > specify "dimm-health-state", it's reasonable to force monitor.event_flags to > equal to > > ND_EVENT_HEALTH_STATE, and I explicitly print a warning. > > > > The line "monitor.event_flags = ND_EVENT_HEALTH_STATE" only runs in a > Linux VM > > running on Hyper-V. It can't adversely affect bare metal cases. In a Linux VM > on > > Hyper-V, all the NVDIMMs appearing in the VM are Hyper-V Virtual > NVDIMMs. > > > > A physical NVDIMM can't be directly passed through to a VM and the > hypervisors (e.g., > > Xen, KVM, Hyper-V, Qemu, etc.) have to virtualize it to make it available to a > VM. In a > > VM, the NVDIMM_FAMILY is NVDIMM_FAMILY_HYPERV only when the VM > runs on > > Hyper-V. > > > > So I think we should be good. :-) > > > > Thanks, > > -- Dexuan > > Hi Dexuan, > > Thanks for your explanation. > I think at least we should document it into man page, so that the users could > know that their options may be modified by monitor. Good suggestion! After the patch receives more comments and/or after it's accepted, I'll add a new patch for the man page: Documentation/ndctl/ndctl-monitor.txt. > Though I have a little concern about that there may be other events for > Hyper-V Virtual NVDIMM can be monitored in future... > Qi As far as I know, that's unlikely, at least in the foreseeable future. :-) The VM is supposed to see a virtual NVDIMM, for which the hypervisor only emulates the minimal necessary info, based on the physical NVDIMM's state. Thanks, --Dexuan
On Wed, 2019-02-20 at 05:11 +0000, Dexuan Cui wrote: > Currently "ndctl monitor" fails for NVDIMM_FAMILY_HYPERV due to > "no smart support". > > NVDIMM_FAMILY_HYPERV doesn't use ND_CMD_SMART to get the health info. > Instead, it uses ND_CMD_CALL, so the checking here can't apply,and it > doesn't support threshold alarms -- actually it only supports one event: > ND_EVENT_HEALTH_STATE. > > See http://www.uefi.org/RFIC_LIST ("Virtual NVDIMM 0x1901"). > > Let's skip the unnecessary checking for NVDIMM_FAMILY_HYPERV, and make > sure we only monitor the "dimm-health-state" event and ignore the others. > > With the patch, when an error happens, we log it with such a message: > > {"timestamp":"1550547497.431731497","pid":1571,"event": > {"dimm-health-state":true},"dimm":{"dev":"nmem1", > "id":"04d5-01-1701-01000000","handle":1,"phys_id":0, > "health":{"health_state":"fatal","shutdown_count":8}}} > > Here the meaningful info is: > "health":{"health_state":"fatal","shutdown_count":8} > > Signed-off-by: Dexuan Cui <decui@microsoft.com> > --- > ndctl/monitor.c | 42 +++++++++++++++++++++++++++++++++++------- > 1 file changed, 35 insertions(+), 7 deletions(-) > > diff --git a/ndctl/monitor.c b/ndctl/monitor.c > index 43b2abe..43beb06 100644 > --- a/ndctl/monitor.c > +++ b/ndctl/monitor.c > @@ -265,31 +265,59 @@ static bool filter_region(struct ndctl_region *region, > return true; > } > > -static void filter_dimm(struct ndctl_dimm *dimm, struct util_filter_ctx *fctx) > +static bool ndctl_dimm_test_and_enable_notification(struct ndctl_dimm *dimm) > { > - struct monitor_dimm *mdimm; > - struct monitor_filter_arg *mfa = fctx->monitor; > const char *name = ndctl_dimm_get_devname(dimm); > > + /* > + * Hyper-V Virtual NVDIMM doesn't use ND_CMD_SMART to get the health > + * info. Instead, it uses ND_CMD_CALL, so the checking here can't > + * apply, and it doesn't support threshold alarms -- actually it only > + * supports one event: ND_EVENT_HEALTH_STATE. > + */ > + if (ndctl_dimm_get_cmd_family(dimm) == NVDIMM_FAMILY_HYPERV) { > + if (monitor.event_flags != ND_EVENT_HEALTH_STATE) { > + monitor.event_flags = ND_EVENT_HEALTH_STATE; > + > + notice(&monitor, > + "%s: only dimm-health-state can be monitored\n", > + name); > + } > + return true; > + } > + I'm not sure about the family-specific special casing -- it leaks family specific details into the common implementation, something that's been avoidable thus far. Is there any reason why the ndctl_dimm_is_cmd_supported() can't be made to fail appropriately for anything that is not supported, by adding the necessary functions to dimm-ops? That way if the user enters any of the unsupported options, they will just fail normally, and the user will be expected to provide the right options for the environment they know they're running in. Indeed, I think that patch 3 of this series should never be required :) > if (!ndctl_dimm_is_cmd_supported(dimm, ND_CMD_SMART)) { > err(&monitor, "%s: no smart support\n", name); > - return; > + return false; > } > if (!ndctl_dimm_is_cmd_supported(dimm, ND_CMD_SMART_THRESHOLD)) { > err(&monitor, "%s: no smart threshold support\n", name); > - return; > + return false; > } > > if (!ndctl_dimm_is_flag_supported(dimm, ND_SMART_ALARM_VALID)) { > err(&monitor, "%s: smart alarm invalid\n", name); > - return; > + return false; > } > > if (enable_dimm_supported_threshold_alarms(dimm)) { > err(&monitor, "%s: enable supported threshold alarms failed\n", name); > - return; > + return false; > } > > + return true; > +} > + > +static void filter_dimm(struct ndctl_dimm *dimm, struct util_filter_ctx *fctx) > +{ > + struct monitor_dimm *mdimm; > + struct monitor_filter_arg *mfa = fctx->monitor; > + const char *name = ndctl_dimm_get_devname(dimm); > + > + > + if (!ndctl_dimm_test_and_enable_notification(dimm)) > + return; > + > mdimm = calloc(1, sizeof(struct monitor_dimm)); > if (!mdimm) { > err(&monitor, "%s: calloc for monitor dimm failed\n", name);
> From: Verma, Vishal L <vishal.l.verma@intel.com> > Sent: Wednesday, March 20, 2019 6:55 PM > ... > > > > -static void filter_dimm(struct ndctl_dimm *dimm, struct util_filter_ctx *fctx) > > +static bool ndctl_dimm_test_and_enable_notification(struct ndctl_dimm > *dimm) > > { > > - struct monitor_dimm *mdimm; > > - struct monitor_filter_arg *mfa = fctx->monitor; > > const char *name = ndctl_dimm_get_devname(dimm); > > > > + /* > > + * Hyper-V Virtual NVDIMM doesn't use ND_CMD_SMART to get the > health > > + * info. Instead, it uses ND_CMD_CALL, so the checking here can't > > + * apply, and it doesn't support threshold alarms -- actually it only > > + * supports one event: ND_EVENT_HEALTH_STATE. > > + */ > > + if (ndctl_dimm_get_cmd_family(dimm) == NVDIMM_FAMILY_HYPERV) { > > + if (monitor.event_flags != ND_EVENT_HEALTH_STATE) { > > + monitor.event_flags = ND_EVENT_HEALTH_STATE; > > + > > + notice(&monitor, > > + "%s: only dimm-health-state can be monitored\n", > > + name); > > + } > > + return true; > > + } > > + > > I'm not sure about the family-specific special casing -- it leaks family > specific details into the common implementation, something that's been > avoidable thus far. > > Is there any reason why the ndctl_dimm_is_cmd_supported() can't be made > to fail appropriately for anything that is not supported, by adding the > necessary functions to dimm-ops? IMO there are 2 issues in ndctl/monitor.c: filter_dimm(): 1. IMO the cmd numbers ND_CMD_SMART (1) and ND_CMD_SMART_THRESHOLD(2) are not really device-neutral. They work for ndctl/lib/intel.c and it looks they happen to work for ndctl/lib/hpe1.c, as hpe "happens" to have: NDN_HPE1_CMD_SMART = 1, NDN_HPE1_CMD_SMART_THRESHOLD = 2, But for ndctl/lib/msft.c, 1 and 2 mean different things. See [1]. So the current "ndctl monitor" can't support msft.c. For ndctl/lib/hyperv.c, 1 and 2 means: ND_HYPERV_CMD_GET_HEALTH_INFO = 1, ND_HYPERV_CMD_GET_SHUTDOWN_INFO = 2, They're also incompatible with ND_CMD_SMART and ND_CMD_SMART_THRESHOLD. Of source, the current code can "work" since these ND_HYPERV_CMD* numbers happen to be the same as ND_CMD_SMART and ND_CMD_SMART_THRESHOLD. So this may not be an isue for hyperv.c. 2. The current code assumes ND_SMART_ALARM_VALID must be supported. However, NVDIMM_FAMILY_HYPERV doesn't support it, so currently ndctl/monitor.c: filter_dimm() reports an error and returns -- for Hyper-V, we should not return... hence I made this patch. Of source, we can add a new op to dimm->ops, e.g. ops->monitor_supported() and then change the common code accordingly, but I can imagine that would require a lot more changes and I guess it may not worth that effort? Please share your insights! > That way if the user enters any of the unsupported options, they will > just fail normally, and the user will be expected to provide the right > options for the environment they know they're running in. When the user enters any of the unsupported options, they will only get an "no dimms to monitor" error, which gives no hint why the error happens, and confuses the user... > Indeed, I think that patch 3 of this series should never be required :) Please see the above. Thanks, -- Dexuan [1] https://docs.microsoft.com/en-us/windows-hardware/drivers/storage/-dsm-interface-for-byte-addressable-energy-backed-function-class--function-interface-1-
On Thu, Mar 21, 2019 at 7:09 PM Dexuan Cui <decui@microsoft.com> wrote: [..] > > That way if the user enters any of the unsupported options, they will > > just fail normally, and the user will be expected to provide the right > > options for the environment they know they're running in. > > When the user enters any of the unsupported options, they will only > get an "no dimms to monitor" error, which gives no hint why the > error happens, and confuses the user... For the monitor it should be ok to specify options and thresholds that the DIMM does not support. The events simply won't fire. Now if the user cares to know which events are supported / live we can add "list" enumeration interface for that, but I otherwise think monitor should just silently handle unsupported monitor options.
> From: Dexuan Cui > Sent: Thursday, March 21, 2019 7:09 PM > IMO there are 2 issues in ndctl/monitor.c: filter_dimm(): > > 1. IMO the cmd numbers ND_CMD_SMART (1) and > ND_CMD_SMART_THRESHOLD(2) are not really device-neutral. They > work for ndctl/lib/intel.c and it looks they happen to work for > ndctl/lib/hpe1.c, as hpe "happens" to have: > NDN_HPE1_CMD_SMART = 1, > NDN_HPE1_CMD_SMART_THRESHOLD = 2, > > But for ndctl/lib/msft.c, 1 and 2 mean different things. See [1]. > So the current "ndctl monitor" can't support msft.c. > > For ndctl/lib/hyperv.c, 1 and 2 means: > ND_HYPERV_CMD_GET_HEALTH_INFO = 1, > ND_HYPERV_CMD_GET_SHUTDOWN_INFO = 2, > They're also incompatible with ND_CMD_SMART and > ND_CMD_SMART_THRESHOLD. > Of source, the current code can "work" since these ND_HYPERV_CMD* > numbers happen to be the same as ND_CMD_SMART and > ND_CMD_SMART_THRESHOLD. So this may not be an isue for hyperv.c. Actually, this _is_ an issue for NVDIMM_FAMILY_HYPERV (and the other families except for NVDIMM_FAMILY_INTEL) : see the kernel function acpi_nfit_register_dimms(), where ND_CMD_SMART is set in the "cmd_mask" only for NVDIMM_FAMILY_INTEL. So, on Hyper-V, ndctl_dimm_is_cmd_supported(dimm, ND_CMD_SMART) is always false, and "ndctl monitor" exits with "no smart support". > 2. The current code assumes ND_SMART_ALARM_VALID must be supported. > However, NVDIMM_FAMILY_HYPERV doesn't support it, so currently > ndctl/monitor.c: filter_dimm() reports an error and returns -- for Hyper-V, > we should not return... hence I made this patch. > > Of source, we can add a new op to dimm->ops, e.g. > ops->monitor_supported() and then change the common code accordingly, > but I can imagine that would require a lot more changes and I guess it may not > worth that effort? Please share your insights! Looking forward to your suggestion! Thanks, -- Dexuan
On Thu, Mar 21, 2019 at 9:06 PM Dexuan Cui <decui@microsoft.com> wrote: > > > From: Dexuan Cui > > Sent: Thursday, March 21, 2019 7:09 PM > > > IMO there are 2 issues in ndctl/monitor.c: filter_dimm(): > > > > 1. IMO the cmd numbers ND_CMD_SMART (1) and > > ND_CMD_SMART_THRESHOLD(2) are not really device-neutral. They > > work for ndctl/lib/intel.c and it looks they happen to work for > > ndctl/lib/hpe1.c, as hpe "happens" to have: > > NDN_HPE1_CMD_SMART = 1, > > NDN_HPE1_CMD_SMART_THRESHOLD = 2, > > > > But for ndctl/lib/msft.c, 1 and 2 mean different things. See [1]. > > So the current "ndctl monitor" can't support msft.c. > > > > For ndctl/lib/hyperv.c, 1 and 2 means: > > ND_HYPERV_CMD_GET_HEALTH_INFO = 1, > > ND_HYPERV_CMD_GET_SHUTDOWN_INFO = 2, > > They're also incompatible with ND_CMD_SMART and > > ND_CMD_SMART_THRESHOLD. > > Of source, the current code can "work" since these ND_HYPERV_CMD* > > numbers happen to be the same as ND_CMD_SMART and > > ND_CMD_SMART_THRESHOLD. So this may not be an isue for hyperv.c. > > Actually, this _is_ an issue for NVDIMM_FAMILY_HYPERV (and the other > families except for NVDIMM_FAMILY_INTEL) : see the kernel function > acpi_nfit_register_dimms(), where ND_CMD_SMART is set in the > "cmd_mask" only for NVDIMM_FAMILY_INTEL. > > So, on Hyper-V, ndctl_dimm_is_cmd_supported(dimm, ND_CMD_SMART) > is always false, and "ndctl monitor" exits with "no smart support". Can the Hyper-V implementation emulate those commands? That's the expectation, i.e. that the implementation can return they required payloads, but it's fine if the payloads disclaim support for certain fields.
> From: Dan Williams <dan.j.williams@intel.com> > Sent: Thursday, March 21, 2019 9:13 PM > > ... > > Actually, this _is_ an issue for NVDIMM_FAMILY_HYPERV (and the other > > families except for NVDIMM_FAMILY_INTEL) : see the kernel function > > acpi_nfit_register_dimms(), where ND_CMD_SMART is set in the > > "cmd_mask" only for NVDIMM_FAMILY_INTEL. > > > > So, on Hyper-V, ndctl_dimm_is_cmd_supported(dimm, ND_CMD_SMART) > > is always false, and "ndctl monitor" exits with "no smart support". > > Can the Hyper-V implementation emulate those commands? That's the > expectation, i.e. that the implementation can return they required > payloads, but it's fine if the payloads disclaim support for certain > fields. Hyper-V Virtual NVDIMM doesn't emulate ND_CMD_SMART(1). The _DSM Function 1 on Hyper-V is "Get Health Information" [1], which is incomptabile with NVDIMM_FAMILY_INTEL's ND_CMD_SMART(1). Even if Hyper-V could emulate ND_CMD_SMART, the current "ndctl monitor" code still wouldn't work for Hyper-V (and the other families): the essence of the issue is that: 1. the kernel only sets ND_CMD_SMART flag for NVDIMM_FAMILY_INTEL. 2. "ndctl monitor" assumes the ND_CMD_SMART should be set by checking /sys/class/nd/ndctl0/device/nmem0/commands. This means ndctl_dimm_is_cmd_supported(dimm, ND_CMD_SMART) can only be true on NVDIMM_FAMILY_INTEL. My patch skips the assumption of ndctl on Hyper-V, so "ndctl monitor" can work on Hyper-V. It looks we do need an ops->monitor_supported() in ndctl? Thanks, -- Dexuan [1] See http://www.uefi.org/RFIC_LIST ("Virtual NVDIMM 0x1901").
On Thu, Mar 21, 2019 at 10:09 PM Dexuan Cui <decui@microsoft.com> wrote: > > > From: Dan Williams <dan.j.williams@intel.com> > > Sent: Thursday, March 21, 2019 9:13 PM > > > ... > > > Actually, this _is_ an issue for NVDIMM_FAMILY_HYPERV (and the other > > > families except for NVDIMM_FAMILY_INTEL) : see the kernel function > > > acpi_nfit_register_dimms(), where ND_CMD_SMART is set in the > > > "cmd_mask" only for NVDIMM_FAMILY_INTEL. > > > > > > So, on Hyper-V, ndctl_dimm_is_cmd_supported(dimm, ND_CMD_SMART) > > > is always false, and "ndctl monitor" exits with "no smart support". > > > > Can the Hyper-V implementation emulate those commands? That's the > > expectation, i.e. that the implementation can return they required > > payloads, but it's fine if the payloads disclaim support for certain > > fields. > > Hyper-V Virtual NVDIMM doesn't emulate ND_CMD_SMART(1). > The _DSM Function 1 on Hyper-V is "Get Health Information" [1], which is > incomptabile with NVDIMM_FAMILY_INTEL's ND_CMD_SMART(1). > > Even if Hyper-V could emulate ND_CMD_SMART, the current "ndctl monitor" > code still wouldn't work for Hyper-V (and the other families): the essence of the > issue is that: > 1. the kernel only sets ND_CMD_SMART flag for NVDIMM_FAMILY_INTEL. > 2. "ndctl monitor" assumes the ND_CMD_SMART should be set by checking > /sys/class/nd/ndctl0/device/nmem0/commands. > > This means ndctl_dimm_is_cmd_supported(dimm, ND_CMD_SMART) can > only be true on NVDIMM_FAMILY_INTEL. > > My patch skips the assumption of ndctl on Hyper-V, so "ndctl monitor" > can work on Hyper-V. > > It looks we do need an ops->monitor_supported() in ndctl? No, I think you misunderstand. Hyper-V implements "function-1", "command-1" support can be emulated. The request is to translate the Hyper-V function-1 payload into the command-1 payload format.
> From: Dan Williams <dan.j.williams@intel.com> > Sent: Thursday, March 21, 2019 10:37 PM > > No, I think you misunderstand. Hyper-V implements "function-1", > "command-1" support can be emulated. The request is to translate the > Hyper-V function-1 payload into the command-1 payload format. Then, yes, I think so. The first 2 patches of this patchset do the translation: [ndctl,v2,1/4] libndctl: add support for NVDIMM_FAMILY_HYPERV's _DSM Function 1 [ndctl,v2,2/4] libndctl: NVDIMM_FAMILY_HYPERV: add .smart_get_shutdown_count (Function 2) (https://patchwork.kernel.org/project/linux-nvdimm/list/?series=82629) The problem is how to skip the checkings in ndctl/monitor.c: filter_dimm() on Hyper-V. If we return early in filter_dimm(), mfa->num_dimm will be zero, then monitor_event() can't be called, and we have no chance to monitor the events and do the translation. Thanks, -- Dexuan
On Thu, Mar 21, 2019 at 11:03 PM Dexuan Cui <decui@microsoft.com> wrote: > > > From: Dan Williams <dan.j.williams@intel.com> > > Sent: Thursday, March 21, 2019 10:37 PM > > > > No, I think you misunderstand. Hyper-V implements "function-1", > > "command-1" support can be emulated. The request is to translate the > > Hyper-V function-1 payload into the command-1 payload format. > > Then, yes, I think so. The first 2 patches of this patchset do the translation: > > [ndctl,v2,1/4] libndctl: add support for NVDIMM_FAMILY_HYPERV's _DSM Function 1 > [ndctl,v2,2/4] libndctl: NVDIMM_FAMILY_HYPERV: add .smart_get_shutdown_count (Function 2) > (https://patchwork.kernel.org/project/linux-nvdimm/list/?series=82629) > > The problem is how to skip the checkings in ndctl/monitor.c: filter_dimm() on Hyper-V. > If we return early in filter_dimm(), mfa->num_dimm will be zero, then monitor_event() > can't be called, and we have no chance to monitor the events and do the translation. I'd rather change ndctl_dimm_cmd_is_supported() to call back into a dimm-op so that the Hyper-V implementation can say "yes, I support (emulate) the necessary monitor commands".
> From: Dan Williams <dan.j.williams@intel.com> > Sent: Thursday, March 21, 2019 11:12 PM > To: Dexuan Cui <decui@microsoft.com> > > On Thu, Mar 21, 2019 at 11:03 PM Dexuan Cui <decui@microsoft.com> wrote: > > > > > From: Dan Williams <dan.j.williams@intel.com> > > > Sent: Thursday, March 21, 2019 10:37 PM > > > > > > No, I think you misunderstand. Hyper-V implements "function-1", > > > "command-1" support can be emulated. The request is to translate the > > > Hyper-V function-1 payload into the command-1 payload format. > > > > Then, yes, I think so. The first 2 patches of this patchset do the translation: > > > > [ndctl,v2,1/4] libndctl: add support for NVDIMM_FAMILY_HYPERV's _DSM > Function 1 > > [ndctl,v2,2/4] libndctl: NVDIMM_FAMILY_HYPERV: > add .smart_get_shutdown_count (Function 2) > > > > The problem is how to skip the checkings in ndctl/monitor.c: filter_dimm() on > Hyper-V. > > If we return early in filter_dimm(), mfa->num_dimm will be zero, then > monitor_event() > > can't be called, and we have no chance to monitor the events and do the > translation. > > I'd rather change ndctl_dimm_cmd_is_supported() to call back into a > dimm-op so that the Hyper-V implementation can say "yes, I support > (emulate) the necessary monitor commands". Hi Dan, Then we need to add a new dimm-op monitor_supported(), and change ndctl/lib/intel.c and ndctl/lib/hyper.c to implement the new dimm-op, and we need to change the common code, i.e. ndctl_dimm_cmd_is_supported(), to use this new dimm-op. If this sounds reasonable to you and Verma, I'll try to make a patchset for you to review. Thanks, -- Dexuan
On Fri, Mar 22, 2019 at 10:56 AM Dexuan Cui <decui@microsoft.com> wrote: > > > From: Dan Williams <dan.j.williams@intel.com> > > Sent: Thursday, March 21, 2019 11:12 PM > > To: Dexuan Cui <decui@microsoft.com> > > > > On Thu, Mar 21, 2019 at 11:03 PM Dexuan Cui <decui@microsoft.com> wrote: > > > > > > > From: Dan Williams <dan.j.williams@intel.com> > > > > Sent: Thursday, March 21, 2019 10:37 PM > > > > > > > > No, I think you misunderstand. Hyper-V implements "function-1", > > > > "command-1" support can be emulated. The request is to translate the > > > > Hyper-V function-1 payload into the command-1 payload format. > > > > > > Then, yes, I think so. The first 2 patches of this patchset do the translation: > > > > > > [ndctl,v2,1/4] libndctl: add support for NVDIMM_FAMILY_HYPERV's _DSM > > Function 1 > > > [ndctl,v2,2/4] libndctl: NVDIMM_FAMILY_HYPERV: > > add .smart_get_shutdown_count (Function 2) > > > > > > The problem is how to skip the checkings in ndctl/monitor.c: filter_dimm() on > > Hyper-V. > > > If we return early in filter_dimm(), mfa->num_dimm will be zero, then > > monitor_event() > > > can't be called, and we have no chance to monitor the events and do the > > translation. > > > > I'd rather change ndctl_dimm_cmd_is_supported() to call back into a > > dimm-op so that the Hyper-V implementation can say "yes, I support > > (emulate) the necessary monitor commands". > > Hi Dan, > Then we need to add a new dimm-op monitor_supported() Where would the monitor_supported() op be used? I would expect a cmd_is_supported() op? > and change > ndctl/lib/intel.c and ndctl/lib/hyper.c to implement the new dimm-op, and we > need to change the common code, i.e. ndctl_dimm_cmd_is_supported(), to use > this new dimm-op. This sounds good. > If this sounds reasonable to you and Verma, I'll try to make a patchset for you > to review. Yes, just the question of what you meant the monitor_supported() op to perform.
> From: Dan Williams <dan.j.williams@intel.com> > Sent: Friday, March 22, 2019 11:04 AM > > On Fri, Mar 22, 2019 at 10:56 AM Dexuan Cui <decui@microsoft.com> wrote: > > > > > I'd rather change ndctl_dimm_cmd_is_supported() to call back into a > > > dimm-op so that the Hyper-V implementation can say "yes, I support > > > (emulate) the necessary monitor commands". > > > > Hi Dan, > > Then we need to add a new dimm-op monitor_supported() > > Where would the monitor_supported() op be used? I would expect a > cmd_is_supported() op? Maybe cmd_is_supported() is better. I'll work on the details and keeo you updated. Thanks -- Dexuan
On Fri, 2019-03-22 at 17:55 +0000, Dexuan Cui wrote: > > From: Dan Williams <dan.j.williams@intel.com> > > Sent: Thursday, March 21, 2019 11:12 PM > > To: Dexuan Cui <decui@microsoft.com> > > > > On Thu, Mar 21, 2019 at 11:03 PM Dexuan Cui <decui@microsoft.com> wrote: > > > > From: Dan Williams <dan.j.williams@intel.com> > > > > Sent: Thursday, March 21, 2019 10:37 PM > > > > > > > > No, I think you misunderstand. Hyper-V implements "function-1", > > > > "command-1" support can be emulated. The request is to translate the > > > > Hyper-V function-1 payload into the command-1 payload format. > > > > > > Then, yes, I think so. The first 2 patches of this patchset do the translation: > > > > > > [ndctl,v2,1/4] libndctl: add support for NVDIMM_FAMILY_HYPERV's _DSM > > Function 1 > > > [ndctl,v2,2/4] libndctl: NVDIMM_FAMILY_HYPERV: > > add .smart_get_shutdown_count (Function 2) > > > The problem is how to skip the checkings in ndctl/monitor.c: filter_dimm() on > > Hyper-V. > > > If we return early in filter_dimm(), mfa->num_dimm will be zero, then > > monitor_event() > > > can't be called, and we have no chance to monitor the events and do the > > translation. > > > > I'd rather change ndctl_dimm_cmd_is_supported() to call back into a > > dimm-op so that the Hyper-V implementation can say "yes, I support > > (emulate) the necessary monitor commands". > > Hi Dan, > Then we need to add a new dimm-op monitor_supported(), and change > ndctl/lib/intel.c and ndctl/lib/hyper.c to implement the new dimm-op, and we > need to change the common code, i.e. ndctl_dimm_cmd_is_supported(), to use > this new dimm-op. > > If this sounds reasonable to you and Verma, I'll try to make a patchset for you > to review. Yep this approach sounds good to me! > > Thanks, > -- Dexuan
> From: Verma, Vishal L <vishal.l.verma@intel.com> > Sent: Friday, March 22, 2019 11:29 AM > > On Fri, 2019-03-22 at 17:55 +0000, Dexuan Cui wrote: > > > From: Dan Williams <dan.j.williams@intel.com> > > > Sent: Thursday, March 21, 2019 11:12 PM > > > To: Dexuan Cui <decui@microsoft.com> > > > > > > On Thu, Mar 21, 2019 at 11:03 PM Dexuan Cui <decui@microsoft.com> > wrote: > > > > > From: Dan Williams <dan.j.williams@intel.com> > > > > > Sent: Thursday, March 21, 2019 10:37 PM > > > > > > > > > > No, I think you misunderstand. Hyper-V implements "function-1", > > > > > "command-1" support can be emulated. The request is to translate the > > > > > Hyper-V function-1 payload into the command-1 payload format. > > > > > > > > Then, yes, I think so. The first 2 patches of this patchset do the > translation: > > > > > > > > [ndctl,v2,1/4] libndctl: add support for NVDIMM_FAMILY_HYPERV's > _DSM > > > Function 1 > > > > [ndctl,v2,2/4] libndctl: NVDIMM_FAMILY_HYPERV: > > > add .smart_get_shutdown_count (Function 2) > > > > The problem is how to skip the checkings in ndctl/monitor.c: filter_dimm() > on > > > Hyper-V. > > > > If we return early in filter_dimm(), mfa->num_dimm will be zero, then > > > monitor_event() > > > > can't be called, and we have no chance to monitor the events and do the > > > translation. > > > > > > I'd rather change ndctl_dimm_cmd_is_supported() to call back into a > > > dimm-op so that the Hyper-V implementation can say "yes, I support > > > (emulate) the necessary monitor commands". > > > > Hi Dan, > > Then we need to add a new dimm-op monitor_supported(), and change > > ndctl/lib/intel.c and ndctl/lib/hyper.c to implement the new dimm-op, and > we > > need to change the common code, i.e. ndctl_dimm_cmd_is_supported(), to > use > > this new dimm-op. > > > > If this sounds reasonable to you and Verma, I'll try to make a patchset for you > > to review. > > Yep this approach sounds good to me! Hi all, I sent out the v3 patchset from my gmail account just now. (There is still an SMTP server issue when I use git send-email with my corporate SMTP server...) Thanks, -- Dexuan
diff --git a/ndctl/monitor.c b/ndctl/monitor.c index 43b2abe..43beb06 100644 --- a/ndctl/monitor.c +++ b/ndctl/monitor.c @@ -265,31 +265,59 @@ static bool filter_region(struct ndctl_region *region, return true; } -static void filter_dimm(struct ndctl_dimm *dimm, struct util_filter_ctx *fctx) +static bool ndctl_dimm_test_and_enable_notification(struct ndctl_dimm *dimm) { - struct monitor_dimm *mdimm; - struct monitor_filter_arg *mfa = fctx->monitor; const char *name = ndctl_dimm_get_devname(dimm); + /* + * Hyper-V Virtual NVDIMM doesn't use ND_CMD_SMART to get the health + * info. Instead, it uses ND_CMD_CALL, so the checking here can't + * apply, and it doesn't support threshold alarms -- actually it only + * supports one event: ND_EVENT_HEALTH_STATE. + */ + if (ndctl_dimm_get_cmd_family(dimm) == NVDIMM_FAMILY_HYPERV) { + if (monitor.event_flags != ND_EVENT_HEALTH_STATE) { + monitor.event_flags = ND_EVENT_HEALTH_STATE; + + notice(&monitor, + "%s: only dimm-health-state can be monitored\n", + name); + } + return true; + } + if (!ndctl_dimm_is_cmd_supported(dimm, ND_CMD_SMART)) { err(&monitor, "%s: no smart support\n", name); - return; + return false; } if (!ndctl_dimm_is_cmd_supported(dimm, ND_CMD_SMART_THRESHOLD)) { err(&monitor, "%s: no smart threshold support\n", name); - return; + return false; } if (!ndctl_dimm_is_flag_supported(dimm, ND_SMART_ALARM_VALID)) { err(&monitor, "%s: smart alarm invalid\n", name); - return; + return false; } if (enable_dimm_supported_threshold_alarms(dimm)) { err(&monitor, "%s: enable supported threshold alarms failed\n", name); - return; + return false; } + return true; +} + +static void filter_dimm(struct ndctl_dimm *dimm, struct util_filter_ctx *fctx) +{ + struct monitor_dimm *mdimm; + struct monitor_filter_arg *mfa = fctx->monitor; + const char *name = ndctl_dimm_get_devname(dimm); + + + if (!ndctl_dimm_test_and_enable_notification(dimm)) + return; + mdimm = calloc(1, sizeof(struct monitor_dimm)); if (!mdimm) { err(&monitor, "%s: calloc for monitor dimm failed\n", name);
Currently "ndctl monitor" fails for NVDIMM_FAMILY_HYPERV due to "no smart support". NVDIMM_FAMILY_HYPERV doesn't use ND_CMD_SMART to get the health info. Instead, it uses ND_CMD_CALL, so the checking here can't apply,and it doesn't support threshold alarms -- actually it only supports one event: ND_EVENT_HEALTH_STATE. See http://www.uefi.org/RFIC_LIST ("Virtual NVDIMM 0x1901"). Let's skip the unnecessary checking for NVDIMM_FAMILY_HYPERV, and make sure we only monitor the "dimm-health-state" event and ignore the others. With the patch, when an error happens, we log it with such a message: {"timestamp":"1550547497.431731497","pid":1571,"event": {"dimm-health-state":true},"dimm":{"dev":"nmem1", "id":"04d5-01-1701-01000000","handle":1,"phys_id":0, "health":{"health_state":"fatal","shutdown_count":8}}} Here the meaningful info is: "health":{"health_state":"fatal","shutdown_count":8} Signed-off-by: Dexuan Cui <decui@microsoft.com> --- ndctl/monitor.c | 42 +++++++++++++++++++++++++++++++++++------- 1 file changed, 35 insertions(+), 7 deletions(-)