| Message ID | 1a0d146dffb17449aa6d8a6b6d06e865e69226de.1525709213.git.swise@opengridcomputing.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On 5/7/18 9:53 AM, Steve Wise wrote: > @@ -152,7 +153,10 @@ int main(int argc, char **argv) > pretty_output = true; > break; > case 'd': > - show_details = true; > + if (show_details) > + show_driver_details = true; > + else > + show_details = true; > break; > case 'j': > json_output = true; The above change should be reflected in the man page. Also, the set needs to be respun after I merged master where Stephen brought in updates to the uapi files. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5/9/2018 11:08 PM, David Ahern wrote: > On 5/7/18 9:53 AM, Steve Wise wrote: >> @@ -152,7 +153,10 @@ int main(int argc, char **argv) >> pretty_output = true; >> break; >> case 'd': >> - show_details = true; >> + if (show_details) >> + show_driver_details = true; >> + else >> + show_details = true; >> break; >> case 'j': >> json_output = true; > The above change should be reflected in the man page. I did mention it in the man page: -d, --details Output detailed information. Adding a second -d includes driver-specific details. But I wasn't sure how to show it in the syntax. Maybe this? OPTIONS := { -V[ersion] | -d[etails] [-d[etails]] } -j[son] } -p[retty] } > Also, the set needs to be respun after I merged master where Stephen > brought in updates to the uapi files. Will do. Thanks for reviewing. Steve. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5/10/18 8:19 AM, Steve Wise wrote: > > On 5/9/2018 11:08 PM, David Ahern wrote: >> On 5/7/18 9:53 AM, Steve Wise wrote: >>> @@ -152,7 +153,10 @@ int main(int argc, char **argv) >>> pretty_output = true; >>> break; >>> case 'd': >>> - show_details = true; >>> + if (show_details) >>> + show_driver_details = true; >>> + else >>> + show_details = true; >>> break; >>> case 'j': >>> json_output = true; >> The above change should be reflected in the man page. > > I did mention it in the man page: > > -d, --details > Output detailed information. Adding a second -d includes > driver-specific details. > > But I wasn't sure how to show it in the syntax. Maybe this? > > OPTIONS := { -V[ersion] | -d[etails] [-d[etails]] } -j[son] } -p[retty] } I should have read the second patch before commenting. Didn't it have first -d = details, a second -d = driver details? That should be fine. > > >> Also, the set needs to be respun after I merged master where Stephen >> brought in updates to the uapi files. > > Will do. Thanks for reviewing. > > Steve. > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 10, 2018 at 08:20:51AM -0600, David Ahern wrote: > On 5/10/18 8:19 AM, Steve Wise wrote: > > > > On 5/9/2018 11:08 PM, David Ahern wrote: > >> On 5/7/18 9:53 AM, Steve Wise wrote: > >>> @@ -152,7 +153,10 @@ int main(int argc, char **argv) > >>> pretty_output = true; > >>> break; > >>> case 'd': > >>> - show_details = true; > >>> + if (show_details) > >>> + show_driver_details = true; > >>> + else > >>> + show_details = true; > >>> break; > >>> case 'j': > >>> json_output = true; > >> The above change should be reflected in the man page. > > > > I did mention it in the man page: > > > > -d, --details > > Output detailed information. Adding a second -d includes > > driver-specific details. > > > > But I wasn't sure how to show it in the syntax. Maybe this? > > > > OPTIONS := { -V[ersion] | -d[etails] [-d[etails]] } -j[son] } -p[retty] } > > I should have read the second patch before commenting. Didn't it have > first -d = details, a second -d = driver details? That should be fine. Yes, our idea is to require "-dd" to print such driver specific information. The level of nesting is: * No arguments -> info usable for most of the users * -d - pre-parsed flags and rarely used information. * -dd - very detailed output, can be very specific to device. Thanks > > > > > > >> Also, the set needs to be respun after I merged master where Stephen > >> brought in updates to the uapi files. > > > > Will do. Thanks for reviewing. > > > > Steve. > > >
On Mon, May 07, 2018 at 08:53:16AM -0700, Steve Wise wrote: > This enhancement allows printing rdma device-specific state, if provided > by the kernel. This is done in a generic manner, so rdma tool doesn't Double space between "." and "This". > need to know about the details of every type of rdma device. > > Driver attributes for a rdma resource are in the form of <key, > [print_type], value> tuples, where the key is a string and the value can > be any supported driver attribute. The print_type attribute, if present, ditto > provides a print format to use vs the standard print format for the type. > For example, the default print type for a PROVIDER_S32 value is "%d ", > but "0x%x " if the print_type of PRINT_TYPE_HEX is included inthe tuple. > > Driver resources are only printed when the -dd flag is present. > If -p is present, then the output is formatted to not exceed 80 columns, > otherwise it is printed as a single row to be grep/awk friendly. > > Example output: > > # rdma resource show qp lqpn 1028 -dd -p > link cxgb4_0/- lqpn 1028 rqpn 0 type RC state RTS rq-psn 0 sq-psn 0 path-mig-state MIGRATED pid 0 comm [nvme_rdma] > sqid 1028 flushed 0 memsize 123968 cidx 85 pidx 85 wq_pidx 106 flush_cidx 85 in_use 0 > size 386 flags 0x0 rqid 1029 memsize 16768 cidx 43 pidx 41 wq_pidx 171 msn 44 rqt_hwaddr 0x2a8a5d00 > rqt_size 256 in_use 128 size 130 idx 43 wr_id 0xffff881057c03408 idx 40 wr_id 0xffff881057c033f0 > > Signed-off-by: Steve Wise <swise@opengridcomputing.com> > --- > rdma/rdma.c | 7 ++- > rdma/rdma.h | 11 ++++ > rdma/res.c | 30 +++------ > rdma/utils.c | 194 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 221 insertions(+), 21 deletions(-) > > diff --git a/rdma/rdma.c b/rdma/rdma.c > index b43e538..0155627 100644 > --- a/rdma/rdma.c > +++ b/rdma/rdma.c > @@ -132,6 +132,7 @@ int main(int argc, char **argv) > const char *batch_file = NULL; > bool pretty_output = false; > bool show_details = false; > + bool show_driver_details = false; Reversed Christmas tree please. > bool json_output = false; > bool force = false; > char *filename; > @@ -152,7 +153,10 @@ int main(int argc, char **argv) > pretty_output = true; > break; > case 'd': > - show_details = true; > + if (show_details) > + show_driver_details = true; > + else > + show_details = true; > break; > case 'j': > json_output = true; > @@ -180,6 +184,7 @@ int main(int argc, char **argv) > argv += optind; > > rd.show_details = show_details; > + rd.show_driver_details = show_driver_details; > rd.json_output = json_output; > rd.pretty_output = pretty_output; > > diff --git a/rdma/rdma.h b/rdma/rdma.h > index 1908fc4..fcaf9e6 100644 > --- a/rdma/rdma.h > +++ b/rdma/rdma.h > @@ -55,6 +55,7 @@ struct rd { > char **argv; > char *filename; > bool show_details; > + bool show_driver_details; > struct list_head dev_map_list; > uint32_t dev_idx; > uint32_t port_idx; > @@ -115,4 +116,14 @@ int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void *data, uint32_t seq); > void rd_prepare_msg(struct rd *rd, uint32_t cmd, uint32_t *seq, uint16_t flags); > int rd_dev_init_cb(const struct nlmsghdr *nlh, void *data); > int rd_attr_cb(const struct nlattr *attr, void *data); > +int rd_attr_check(const struct nlattr *attr, int *typep); > + > +/* > + * Print helpers > + */ > +void print_driver_table(struct rd *rd, struct nlattr *tb); > +void newline(struct rd *rd); > +void newline_indent(struct rd *rd); > +#define MAX_LINE_LENGTH 80 > + > #endif /* _RDMA_TOOL_H_ */ > diff --git a/rdma/res.c b/rdma/res.c > index 1a0aab6..074b992 100644 > --- a/rdma/res.c > +++ b/rdma/res.c > @@ -439,10 +439,8 @@ static int res_qp_parse_cb(const struct nlmsghdr *nlh, void *data) > if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) > free(comm); > > - if (rd->json_output) > - jsonw_end_array(rd->jw); > - else > - pr_out("\n"); > + print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]); > + newline(rd); > } > return MNL_CB_OK; > } > @@ -678,10 +676,8 @@ static int res_cm_id_parse_cb(const struct nlmsghdr *nlh, void *data) > if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) > free(comm); > > - if (rd->json_output) > - jsonw_end_array(rd->jw); > - else > - pr_out("\n"); > + print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]); > + newline(rd); > } > return MNL_CB_OK; > } > @@ -804,10 +800,8 @@ static int res_cq_parse_cb(const struct nlmsghdr *nlh, void *data) > if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) > free(comm); > > - if (rd->json_output) > - jsonw_end_array(rd->jw); > - else > - pr_out("\n"); > + print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]); > + newline(rd); > } > return MNL_CB_OK; > } > @@ -919,10 +913,8 @@ static int res_mr_parse_cb(const struct nlmsghdr *nlh, void *data) > if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) > free(comm); > > - if (rd->json_output) > - jsonw_end_array(rd->jw); > - else > - pr_out("\n"); > + print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]); > + newline(rd); > } > return MNL_CB_OK; > } > @@ -1004,10 +996,8 @@ static int res_pd_parse_cb(const struct nlmsghdr *nlh, void *data) > if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) > free(comm); > > - if (rd->json_output) > - jsonw_end_array(rd->jw); > - else > - pr_out("\n"); > + print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]); > + newline(rd); > } > return MNL_CB_OK; > } > diff --git a/rdma/utils.c b/rdma/utils.c > index 49c967f..452fe92 100644 > --- a/rdma/utils.c > +++ b/rdma/utils.c > @@ -11,6 +11,7 @@ > > #include "rdma.h" > #include <ctype.h> > +#include <inttypes.h> > > int rd_argc(struct rd *rd) > { > @@ -393,8 +394,32 @@ static const enum mnl_attr_data_type nldev_policy[RDMA_NLDEV_ATTR_MAX] = { > [RDMA_NLDEV_ATTR_RES_MRLEN] = MNL_TYPE_U64, > [RDMA_NLDEV_ATTR_NDEV_INDEX] = MNL_TYPE_U32, > [RDMA_NLDEV_ATTR_NDEV_NAME] = MNL_TYPE_NUL_STRING, > + [RDMA_NLDEV_ATTR_DRIVER] = MNL_TYPE_NESTED, > + [RDMA_NLDEV_ATTR_DRIVER_ENTRY] = MNL_TYPE_NESTED, > + [RDMA_NLDEV_ATTR_DRIVER_STRING] = MNL_TYPE_NUL_STRING, > + [RDMA_NLDEV_ATTR_DRIVER_PRINT_TYPE] = MNL_TYPE_U8, > + [RDMA_NLDEV_ATTR_DRIVER_S32] = MNL_TYPE_U32, > + [RDMA_NLDEV_ATTR_DRIVER_U32] = MNL_TYPE_U32, > + [RDMA_NLDEV_ATTR_DRIVER_S64] = MNL_TYPE_U64, > + [RDMA_NLDEV_ATTR_DRIVER_U64] = MNL_TYPE_U64, > }; > > +int rd_attr_check(const struct nlattr *attr, int *typep) > +{ > + int type; > + > + if (mnl_attr_type_valid(attr, RDMA_NLDEV_ATTR_MAX) < 0) > + return MNL_CB_ERROR; > + > + type = mnl_attr_get_type(attr); > + > + if (mnl_attr_validate(attr, nldev_policy[type]) < 0) > + return MNL_CB_ERROR; > + > + *typep = nldev_policy[type]; > + return MNL_CB_OK; > +} > + > int rd_attr_cb(const struct nlattr *attr, void *data) > { > const struct nlattr **tb = data; > @@ -660,3 +685,172 @@ struct dev_map *dev_map_lookup(struct rd *rd, bool allow_port_index) > free(dev_name); > return dev_map; > } > + > +#define nla_type(attr) ((attr)->nla_type & NLA_TYPE_MASK) > + > +void newline(struct rd *rd) > +{ > + if (rd->json_output) > + jsonw_end_array(rd->jw); > + else > + pr_out("\n"); > +} > + > +void newline_indent(struct rd *rd) > +{ > + newline(rd); > + if (!rd->json_output) > + pr_out(" "); > +} > + > +static int print_driver_string(struct rd *rd, const char *key_str, > + const char *val_str) > +{ > + if (rd->json_output) { > + jsonw_string_field(rd->jw, key_str, val_str); > + return 0; > + } else { > + return pr_out("%s %s ", key_str, val_str); > + } > +} > + > +static int print_driver_s32(struct rd *rd, const char *key_str, int32_t val, > + enum rdma_nldev_print_type print_type) > +{ > + if (rd->json_output) { > + jsonw_int_field(rd->jw, key_str, val); > + return 0; > + } > + switch (print_type) { > + case RDMA_NLDEV_PRINT_TYPE_UNSPEC: > + return pr_out("%s %d ", key_str, val); > + case RDMA_NLDEV_PRINT_TYPE_HEX: > + return pr_out("%s 0x%x ", key_str, val); > + default: > + return -EINVAL; > + } > +} > + > +static int print_driver_u32(struct rd *rd, const char *key_str, uint32_t val, > + enum rdma_nldev_print_type print_type) > +{ > + if (rd->json_output) { > + jsonw_int_field(rd->jw, key_str, val); > + return 0; > + } > + switch (print_type) { > + case RDMA_NLDEV_PRINT_TYPE_UNSPEC: > + return pr_out("%s %u ", key_str, val); > + case RDMA_NLDEV_PRINT_TYPE_HEX: > + return pr_out("%s 0x%x ", key_str, val); > + default: > + return -EINVAL; > + } > +} > + > +static int print_driver_s64(struct rd *rd, const char *key_str, int64_t val, > + enum rdma_nldev_print_type print_type) > +{ > + if (rd->json_output) { > + jsonw_int_field(rd->jw, key_str, val); > + return 0; > + } > + switch (print_type) { > + case RDMA_NLDEV_PRINT_TYPE_UNSPEC: > + return pr_out("%s %" PRId64 " ", key_str, val); > + case RDMA_NLDEV_PRINT_TYPE_HEX: > + return pr_out("%s 0x%" PRIx64 " ", key_str, val); > + default: > + return -EINVAL; > + } > +} > + > +static int print_driver_u64(struct rd *rd, const char *key_str, uint64_t val, > + enum rdma_nldev_print_type print_type) > +{ > + if (rd->json_output) { > + jsonw_int_field(rd->jw, key_str, val); > + return 0; > + } > + switch (print_type) { > + case RDMA_NLDEV_PRINT_TYPE_UNSPEC: > + return pr_out("%s %" PRIu64 " ", key_str, val); > + case RDMA_NLDEV_PRINT_TYPE_HEX: > + return pr_out("%s 0x%" PRIx64 " ", key_str, val); > + default: > + return -EINVAL; > + } > +} > + > +static int print_driver_entry(struct rd *rd, struct nlattr *key_attr, > + struct nlattr *val_attr, > + enum rdma_nldev_print_type print_type) > +{ > + const char *key_str = mnl_attr_get_str(key_attr); > + int attr_type = nla_type(val_attr); > + > + switch (attr_type) { > + case RDMA_NLDEV_ATTR_DRIVER_STRING: > + return print_driver_string(rd, key_str, > + mnl_attr_get_str(val_attr)); > + case RDMA_NLDEV_ATTR_DRIVER_S32: > + return print_driver_s32(rd, key_str, > + mnl_attr_get_u32(val_attr), print_type); > + case RDMA_NLDEV_ATTR_DRIVER_U32: > + return print_driver_u32(rd, key_str, > + mnl_attr_get_u32(val_attr), print_type); > + case RDMA_NLDEV_ATTR_DRIVER_S64: > + return print_driver_s64(rd, key_str, > + mnl_attr_get_u64(val_attr), print_type); > + case RDMA_NLDEV_ATTR_DRIVER_U64: > + return print_driver_u64(rd, key_str, > + mnl_attr_get_u64(val_attr), print_type); > + } > + return -EINVAL; > +} > + > +void print_driver_table(struct rd *rd, struct nlattr *tb) > +{ > + int print_type = RDMA_NLDEV_PRINT_TYPE_UNSPEC; > + struct nlattr *tb_entry, *key = NULL, *val; > + int type, cc = 0; > + > + if (!rd->show_driver_details || !tb) > + return; > + > + if (rd->pretty_output) > + newline_indent(rd); > + > + /* > + * Driver attrs are tuples of {key, [print-type], value}. > + * The key must be a string. If print-type is present, it > + * defines an alternate printf format type vs the native format > + * for the attribute. And the value can be any available > + * driver type. > + */ > + mnl_attr_for_each_nested(tb_entry, tb) { > + > + if (cc > MAX_LINE_LENGTH) { > + if (rd->pretty_output) > + newline_indent(rd); > + cc = 0; > + } > + if (rd_attr_check(tb_entry, &type) != MNL_CB_OK) > + return; > + if (!key) { > + if (type != MNL_TYPE_NUL_STRING) > + return; > + key = tb_entry; > + } else if (type == MNL_TYPE_U8) { > + print_type = mnl_attr_get_u8(tb_entry); > + } else { > + val = tb_entry; > + cc += print_driver_entry(rd, key, val, print_type); I stopped to read here, because of two problems: 1. print_driver_entry can return negative number, so unclear to me what will be the final result of "cc += ..". 2. The netlink design is to ignore unknown attributes and not return error. It allows to use new kernels with old applications. > + if (cc < 0) > + return; > + print_type = RDMA_NLDEV_PRINT_TYPE_UNSPEC; > + key = NULL; > + } > + } > + return; > +} > -- > 1.8.3.1 >
On 5/13/2018 8:24 AM, Leon Romanovsky wrote: > On Mon, May 07, 2018 at 08:53:16AM -0700, Steve Wise wrote: >> This enhancement allows printing rdma device-specific state, if provided >> by the kernel. This is done in a generic manner, so rdma tool doesn't > Double space between "." and "This". > >> need to know about the details of every type of rdma device. >> >> Driver attributes for a rdma resource are in the form of <key, >> [print_type], value> tuples, where the key is a string and the value can >> be any supported driver attribute. The print_type attribute, if present, > ditto I'll fix these. >> provides a print format to use vs the standard print format for the type. >> For example, the default print type for a PROVIDER_S32 value is "%d ", >> but "0x%x " if the print_type of PRINT_TYPE_HEX is included inthe tuple. >> >> Driver resources are only printed when the -dd flag is present. >> If -p is present, then the output is formatted to not exceed 80 columns, >> otherwise it is printed as a single row to be grep/awk friendly. >> >> Example output: >> >> # rdma resource show qp lqpn 1028 -dd -p >> link cxgb4_0/- lqpn 1028 rqpn 0 type RC state RTS rq-psn 0 sq-psn 0 path-mig-state MIGRATED pid 0 comm [nvme_rdma] >> sqid 1028 flushed 0 memsize 123968 cidx 85 pidx 85 wq_pidx 106 flush_cidx 85 in_use 0 >> size 386 flags 0x0 rqid 1029 memsize 16768 cidx 43 pidx 41 wq_pidx 171 msn 44 rqt_hwaddr 0x2a8a5d00 >> rqt_size 256 in_use 128 size 130 idx 43 wr_id 0xffff881057c03408 idx 40 wr_id 0xffff881057c033f0 >> >> Signed-off-by: Steve Wise <swise@opengridcomputing.com> >> --- >> rdma/rdma.c | 7 ++- >> rdma/rdma.h | 11 ++++ >> rdma/res.c | 30 +++------ >> rdma/utils.c | 194 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 4 files changed, 221 insertions(+), 21 deletions(-) >> >> diff --git a/rdma/rdma.c b/rdma/rdma.c >> index b43e538..0155627 100644 >> --- a/rdma/rdma.c >> +++ b/rdma/rdma.c >> @@ -132,6 +132,7 @@ int main(int argc, char **argv) >> const char *batch_file = NULL; >> bool pretty_output = false; >> bool show_details = false; >> + bool show_driver_details = false; > Reversed Christmas tree please. Sure. >> bool json_output = false; >> bool force = false; >> char *filename; >> @@ -152,7 +153,10 @@ int main(int argc, char **argv) >> pretty_output = true; >> break; >> case 'd': >> - show_details = true; >> + if (show_details) >> + show_driver_details = true; >> + else >> + show_details = true; >> break; >> case 'j': >> json_output = true; >> @@ -180,6 +184,7 @@ int main(int argc, char **argv) >> argv += optind; >> >> rd.show_details = show_details; >> + rd.show_driver_details = show_driver_details; >> rd.json_output = json_output; >> rd.pretty_output = pretty_output; >> >> diff --git a/rdma/rdma.h b/rdma/rdma.h >> index 1908fc4..fcaf9e6 100644 >> --- a/rdma/rdma.h >> +++ b/rdma/rdma.h >> @@ -55,6 +55,7 @@ struct rd { >> char **argv; >> char *filename; >> bool show_details; >> + bool show_driver_details; >> struct list_head dev_map_list; >> uint32_t dev_idx; >> uint32_t port_idx; >> @@ -115,4 +116,14 @@ int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void *data, uint32_t seq); >> void rd_prepare_msg(struct rd *rd, uint32_t cmd, uint32_t *seq, uint16_t flags); >> int rd_dev_init_cb(const struct nlmsghdr *nlh, void *data); >> int rd_attr_cb(const struct nlattr *attr, void *data); >> +int rd_attr_check(const struct nlattr *attr, int *typep); >> + >> +/* >> + * Print helpers >> + */ >> +void print_driver_table(struct rd *rd, struct nlattr *tb); >> +void newline(struct rd *rd); >> +void newline_indent(struct rd *rd); >> +#define MAX_LINE_LENGTH 80 >> + >> #endif /* _RDMA_TOOL_H_ */ >> diff --git a/rdma/res.c b/rdma/res.c >> index 1a0aab6..074b992 100644 >> --- a/rdma/res.c >> +++ b/rdma/res.c >> @@ -439,10 +439,8 @@ static int res_qp_parse_cb(const struct nlmsghdr *nlh, void *data) >> if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) >> free(comm); >> >> - if (rd->json_output) >> - jsonw_end_array(rd->jw); >> - else >> - pr_out("\n"); >> + print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]); >> + newline(rd); >> } >> return MNL_CB_OK; >> } >> @@ -678,10 +676,8 @@ static int res_cm_id_parse_cb(const struct nlmsghdr *nlh, void *data) >> if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) >> free(comm); >> >> - if (rd->json_output) >> - jsonw_end_array(rd->jw); >> - else >> - pr_out("\n"); >> + print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]); >> + newline(rd); >> } >> return MNL_CB_OK; >> } >> @@ -804,10 +800,8 @@ static int res_cq_parse_cb(const struct nlmsghdr *nlh, void *data) >> if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) >> free(comm); >> >> - if (rd->json_output) >> - jsonw_end_array(rd->jw); >> - else >> - pr_out("\n"); >> + print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]); >> + newline(rd); >> } >> return MNL_CB_OK; >> } >> @@ -919,10 +913,8 @@ static int res_mr_parse_cb(const struct nlmsghdr *nlh, void *data) >> if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) >> free(comm); >> >> - if (rd->json_output) >> - jsonw_end_array(rd->jw); >> - else >> - pr_out("\n"); >> + print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]); >> + newline(rd); >> } >> return MNL_CB_OK; >> } >> @@ -1004,10 +996,8 @@ static int res_pd_parse_cb(const struct nlmsghdr *nlh, void *data) >> if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) >> free(comm); >> >> - if (rd->json_output) >> - jsonw_end_array(rd->jw); >> - else >> - pr_out("\n"); >> + print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]); >> + newline(rd); >> } >> return MNL_CB_OK; >> } >> diff --git a/rdma/utils.c b/rdma/utils.c >> index 49c967f..452fe92 100644 >> --- a/rdma/utils.c >> +++ b/rdma/utils.c >> @@ -11,6 +11,7 @@ >> >> #include "rdma.h" >> #include <ctype.h> >> +#include <inttypes.h> >> >> int rd_argc(struct rd *rd) >> { >> @@ -393,8 +394,32 @@ static const enum mnl_attr_data_type nldev_policy[RDMA_NLDEV_ATTR_MAX] = { >> [RDMA_NLDEV_ATTR_RES_MRLEN] = MNL_TYPE_U64, >> [RDMA_NLDEV_ATTR_NDEV_INDEX] = MNL_TYPE_U32, >> [RDMA_NLDEV_ATTR_NDEV_NAME] = MNL_TYPE_NUL_STRING, >> + [RDMA_NLDEV_ATTR_DRIVER] = MNL_TYPE_NESTED, >> + [RDMA_NLDEV_ATTR_DRIVER_ENTRY] = MNL_TYPE_NESTED, >> + [RDMA_NLDEV_ATTR_DRIVER_STRING] = MNL_TYPE_NUL_STRING, >> + [RDMA_NLDEV_ATTR_DRIVER_PRINT_TYPE] = MNL_TYPE_U8, >> + [RDMA_NLDEV_ATTR_DRIVER_S32] = MNL_TYPE_U32, >> + [RDMA_NLDEV_ATTR_DRIVER_U32] = MNL_TYPE_U32, >> + [RDMA_NLDEV_ATTR_DRIVER_S64] = MNL_TYPE_U64, >> + [RDMA_NLDEV_ATTR_DRIVER_U64] = MNL_TYPE_U64, >> }; >> >> +int rd_attr_check(const struct nlattr *attr, int *typep) >> +{ >> + int type; >> + >> + if (mnl_attr_type_valid(attr, RDMA_NLDEV_ATTR_MAX) < 0) >> + return MNL_CB_ERROR; >> + >> + type = mnl_attr_get_type(attr); >> + >> + if (mnl_attr_validate(attr, nldev_policy[type]) < 0) >> + return MNL_CB_ERROR; >> + >> + *typep = nldev_policy[type]; >> + return MNL_CB_OK; >> +} >> + >> int rd_attr_cb(const struct nlattr *attr, void *data) >> { >> const struct nlattr **tb = data; >> @@ -660,3 +685,172 @@ struct dev_map *dev_map_lookup(struct rd *rd, bool allow_port_index) >> free(dev_name); >> return dev_map; >> } >> + >> +#define nla_type(attr) ((attr)->nla_type & NLA_TYPE_MASK) >> + >> +void newline(struct rd *rd) >> +{ >> + if (rd->json_output) >> + jsonw_end_array(rd->jw); >> + else >> + pr_out("\n"); >> +} >> + >> +void newline_indent(struct rd *rd) >> +{ >> + newline(rd); >> + if (!rd->json_output) >> + pr_out(" "); >> +} >> + >> +static int print_driver_string(struct rd *rd, const char *key_str, >> + const char *val_str) >> +{ >> + if (rd->json_output) { >> + jsonw_string_field(rd->jw, key_str, val_str); >> + return 0; >> + } else { >> + return pr_out("%s %s ", key_str, val_str); >> + } >> +} >> + >> +static int print_driver_s32(struct rd *rd, const char *key_str, int32_t val, >> + enum rdma_nldev_print_type print_type) >> +{ >> + if (rd->json_output) { >> + jsonw_int_field(rd->jw, key_str, val); >> + return 0; >> + } >> + switch (print_type) { >> + case RDMA_NLDEV_PRINT_TYPE_UNSPEC: >> + return pr_out("%s %d ", key_str, val); >> + case RDMA_NLDEV_PRINT_TYPE_HEX: >> + return pr_out("%s 0x%x ", key_str, val); >> + default: >> + return -EINVAL; >> + } >> +} >> + >> +static int print_driver_u32(struct rd *rd, const char *key_str, uint32_t val, >> + enum rdma_nldev_print_type print_type) >> +{ >> + if (rd->json_output) { >> + jsonw_int_field(rd->jw, key_str, val); >> + return 0; >> + } >> + switch (print_type) { >> + case RDMA_NLDEV_PRINT_TYPE_UNSPEC: >> + return pr_out("%s %u ", key_str, val); >> + case RDMA_NLDEV_PRINT_TYPE_HEX: >> + return pr_out("%s 0x%x ", key_str, val); >> + default: >> + return -EINVAL; >> + } >> +} >> + >> +static int print_driver_s64(struct rd *rd, const char *key_str, int64_t val, >> + enum rdma_nldev_print_type print_type) >> +{ >> + if (rd->json_output) { >> + jsonw_int_field(rd->jw, key_str, val); >> + return 0; >> + } >> + switch (print_type) { >> + case RDMA_NLDEV_PRINT_TYPE_UNSPEC: >> + return pr_out("%s %" PRId64 " ", key_str, val); >> + case RDMA_NLDEV_PRINT_TYPE_HEX: >> + return pr_out("%s 0x%" PRIx64 " ", key_str, val); >> + default: >> + return -EINVAL; >> + } >> +} >> + >> +static int print_driver_u64(struct rd *rd, const char *key_str, uint64_t val, >> + enum rdma_nldev_print_type print_type) >> +{ >> + if (rd->json_output) { >> + jsonw_int_field(rd->jw, key_str, val); >> + return 0; >> + } >> + switch (print_type) { >> + case RDMA_NLDEV_PRINT_TYPE_UNSPEC: >> + return pr_out("%s %" PRIu64 " ", key_str, val); >> + case RDMA_NLDEV_PRINT_TYPE_HEX: >> + return pr_out("%s 0x%" PRIx64 " ", key_str, val); >> + default: >> + return -EINVAL; >> + } >> +} >> + >> +static int print_driver_entry(struct rd *rd, struct nlattr *key_attr, >> + struct nlattr *val_attr, >> + enum rdma_nldev_print_type print_type) >> +{ >> + const char *key_str = mnl_attr_get_str(key_attr); >> + int attr_type = nla_type(val_attr); >> + >> + switch (attr_type) { >> + case RDMA_NLDEV_ATTR_DRIVER_STRING: >> + return print_driver_string(rd, key_str, >> + mnl_attr_get_str(val_attr)); >> + case RDMA_NLDEV_ATTR_DRIVER_S32: >> + return print_driver_s32(rd, key_str, >> + mnl_attr_get_u32(val_attr), print_type); >> + case RDMA_NLDEV_ATTR_DRIVER_U32: >> + return print_driver_u32(rd, key_str, >> + mnl_attr_get_u32(val_attr), print_type); >> + case RDMA_NLDEV_ATTR_DRIVER_S64: >> + return print_driver_s64(rd, key_str, >> + mnl_attr_get_u64(val_attr), print_type); >> + case RDMA_NLDEV_ATTR_DRIVER_U64: >> + return print_driver_u64(rd, key_str, >> + mnl_attr_get_u64(val_attr), print_type); >> + } >> + return -EINVAL; >> +} >> + >> +void print_driver_table(struct rd *rd, struct nlattr *tb) >> +{ >> + int print_type = RDMA_NLDEV_PRINT_TYPE_UNSPEC; >> + struct nlattr *tb_entry, *key = NULL, *val; >> + int type, cc = 0; >> + >> + if (!rd->show_driver_details || !tb) >> + return; >> + >> + if (rd->pretty_output) >> + newline_indent(rd); >> + >> + /* >> + * Driver attrs are tuples of {key, [print-type], value}. >> + * The key must be a string. If print-type is present, it >> + * defines an alternate printf format type vs the native format >> + * for the attribute. And the value can be any available >> + * driver type. >> + */ >> + mnl_attr_for_each_nested(tb_entry, tb) { >> + >> + if (cc > MAX_LINE_LENGTH) { >> + if (rd->pretty_output) >> + newline_indent(rd); >> + cc = 0; >> + } >> + if (rd_attr_check(tb_entry, &type) != MNL_CB_OK) >> + return; >> + if (!key) { >> + if (type != MNL_TYPE_NUL_STRING) >> + return; >> + key = tb_entry; >> + } else if (type == MNL_TYPE_U8) { >> + print_type = mnl_attr_get_u8(tb_entry); >> + } else { >> + val = tb_entry; >> + cc += print_driver_entry(rd, key, val, print_type); > I stopped to read here, because of two problems: > 1. print_driver_entry can return negative number, so unclear to me what > will be the final result of "cc += ..". > 2. The netlink design is to ignore unknown attributes and not return > error. It allows to use new kernels with old applications. Yes, this is a bug. See below the check for 'cc < 0': >> + if (cc < 0) >> + return; It's incorrect. The return from print_driver_entry() should be checked for error to allow ignoring unknown attrs, and then if no error, cc gets incremented. I'll fix this up. Thanks for reviewing, Steve. >> + print_type = RDMA_NLDEV_PRINT_TYPE_UNSPEC; >> + key = NULL; >> + } >> + } >> + return; >> +} >> -- >> 1.8.3.1 >> -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 07, 2018 at 08:53:16AM -0700, Steve Wise wrote: > This enhancement allows printing rdma device-specific state, if provided > by the kernel. This is done in a generic manner, so rdma tool doesn't > need to know about the details of every type of rdma device. > > Driver attributes for a rdma resource are in the form of <key, > [print_type], value> tuples, where the key is a string and the value can > be any supported driver attribute. The print_type attribute, if present, > provides a print format to use vs the standard print format for the type. > For example, the default print type for a PROVIDER_S32 value is "%d ", > but "0x%x " if the print_type of PRINT_TYPE_HEX is included inthe tuple. > > Driver resources are only printed when the -dd flag is present. > If -p is present, then the output is formatted to not exceed 80 columns, > otherwise it is printed as a single row to be grep/awk friendly. > > Example output: > > # rdma resource show qp lqpn 1028 -dd -p > link cxgb4_0/- lqpn 1028 rqpn 0 type RC state RTS rq-psn 0 sq-psn 0 path-mig-state MIGRATED pid 0 comm [nvme_rdma] > sqid 1028 flushed 0 memsize 123968 cidx 85 pidx 85 wq_pidx 106 flush_cidx 85 in_use 0 > size 386 flags 0x0 rqid 1029 memsize 16768 cidx 43 pidx 41 wq_pidx 171 msn 44 rqt_hwaddr 0x2a8a5d00 > rqt_size 256 in_use 128 size 130 idx 43 wr_id 0xffff881057c03408 idx 40 wr_id 0xffff881057c033f0 Hey some of these look like kernel pointers.. That is a no-no.. What is up there? The wr_id often contains a pointer, right? So we cannot just pass it to user space.. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5/14/2018 3:41 PM, Jason Gunthorpe wrote: > On Mon, May 07, 2018 at 08:53:16AM -0700, Steve Wise wrote: >> This enhancement allows printing rdma device-specific state, if provided >> by the kernel. This is done in a generic manner, so rdma tool doesn't >> need to know about the details of every type of rdma device. >> >> Driver attributes for a rdma resource are in the form of <key, >> [print_type], value> tuples, where the key is a string and the value can >> be any supported driver attribute. The print_type attribute, if present, >> provides a print format to use vs the standard print format for the type. >> For example, the default print type for a PROVIDER_S32 value is "%d ", >> but "0x%x " if the print_type of PRINT_TYPE_HEX is included inthe tuple. >> >> Driver resources are only printed when the -dd flag is present. >> If -p is present, then the output is formatted to not exceed 80 columns, >> otherwise it is printed as a single row to be grep/awk friendly. >> >> Example output: >> >> # rdma resource show qp lqpn 1028 -dd -p >> link cxgb4_0/- lqpn 1028 rqpn 0 type RC state RTS rq-psn 0 sq-psn 0 path-mig-state MIGRATED pid 0 comm [nvme_rdma] >> sqid 1028 flushed 0 memsize 123968 cidx 85 pidx 85 wq_pidx 106 flush_cidx 85 in_use 0 >> size 386 flags 0x0 rqid 1029 memsize 16768 cidx 43 pidx 41 wq_pidx 171 msn 44 rqt_hwaddr 0x2a8a5d00 >> rqt_size 256 in_use 128 size 130 idx 43 wr_id 0xffff881057c03408 idx 40 wr_id 0xffff881057c033f0 > Hey some of these look like kernel pointers.. That is a no-no.. What > is up there? Nothing is defined as a kernel pointer. But wr_id is often a pointer to the kernel rdma application's context... > The wr_id often contains a pointer, right? So we cannot just pass it > to user space.. Hmm. It is useful for debugging kernel rdma applications. Perhaps these attrs can be only be sent up by the kernel if the capabilities allow. But previous review comments of the kernel series, which is now merged, forced me to remove passing the capabilities information to the driver resource fill functions. So what's the right way to do this? Steve. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 14, 2018 at 05:04:26PM -0500, Steve Wise wrote: > > > On 5/14/2018 3:41 PM, Jason Gunthorpe wrote: > > On Mon, May 07, 2018 at 08:53:16AM -0700, Steve Wise wrote: > >> This enhancement allows printing rdma device-specific state, if provided > >> by the kernel. This is done in a generic manner, so rdma tool doesn't > >> need to know about the details of every type of rdma device. > >> > >> Driver attributes for a rdma resource are in the form of <key, > >> [print_type], value> tuples, where the key is a string and the value can > >> be any supported driver attribute. The print_type attribute, if present, > >> provides a print format to use vs the standard print format for the type. > >> For example, the default print type for a PROVIDER_S32 value is "%d ", > >> but "0x%x " if the print_type of PRINT_TYPE_HEX is included inthe tuple. > >> > >> Driver resources are only printed when the -dd flag is present. > >> If -p is present, then the output is formatted to not exceed 80 columns, > >> otherwise it is printed as a single row to be grep/awk friendly. > >> > >> Example output: > >> > >> # rdma resource show qp lqpn 1028 -dd -p > >> link cxgb4_0/- lqpn 1028 rqpn 0 type RC state RTS rq-psn 0 sq-psn 0 path-mig-state MIGRATED pid 0 comm [nvme_rdma] > >> sqid 1028 flushed 0 memsize 123968 cidx 85 pidx 85 wq_pidx 106 flush_cidx 85 in_use 0 > >> size 386 flags 0x0 rqid 1029 memsize 16768 cidx 43 pidx 41 wq_pidx 171 msn 44 rqt_hwaddr 0x2a8a5d00 > >> rqt_size 256 in_use 128 size 130 idx 43 wr_id 0xffff881057c03408 idx 40 wr_id 0xffff881057c033f0 > > Hey some of these look like kernel pointers.. That is a no-no.. What > > is up there? > > Nothing is defined as a kernel pointer. But wr_id is often a pointer to > the kernel rdma application's context... > > > The wr_id often contains a pointer, right? So we cannot just pass it > > to user space.. > > Hmm. It is useful for debugging kernel rdma applications. Perhaps > these attrs can be only be sent up by the kernel if the capabilities > allow. But previous review comments of the kernel series, which is now > merged, forced me to remove passing the capabilities information to the > driver resource fill functions. > > So what's the right way to do this? The reviewer asked do not pass to drivers whole CAP_.. bits, because they anyway don't need such granularity. > > Steve. > >
> On Mon, May 14, 2018 at 05:04:26PM -0500, Steve Wise wrote: > > > > > > On 5/14/2018 3:41 PM, Jason Gunthorpe wrote: > > > On Mon, May 07, 2018 at 08:53:16AM -0700, Steve Wise wrote: > > >> This enhancement allows printing rdma device-specific state, if provided > > >> by the kernel. This is done in a generic manner, so rdma tool doesn't > > >> need to know about the details of every type of rdma device. > > >> > > >> Driver attributes for a rdma resource are in the form of <key, > > >> [print_type], value> tuples, where the key is a string and the value can > > >> be any supported driver attribute. The print_type attribute, if present, > > >> provides a print format to use vs the standard print format for the type. > > >> For example, the default print type for a PROVIDER_S32 value is "%d ", > > >> but "0x%x " if the print_type of PRINT_TYPE_HEX is included inthe tuple. > > >> > > >> Driver resources are only printed when the -dd flag is present. > > >> If -p is present, then the output is formatted to not exceed 80 columns, > > >> otherwise it is printed as a single row to be grep/awk friendly. > > >> > > >> Example output: > > >> > > >> # rdma resource show qp lqpn 1028 -dd -p > > >> link cxgb4_0/- lqpn 1028 rqpn 0 type RC state RTS rq-psn 0 sq-psn 0 > path-mig-state MIGRATED pid 0 comm [nvme_rdma] > > >> sqid 1028 flushed 0 memsize 123968 cidx 85 pidx 85 wq_pidx 106 > flush_cidx 85 in_use 0 > > >> size 386 flags 0x0 rqid 1029 memsize 16768 cidx 43 pidx 41 wq_pidx > 171 msn 44 rqt_hwaddr 0x2a8a5d00 > > >> rqt_size 256 in_use 128 size 130 idx 43 wr_id 0xffff881057c03408 idx > 40 wr_id 0xffff881057c033f0 > > > Hey some of these look like kernel pointers.. That is a no-no.. What > > > is up there? > > > > Nothing is defined as a kernel pointer. But wr_id is often a pointer to > > the kernel rdma application's context... > > > > > The wr_id often contains a pointer, right? So we cannot just pass it > > > to user space.. > > > > Hmm. It is useful for debugging kernel rdma applications. Perhaps > > these attrs can be only be sent up by the kernel if the capabilities > > allow. But previous review comments of the kernel series, which is now > > merged, forced me to remove passing the capabilities information to the > > driver resource fill functions. > > > > So what's the right way to do this? > > The reviewer asked do not pass to drivers whole CAP_.. bits, because > they anyway don't need such granularity. > Ok thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2018-05-14 at 09:51 -0500, Steve Wise wrote: > > On 5/13/2018 8:24 AM, Leon Romanovsky wrote: > > On Mon, May 07, 2018 at 08:53:16AM -0700, Steve Wise wrote: > > > This enhancement allows printing rdma device-specific state, if provided > > > by the kernel. This is done in a generic manner, so rdma tool doesn't > > > > Double space between "." and "This". > > > > > need to know about the details of every type of rdma device. > > > > > > Driver attributes for a rdma resource are in the form of <key, > > > [print_type], value> tuples, where the key is a string and the value can > > > be any supported driver attribute. The print_type attribute, if present, > > > > ditto > > I'll fix these. Fix it if you want, but don't do it because Leon told you to. A double space after period is perfectly acceptable.
On Tue, May 15, 2018 at 12:35:34PM -0400, Doug Ledford wrote: > On Mon, 2018-05-14 at 09:51 -0500, Steve Wise wrote: > > > > On 5/13/2018 8:24 AM, Leon Romanovsky wrote: > > > On Mon, May 07, 2018 at 08:53:16AM -0700, Steve Wise wrote: > > > > This enhancement allows printing rdma device-specific state, if provided > > > > by the kernel. This is done in a generic manner, so rdma tool doesn't > > > > > > Double space between "." and "This". > > > > > > > need to know about the details of every type of rdma device. > > > > > > > > Driver attributes for a rdma resource are in the form of <key, > > > > [print_type], value> tuples, where the key is a string and the value can > > > > be any supported driver attribute. The print_type attribute, if present, > > > > > > ditto > > > > I'll fix these. > > Fix it if you want, but don't do it because Leon told you to. A double > space after period is perfectly acceptable. It is very controversial thing [1], "Most style guides indicate that single sentence spacing is proper for final or published work today, and most publishers require manuscripts to be submitted as they will appear in publication—single sentence spaced." [1] https://en.wikipedia.org/wiki/Sentence_spacing > > -- > Doug Ledford <dledford@redhat.com> > GPG KeyID: B826A3330E572FDD > Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
> > On Tue, May 15, 2018 at 12:35:34PM -0400, Doug Ledford wrote: > > On Mon, 2018-05-14 at 09:51 -0500, Steve Wise wrote: > > > > > > On 5/13/2018 8:24 AM, Leon Romanovsky wrote: > > > > On Mon, May 07, 2018 at 08:53:16AM -0700, Steve Wise wrote: > > > > > This enhancement allows printing rdma device-specific state, if > provided > > > > > by the kernel. This is done in a generic manner, so rdma tool doesn't > > > > > > > > Double space between "." and "This". > > > > > > > > > need to know about the details of every type of rdma device. > > > > > > > > > > Driver attributes for a rdma resource are in the form of <key, > > > > > [print_type], value> tuples, where the key is a string and the value can > > > > > be any supported driver attribute. The print_type attribute, if > present, > > > > > > > > ditto > > > > > > I'll fix these. > > > > Fix it if you want, but don't do it because Leon told you to. A double > > space after period is perfectly acceptable. > > It is very controversial thing [1], > > "Most style guides indicate that single sentence spacing is proper for > final or published work today, and most publishers require manuscripts > to be submitted as they will appear in publication—single > sentence spaced." > > [1] https://en.wikipedia.org/wiki/Sentence_spacing We're not writing a manuscript.
On Tue, 2018-05-15 at 19:59 +0300, Leon Romanovsky wrote: > On Tue, May 15, 2018 at 12:35:34PM -0400, Doug Ledford wrote: > > On Mon, 2018-05-14 at 09:51 -0500, Steve Wise wrote: > > > > > > On 5/13/2018 8:24 AM, Leon Romanovsky wrote: > > > > On Mon, May 07, 2018 at 08:53:16AM -0700, Steve Wise wrote: > > > > > This enhancement allows printing rdma device-specific state, if provided > > > > > by the kernel. This is done in a generic manner, so rdma tool doesn't > > > > > > > > Double space between "." and "This". > > > > > > > > > need to know about the details of every type of rdma device. > > > > > > > > > > Driver attributes for a rdma resource are in the form of <key, > > > > > [print_type], value> tuples, where the key is a string and the value can > > > > > be any supported driver attribute. The print_type attribute, if present, > > > > > > > > ditto > > > > > > I'll fix these. > > > > Fix it if you want, but don't do it because Leon told you to. A double > > space after period is perfectly acceptable. > > It is very controversial thing [1], > > "Most style guides indicate that single sentence spacing is proper for > final or published work today, and most publishers require manuscripts > to be submitted as they will appear in publication—single > sentence spaced." > > [1] https://en.wikipedia.org/wiki/Sentence_spacing Yes...and the justification is that proportional font systems resolve the issue of delineating sentences without the need for a second space (they actually don't because they are improperly implemented, but that's another issue). But we don't work on proportional font systems, we work with git, on command lines, with fixed spacing fonts, all the things that indicate double spaces are actually preferred even by those people that are proponents of single spacing. And on top of that, current research suggests maybe the single spacers are wrong after all :-o https://www.theatlantic.com/science/archive/2018/05/two-spaces-after-a-p eriod/559304/ Anyway, my original point stands. It's controversial, as you said, so Steve should do what Steve feels is best and not worry about anyone else' opinion on the matter.
diff --git a/rdma/rdma.c b/rdma/rdma.c index b43e538..0155627 100644 --- a/rdma/rdma.c +++ b/rdma/rdma.c @@ -132,6 +132,7 @@ int main(int argc, char **argv) const char *batch_file = NULL; bool pretty_output = false; bool show_details = false; + bool show_driver_details = false; bool json_output = false; bool force = false; char *filename; @@ -152,7 +153,10 @@ int main(int argc, char **argv) pretty_output = true; break; case 'd': - show_details = true; + if (show_details) + show_driver_details = true; + else + show_details = true; break; case 'j': json_output = true; @@ -180,6 +184,7 @@ int main(int argc, char **argv) argv += optind; rd.show_details = show_details; + rd.show_driver_details = show_driver_details; rd.json_output = json_output; rd.pretty_output = pretty_output; diff --git a/rdma/rdma.h b/rdma/rdma.h index 1908fc4..fcaf9e6 100644 --- a/rdma/rdma.h +++ b/rdma/rdma.h @@ -55,6 +55,7 @@ struct rd { char **argv; char *filename; bool show_details; + bool show_driver_details; struct list_head dev_map_list; uint32_t dev_idx; uint32_t port_idx; @@ -115,4 +116,14 @@ int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void *data, uint32_t seq); void rd_prepare_msg(struct rd *rd, uint32_t cmd, uint32_t *seq, uint16_t flags); int rd_dev_init_cb(const struct nlmsghdr *nlh, void *data); int rd_attr_cb(const struct nlattr *attr, void *data); +int rd_attr_check(const struct nlattr *attr, int *typep); + +/* + * Print helpers + */ +void print_driver_table(struct rd *rd, struct nlattr *tb); +void newline(struct rd *rd); +void newline_indent(struct rd *rd); +#define MAX_LINE_LENGTH 80 + #endif /* _RDMA_TOOL_H_ */ diff --git a/rdma/res.c b/rdma/res.c index 1a0aab6..074b992 100644 --- a/rdma/res.c +++ b/rdma/res.c @@ -439,10 +439,8 @@ static int res_qp_parse_cb(const struct nlmsghdr *nlh, void *data) if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) free(comm); - if (rd->json_output) - jsonw_end_array(rd->jw); - else - pr_out("\n"); + print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]); + newline(rd); } return MNL_CB_OK; } @@ -678,10 +676,8 @@ static int res_cm_id_parse_cb(const struct nlmsghdr *nlh, void *data) if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) free(comm); - if (rd->json_output) - jsonw_end_array(rd->jw); - else - pr_out("\n"); + print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]); + newline(rd); } return MNL_CB_OK; } @@ -804,10 +800,8 @@ static int res_cq_parse_cb(const struct nlmsghdr *nlh, void *data) if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) free(comm); - if (rd->json_output) - jsonw_end_array(rd->jw); - else - pr_out("\n"); + print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]); + newline(rd); } return MNL_CB_OK; } @@ -919,10 +913,8 @@ static int res_mr_parse_cb(const struct nlmsghdr *nlh, void *data) if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) free(comm); - if (rd->json_output) - jsonw_end_array(rd->jw); - else - pr_out("\n"); + print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]); + newline(rd); } return MNL_CB_OK; } @@ -1004,10 +996,8 @@ static int res_pd_parse_cb(const struct nlmsghdr *nlh, void *data) if (nla_line[RDMA_NLDEV_ATTR_RES_PID]) free(comm); - if (rd->json_output) - jsonw_end_array(rd->jw); - else - pr_out("\n"); + print_driver_table(rd, nla_line[RDMA_NLDEV_ATTR_DRIVER]); + newline(rd); } return MNL_CB_OK; } diff --git a/rdma/utils.c b/rdma/utils.c index 49c967f..452fe92 100644 --- a/rdma/utils.c +++ b/rdma/utils.c @@ -11,6 +11,7 @@ #include "rdma.h" #include <ctype.h> +#include <inttypes.h> int rd_argc(struct rd *rd) { @@ -393,8 +394,32 @@ static const enum mnl_attr_data_type nldev_policy[RDMA_NLDEV_ATTR_MAX] = { [RDMA_NLDEV_ATTR_RES_MRLEN] = MNL_TYPE_U64, [RDMA_NLDEV_ATTR_NDEV_INDEX] = MNL_TYPE_U32, [RDMA_NLDEV_ATTR_NDEV_NAME] = MNL_TYPE_NUL_STRING, + [RDMA_NLDEV_ATTR_DRIVER] = MNL_TYPE_NESTED, + [RDMA_NLDEV_ATTR_DRIVER_ENTRY] = MNL_TYPE_NESTED, + [RDMA_NLDEV_ATTR_DRIVER_STRING] = MNL_TYPE_NUL_STRING, + [RDMA_NLDEV_ATTR_DRIVER_PRINT_TYPE] = MNL_TYPE_U8, + [RDMA_NLDEV_ATTR_DRIVER_S32] = MNL_TYPE_U32, + [RDMA_NLDEV_ATTR_DRIVER_U32] = MNL_TYPE_U32, + [RDMA_NLDEV_ATTR_DRIVER_S64] = MNL_TYPE_U64, + [RDMA_NLDEV_ATTR_DRIVER_U64] = MNL_TYPE_U64, }; +int rd_attr_check(const struct nlattr *attr, int *typep) +{ + int type; + + if (mnl_attr_type_valid(attr, RDMA_NLDEV_ATTR_MAX) < 0) + return MNL_CB_ERROR; + + type = mnl_attr_get_type(attr); + + if (mnl_attr_validate(attr, nldev_policy[type]) < 0) + return MNL_CB_ERROR; + + *typep = nldev_policy[type]; + return MNL_CB_OK; +} + int rd_attr_cb(const struct nlattr *attr, void *data) { const struct nlattr **tb = data; @@ -660,3 +685,172 @@ struct dev_map *dev_map_lookup(struct rd *rd, bool allow_port_index) free(dev_name); return dev_map; } + +#define nla_type(attr) ((attr)->nla_type & NLA_TYPE_MASK) + +void newline(struct rd *rd) +{ + if (rd->json_output) + jsonw_end_array(rd->jw); + else + pr_out("\n"); +} + +void newline_indent(struct rd *rd) +{ + newline(rd); + if (!rd->json_output) + pr_out(" "); +} + +static int print_driver_string(struct rd *rd, const char *key_str, + const char *val_str) +{ + if (rd->json_output) { + jsonw_string_field(rd->jw, key_str, val_str); + return 0; + } else { + return pr_out("%s %s ", key_str, val_str); + } +} + +static int print_driver_s32(struct rd *rd, const char *key_str, int32_t val, + enum rdma_nldev_print_type print_type) +{ + if (rd->json_output) { + jsonw_int_field(rd->jw, key_str, val); + return 0; + } + switch (print_type) { + case RDMA_NLDEV_PRINT_TYPE_UNSPEC: + return pr_out("%s %d ", key_str, val); + case RDMA_NLDEV_PRINT_TYPE_HEX: + return pr_out("%s 0x%x ", key_str, val); + default: + return -EINVAL; + } +} + +static int print_driver_u32(struct rd *rd, const char *key_str, uint32_t val, + enum rdma_nldev_print_type print_type) +{ + if (rd->json_output) { + jsonw_int_field(rd->jw, key_str, val); + return 0; + } + switch (print_type) { + case RDMA_NLDEV_PRINT_TYPE_UNSPEC: + return pr_out("%s %u ", key_str, val); + case RDMA_NLDEV_PRINT_TYPE_HEX: + return pr_out("%s 0x%x ", key_str, val); + default: + return -EINVAL; + } +} + +static int print_driver_s64(struct rd *rd, const char *key_str, int64_t val, + enum rdma_nldev_print_type print_type) +{ + if (rd->json_output) { + jsonw_int_field(rd->jw, key_str, val); + return 0; + } + switch (print_type) { + case RDMA_NLDEV_PRINT_TYPE_UNSPEC: + return pr_out("%s %" PRId64 " ", key_str, val); + case RDMA_NLDEV_PRINT_TYPE_HEX: + return pr_out("%s 0x%" PRIx64 " ", key_str, val); + default: + return -EINVAL; + } +} + +static int print_driver_u64(struct rd *rd, const char *key_str, uint64_t val, + enum rdma_nldev_print_type print_type) +{ + if (rd->json_output) { + jsonw_int_field(rd->jw, key_str, val); + return 0; + } + switch (print_type) { + case RDMA_NLDEV_PRINT_TYPE_UNSPEC: + return pr_out("%s %" PRIu64 " ", key_str, val); + case RDMA_NLDEV_PRINT_TYPE_HEX: + return pr_out("%s 0x%" PRIx64 " ", key_str, val); + default: + return -EINVAL; + } +} + +static int print_driver_entry(struct rd *rd, struct nlattr *key_attr, + struct nlattr *val_attr, + enum rdma_nldev_print_type print_type) +{ + const char *key_str = mnl_attr_get_str(key_attr); + int attr_type = nla_type(val_attr); + + switch (attr_type) { + case RDMA_NLDEV_ATTR_DRIVER_STRING: + return print_driver_string(rd, key_str, + mnl_attr_get_str(val_attr)); + case RDMA_NLDEV_ATTR_DRIVER_S32: + return print_driver_s32(rd, key_str, + mnl_attr_get_u32(val_attr), print_type); + case RDMA_NLDEV_ATTR_DRIVER_U32: + return print_driver_u32(rd, key_str, + mnl_attr_get_u32(val_attr), print_type); + case RDMA_NLDEV_ATTR_DRIVER_S64: + return print_driver_s64(rd, key_str, + mnl_attr_get_u64(val_attr), print_type); + case RDMA_NLDEV_ATTR_DRIVER_U64: + return print_driver_u64(rd, key_str, + mnl_attr_get_u64(val_attr), print_type); + } + return -EINVAL; +} + +void print_driver_table(struct rd *rd, struct nlattr *tb) +{ + int print_type = RDMA_NLDEV_PRINT_TYPE_UNSPEC; + struct nlattr *tb_entry, *key = NULL, *val; + int type, cc = 0; + + if (!rd->show_driver_details || !tb) + return; + + if (rd->pretty_output) + newline_indent(rd); + + /* + * Driver attrs are tuples of {key, [print-type], value}. + * The key must be a string. If print-type is present, it + * defines an alternate printf format type vs the native format + * for the attribute. And the value can be any available + * driver type. + */ + mnl_attr_for_each_nested(tb_entry, tb) { + + if (cc > MAX_LINE_LENGTH) { + if (rd->pretty_output) + newline_indent(rd); + cc = 0; + } + if (rd_attr_check(tb_entry, &type) != MNL_CB_OK) + return; + if (!key) { + if (type != MNL_TYPE_NUL_STRING) + return; + key = tb_entry; + } else if (type == MNL_TYPE_U8) { + print_type = mnl_attr_get_u8(tb_entry); + } else { + val = tb_entry; + cc += print_driver_entry(rd, key, val, print_type); + if (cc < 0) + return; + print_type = RDMA_NLDEV_PRINT_TYPE_UNSPEC; + key = NULL; + } + } + return; +}
This enhancement allows printing rdma device-specific state, if provided by the kernel. This is done in a generic manner, so rdma tool doesn't need to know about the details of every type of rdma device. Driver attributes for a rdma resource are in the form of <key, [print_type], value> tuples, where the key is a string and the value can be any supported driver attribute. The print_type attribute, if present, provides a print format to use vs the standard print format for the type. For example, the default print type for a PROVIDER_S32 value is "%d ", but "0x%x " if the print_type of PRINT_TYPE_HEX is included inthe tuple. Driver resources are only printed when the -dd flag is present. If -p is present, then the output is formatted to not exceed 80 columns, otherwise it is printed as a single row to be grep/awk friendly. Example output: # rdma resource show qp lqpn 1028 -dd -p link cxgb4_0/- lqpn 1028 rqpn 0 type RC state RTS rq-psn 0 sq-psn 0 path-mig-state MIGRATED pid 0 comm [nvme_rdma] sqid 1028 flushed 0 memsize 123968 cidx 85 pidx 85 wq_pidx 106 flush_cidx 85 in_use 0 size 386 flags 0x0 rqid 1029 memsize 16768 cidx 43 pidx 41 wq_pidx 171 msn 44 rqt_hwaddr 0x2a8a5d00 rqt_size 256 in_use 128 size 130 idx 43 wr_id 0xffff881057c03408 idx 40 wr_id 0xffff881057c033f0 Signed-off-by: Steve Wise <swise@opengridcomputing.com> --- rdma/rdma.c | 7 ++- rdma/rdma.h | 11 ++++ rdma/res.c | 30 +++------ rdma/utils.c | 194 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 221 insertions(+), 21 deletions(-)