| Message ID | 20170705181250.8D09.E1E9C6FF@jp.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 5, 2017 at 2:12 AM, Yasunori Goto <y-goto@jp.fujitsu.com> wrote: > Hello, > > The current "id" data of dimm (ndctl list -D) shows the information > of DIMM module vendor or serial number. However, I think it is not enough. > > If a NVDIMM becomes broken, users need to know its physical place > rather than vendor or serial number to replace the NVDIMM. > > There are 2 candidate of such information. > a) NFIT Device Handle (_ADR of the NVDIMM device) > This date includes node controller id and socket id. > b) NVDIMM Physical ID (Handle for the SMBIOS Memory Device (Type 17)). > The dmidecode can show this handle with more information like "Locator" > So, user can find the location of NVDIMM. > > At first, I think _ADR is enough information. > > However, when I talked with our firmware team about this requirement, > they said it may not be good data, because the node contoller ID is not > stable on our server due to "partitioning" feature. > (Our server can have plural partition on one box, and may have plural > node 0.) > > So, I make the ndctl shows not only NFIT Device Handle but also > NVDIMM Physical ID. Then, user can find the location with dmidecode. > > This is the first trial patch. So any comments are welcome. This looks ok to me. The only change I would request is to not assume that all NVDIMMs have this NFIT-specific data available. > > Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com> > --- > diff --git a/util/json.c b/util/json.c > index b718d74..749a358 100644 > --- a/util/json.c > +++ b/util/json.c > @@ -68,6 +68,9 @@ struct json_object *util_dimm_to_json(struct ndctl_dimm *dimm) > { > struct json_object *jdimm = json_object_new_object(); > const char *id = ndctl_dimm_get_unique_id(dimm); > + unsigned int handle = ndctl_dimm_get_handle(dimm); > + short phys_id = ndctl_dimm_get_phys_id(dimm); > + char buf[11]; > struct json_object *jobj; > > if (!jdimm) > @@ -85,6 +88,18 @@ struct json_object *util_dimm_to_json(struct ndctl_dimm *dimm) > json_object_object_add(jdimm, "id", jobj); > } > > + snprintf(buf, 11, "0x%08x", handle); > + jobj = json_object_new_string(buf); > + if (!jobj) > + goto err; > + json_object_object_add(jdimm, "handle", jobj); For example, this should be in a "if (handle < UINIT_MAX)" branch... > + > + snprintf(buf, 7, "0x%04x", phys_id ); > + jobj = json_object_new_string(buf); > + if (!jobj) > + goto err; > + json_object_object_add(jdimm, "phys_id", jobj); > + ..., and this should be in a "if (phys < USHORT_MAX)" branch. Lastly, I think these should be json_object_new_int() types since they are numbers not strings.
On 07/05/2017 01:23 PM, Dan Williams wrote: > On Wed, Jul 5, 2017 at 2:12 AM, Yasunori Goto <y-goto@jp.fujitsu.com> wrote: >> Hello, >> >> The current "id" data of dimm (ndctl list -D) shows the information >> of DIMM module vendor or serial number. However, I think it is not enough. >> >> If a NVDIMM becomes broken, users need to know its physical place >> rather than vendor or serial number to replace the NVDIMM. >> >> There are 2 candidate of such information. >> a) NFIT Device Handle (_ADR of the NVDIMM device) >> This date includes node controller id and socket id. >> b) NVDIMM Physical ID (Handle for the SMBIOS Memory Device (Type 17)). >> The dmidecode can show this handle with more information like "Locator" >> So, user can find the location of NVDIMM. >> >> At first, I think _ADR is enough information. >> >> However, when I talked with our firmware team about this requirement, >> they said it may not be good data, because the node contoller ID is not >> stable on our server due to "partitioning" feature. >> (Our server can have plural partition on one box, and may have plural >> node 0.) >> >> So, I make the ndctl shows not only NFIT Device Handle but also >> NVDIMM Physical ID. Then, user can find the location with dmidecode. >> >> This is the first trial patch. So any comments are welcome. > > This looks ok to me. The only change I would request is to not assume > that all NVDIMMs have this NFIT-specific data available. > >> >> Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com> >> --- >> diff --git a/util/json.c b/util/json.c >> index b718d74..749a358 100644 >> --- a/util/json.c >> +++ b/util/json.c >> @@ -68,6 +68,9 @@ struct json_object *util_dimm_to_json(struct ndctl_dimm *dimm) >> { >> struct json_object *jdimm = json_object_new_object(); >> const char *id = ndctl_dimm_get_unique_id(dimm); >> + unsigned int handle = ndctl_dimm_get_handle(dimm); >> + short phys_id = ndctl_dimm_get_phys_id(dimm); Should phys_id be an unsigned short? >> + char buf[11]; >> struct json_object *jobj; >> >> if (!jdimm) >> @@ -85,6 +88,18 @@ struct json_object *util_dimm_to_json(struct ndctl_dimm *dimm) >> json_object_object_add(jdimm, "id", jobj); >> } >> >> + snprintf(buf, 11, "0x%08x", handle); >> + jobj = json_object_new_string(buf); >> + if (!jobj) >> + goto err; >> + json_object_object_add(jdimm, "handle", jobj); > > For example, this should be in a "if (handle < UINIT_MAX)" branch... Is that check actually necessary when handle is an unsigned int? Is there any illegal value? > >> + >> + snprintf(buf, 7, "0x%04x", phys_id ); >> + jobj = json_object_new_string(buf); >> + if (!jobj) >> + goto err; >> + json_object_object_add(jdimm, "phys_id", jobj); >> + > > ..., and this should be in a "if (phys < USHORT_MAX)" branch. Same question here. What's an illegal value? > > Lastly, I think these should be json_object_new_int() types since they > are numbers not strings. Is it possible to get hex values if they're numbers? For the device handle, hex is helpful. -- ljk > _______________________________________________ > Linux-nvdimm mailing list > Linux-nvdimm@lists.01.org > https://lists.01.org/mailman/listinfo/linux-nvdimm >
On Wed, Jul 5, 2017 at 11:08 AM, Linda Knippers <linda.knippers@hpe.com> wrote: > On 07/05/2017 01:23 PM, Dan Williams wrote: >> On Wed, Jul 5, 2017 at 2:12 AM, Yasunori Goto <y-goto@jp.fujitsu.com> wrote: >>> Hello, >>> >>> The current "id" data of dimm (ndctl list -D) shows the information >>> of DIMM module vendor or serial number. However, I think it is not enough. >>> >>> If a NVDIMM becomes broken, users need to know its physical place >>> rather than vendor or serial number to replace the NVDIMM. >>> >>> There are 2 candidate of such information. >>> a) NFIT Device Handle (_ADR of the NVDIMM device) >>> This date includes node controller id and socket id. >>> b) NVDIMM Physical ID (Handle for the SMBIOS Memory Device (Type 17)). >>> The dmidecode can show this handle with more information like "Locator" >>> So, user can find the location of NVDIMM. >>> >>> At first, I think _ADR is enough information. >>> >>> However, when I talked with our firmware team about this requirement, >>> they said it may not be good data, because the node contoller ID is not >>> stable on our server due to "partitioning" feature. >>> (Our server can have plural partition on one box, and may have plural >>> node 0.) >>> >>> So, I make the ndctl shows not only NFIT Device Handle but also >>> NVDIMM Physical ID. Then, user can find the location with dmidecode. >>> >>> This is the first trial patch. So any comments are welcome. >> >> This looks ok to me. The only change I would request is to not assume >> that all NVDIMMs have this NFIT-specific data available. >> >>> >>> Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com> >>> --- >>> diff --git a/util/json.c b/util/json.c >>> index b718d74..749a358 100644 >>> --- a/util/json.c >>> +++ b/util/json.c >>> @@ -68,6 +68,9 @@ struct json_object *util_dimm_to_json(struct ndctl_dimm *dimm) >>> { >>> struct json_object *jdimm = json_object_new_object(); >>> const char *id = ndctl_dimm_get_unique_id(dimm); >>> + unsigned int handle = ndctl_dimm_get_handle(dimm); >>> + short phys_id = ndctl_dimm_get_phys_id(dimm); > > Should phys_id be an unsigned short? Yes, "NVDIMM Physical ID" is defined as a 16-bit field in the NFIT. > >>> + char buf[11]; >>> struct json_object *jobj; >>> >>> if (!jdimm) >>> @@ -85,6 +88,18 @@ struct json_object *util_dimm_to_json(struct ndctl_dimm *dimm) >>> json_object_object_add(jdimm, "id", jobj); >>> } >>> >>> + snprintf(buf, 11, "0x%08x", handle); >>> + jobj = json_object_new_string(buf); >>> + if (!jobj) >>> + goto err; >>> + json_object_object_add(jdimm, "handle", jobj); >> >> For example, this should be in a "if (handle < UINIT_MAX)" branch... > > Is that check actually necessary when handle is an unsigned int? Is there any illegal value? Yes, if the DIMM is not defined by NFIT then we initialize the value to -1 since only ACPI defined DIMMs have handles. I think UINT_MAX is a safe value for an invalid handle. Otherwise, if you parse out UINT_MAX as a valid handle, that indicates the last DIMM on 4096 node system. If such a system is possible we can also use ndctl_bus_has_nfit() to gate whether the NFIT-specific properties of a DIMM are available. >> >>> + >>> + snprintf(buf, 7, "0x%04x", phys_id ); >>> + jobj = json_object_new_string(buf); >>> + if (!jobj) >>> + goto err; >>> + json_object_object_add(jdimm, "phys_id", jobj); >>> + >> >> ..., and this should be in a "if (phys < USHORT_MAX)" branch. > > Same question here. What's an illegal value? I think it is safe to assume that USHORT_MAX is invalid, but we can again use the ndctl_bus_has_nfit() if this might be a problem on some platform. >> Lastly, I think these should be json_object_new_int() types since they >> are numbers not strings. > > Is it possible to get hex values if they're numbers? For the device handle, hex is > helpful. Yes, that would be great. Last I checked the "json-c" library that ndctl is using did not support this, but I don't think anything prevents json-c from growing this support. I.e. as far as I know it supports hex numbers for input, so I think it is valid json either way.
> >>> This is the first trial patch. So any comments are welcome. > >> > >> This looks ok to me. The only change I would request is to not assume > >> that all NVDIMMs have this NFIT-specific data available. > >> > >>> > >>> Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com> > >>> --- > >>> diff --git a/util/json.c b/util/json.c > >>> index b718d74..749a358 100644 > >>> --- a/util/json.c > >>> +++ b/util/json.c > >>> @@ -68,6 +68,9 @@ struct json_object *util_dimm_to_json(struct ndctl_dimm *dimm) > >>> { > >>> struct json_object *jdimm = json_object_new_object(); > >>> const char *id = ndctl_dimm_get_unique_id(dimm); > >>> + unsigned int handle = ndctl_dimm_get_handle(dimm); > >>> + short phys_id = ndctl_dimm_get_phys_id(dimm); > > > > Should phys_id be an unsigned short? > > Yes, "NVDIMM Physical ID" is defined as a 16-bit field in the NFIT. Ah, OK. I'll fix it. > > > > >>> + char buf[11]; > >>> struct json_object *jobj; > >>> > >>> if (!jdimm) > >>> @@ -85,6 +88,18 @@ struct json_object *util_dimm_to_json(struct ndctl_dimm *dimm) > >>> json_object_object_add(jdimm, "id", jobj); > >>> } > >>> > >>> + snprintf(buf, 11, "0x%08x", handle); > >>> + jobj = json_object_new_string(buf); > >>> + if (!jobj) > >>> + goto err; > >>> + json_object_object_add(jdimm, "handle", jobj); > >> > >> For example, this should be in a "if (handle < UINIT_MAX)" branch... > > > > Is that check actually necessary when handle is an unsigned int? Is there any illegal value? > > Yes, if the DIMM is not defined by NFIT then we initialize the value > to -1 since only ACPI defined DIMMs have handles. I think UINT_MAX is > a safe value for an invalid handle. Otherwise, if you parse out > UINT_MAX as a valid handle, that indicates the last DIMM on 4096 node > system. If such a system is possible we can also use > ndctl_bus_has_nfit() to gate whether the NFIT-specific properties of a > DIMM are available. Ok. > >>> + snprintf(buf, 7, "0x%04x", phys_id ); > >>> + jobj = json_object_new_string(buf); > >>> + if (!jobj) > >>> + goto err; > >>> + json_object_object_add(jdimm, "phys_id", jobj); > >>> + > >> > >> ..., and this should be in a "if (phys < USHORT_MAX)" branch. > > > > Same question here. What's an illegal value? > > I think it is safe to assume that USHORT_MAX is invalid, but we can > again use the ndctl_bus_has_nfit() if this might be a problem on some > platform. Ok. > > >> Lastly, I think these should be json_object_new_int() types since they > >> are numbers not strings. > > > > Is it possible to get hex values if they're numbers? For the device handle, hex is > > helpful. > > Yes, that would be great. Last I checked the "json-c" library that > ndctl is using did not support this, but I don't think anything > prevents json-c from growing this support. I.e. as far as I know it > supports hex numbers for input, so I think it is valid json either > way. Sorry, maybe I could not follow here. Please let me confirm. In conclusion, json_object_new_int() should be used at here? Since I tried to output hex values for user to compare with dmidecode's output easily, I translated to hex strings at here. But, I should json_object_new_int() in conclusion, and should expect that json-c library will be able to output hex value in future? Bye,
On Wed, Jul 5, 2017 at 9:13 PM, Yasunori Goto <y-goto@jp.fujitsu.com> wrote: [..] >> Yes, that would be great. Last I checked the "json-c" library that >> ndctl is using did not support this, but I don't think anything >> prevents json-c from growing this support. I.e. as far as I know it >> supports hex numbers for input, so I think it is valid json either >> way. > > Sorry, maybe I could not follow here. Please let me confirm. > In conclusion, json_object_new_int() should be used at here? > Yes. > Since I tried to output hex values for user to compare with dmidecode's output easily, > I translated to hex strings at here. > But, I should json_object_new_int() in conclusion, and should expect that json-c > library will be able to output hex value in future? I checked, it seems hex output is not valid json. However, I think what I will do is add a "--hex" option to "ndctl list" that will make util_display_json_array() manually print the json output object and convert all integers to hex in the process. While that option will make the output not machine readable by standards compliant json parsers it will make it easier for humans to read in some cases.
> On Wed, Jul 5, 2017 at 9:13 PM, Yasunori Goto <y-goto@jp.fujitsu.com> wrote: > [..] > >> Yes, that would be great. Last I checked the "json-c" library that > >> ndctl is using did not support this, but I don't think anything > >> prevents json-c from growing this support. I.e. as far as I know it > >> supports hex numbers for input, so I think it is valid json either > >> way. > > > > Sorry, maybe I could not follow here. Please let me confirm. > > In conclusion, json_object_new_int() should be used at here? > > > > Yes. > > > Since I tried to output hex values for user to compare with dmidecode's output easily, > > I translated to hex strings at here. > > But, I should json_object_new_int() in conclusion, and should expect that json-c > > library will be able to output hex value in future? > > I checked, it seems hex output is not valid json. However, I think > what I will do is add a "--hex" option to "ndctl list" that will make > util_display_json_array() manually print the json output object and > convert all integers to hex in the process. While that option will > make the output not machine readable by standards compliant json > parsers it will make it easier for humans to read in some cases. Ok! I understood! Thank you for your comments. Bye,
On 7/6/2017 1:41 AM, Dan Williams wrote: > On Wed, Jul 5, 2017 at 9:13 PM, Yasunori Goto <y-goto@jp.fujitsu.com> wrote: > [..] >>> Yes, that would be great. Last I checked the "json-c" library that >>> ndctl is using did not support this, but I don't think anything >>> prevents json-c from growing this support. I.e. as far as I know it >>> supports hex numbers for input, so I think it is valid json either >>> way. >> >> Sorry, maybe I could not follow here. Please let me confirm. >> In conclusion, json_object_new_int() should be used at here? >> > > Yes. > >> Since I tried to output hex values for user to compare with dmidecode's output easily, >> I translated to hex strings at here. >> But, I should json_object_new_int() in conclusion, and should expect that json-c >> library will be able to output hex value in future? > > I checked, it seems hex output is not valid json. However, I think > what I will do is add a "--hex" option to "ndctl list" that will make > util_display_json_array() manually print the json output object and > convert all integers to hex in the process. While that option will > make the output not machine readable by standards compliant json > parsers it will make it easier for humans to read in some cases. Will that convert all integers to hex if that option is used? Some values are more useful as decimal and some are more useful as hex. In this case where the value is never really useful as decimal, I thought the original code did pretty well. I'd prefer that to yet another option. -- ljk
On Thu, Jul 6, 2017 at 6:31 AM, Linda Knippers <linda.knippers@hpe.com> wrote: > > > On 7/6/2017 1:41 AM, Dan Williams wrote: >> >> On Wed, Jul 5, 2017 at 9:13 PM, Yasunori Goto <y-goto@jp.fujitsu.com> >> wrote: >> [..] >>>> >>>> Yes, that would be great. Last I checked the "json-c" library that >>>> ndctl is using did not support this, but I don't think anything >>>> prevents json-c from growing this support. I.e. as far as I know it >>>> supports hex numbers for input, so I think it is valid json either >>>> way. >>> >>> >>> Sorry, maybe I could not follow here. Please let me confirm. >>> In conclusion, json_object_new_int() should be used at here? >>> >> >> Yes. >> >>> Since I tried to output hex values for user to compare with dmidecode's >>> output easily, >>> I translated to hex strings at here. >>> But, I should json_object_new_int() in conclusion, and should expect that >>> json-c >>> library will be able to output hex value in future? >> >> >> I checked, it seems hex output is not valid json. However, I think >> what I will do is add a "--hex" option to "ndctl list" that will make >> util_display_json_array() manually print the json output object and >> convert all integers to hex in the process. While that option will >> make the output not machine readable by standards compliant json >> parsers it will make it easier for humans to read in some cases. > > > Will that convert all integers to hex if that option is used? Some values > are more useful as decimal and some are more useful as hex. In this case > where the value is never really useful as decimal, I thought the original > code did pretty well. I'd prefer that to yet another option. True, converting all of the integers to hex is not entirely useful. What would be better I think is a --human option that turns sizes into values with units i.e. "805306368000" -> "750G", but display values like handles and ids as hex. The display conversion can be customized with json_object_set_serializer() on a per-object / per-field basis.
diff --git a/util/json.c b/util/json.c index b718d74..749a358 100644 --- a/util/json.c +++ b/util/json.c @@ -68,6 +68,9 @@ struct json_object *util_dimm_to_json(struct ndctl_dimm *dimm) { struct json_object *jdimm = json_object_new_object(); const char *id = ndctl_dimm_get_unique_id(dimm); + unsigned int handle = ndctl_dimm_get_handle(dimm); + short phys_id = ndctl_dimm_get_phys_id(dimm); + char buf[11]; struct json_object *jobj; if (!jdimm) @@ -85,6 +88,18 @@ struct json_object *util_dimm_to_json(struct ndctl_dimm *dimm) json_object_object_add(jdimm, "id", jobj); } + snprintf(buf, 11, "0x%08x", handle); + jobj = json_object_new_string(buf); + if (!jobj) + goto err; + json_object_object_add(jdimm, "handle", jobj); + + snprintf(buf, 7, "0x%04x", phys_id ); + jobj = json_object_new_string(buf); + if (!jobj) + goto err; + json_object_object_add(jdimm, "phys_id", jobj); + if (!ndctl_dimm_is_enabled(dimm)) { jobj = json_object_new_string("disabled"); if (!jobj)
Hello, The current "id" data of dimm (ndctl list -D) shows the information of DIMM module vendor or serial number. However, I think it is not enough. If a NVDIMM becomes broken, users need to know its physical place rather than vendor or serial number to replace the NVDIMM. There are 2 candidate of such information. a) NFIT Device Handle (_ADR of the NVDIMM device) This date includes node controller id and socket id. b) NVDIMM Physical ID (Handle for the SMBIOS Memory Device (Type 17)). The dmidecode can show this handle with more information like "Locator" So, user can find the location of NVDIMM. At first, I think _ADR is enough information. However, when I talked with our firmware team about this requirement, they said it may not be good data, because the node contoller ID is not stable on our server due to "partitioning" feature. (Our server can have plural partition on one box, and may have plural node 0.) So, I make the ndctl shows not only NFIT Device Handle but also NVDIMM Physical ID. Then, user can find the location with dmidecode. This is the first trial patch. So any comments are welcome. Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com> --- --- Yasunori Goto