| Message ID | 8127af325344d0c2da8118d9074bfb50bca2c219.1521827521.git.swise@opengridcomputing.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Mar 23, 2018 at 09:49:39AM -0700, Steve Wise wrote: > Each provider can register a "fill entry" function with the restrack core. > This function will be called when filling out a resource, allowing the > provider to add provider-specific details. The details consist of a > table of nested attributes, that are in the form of "key, value" tuple. > The key nlattr must be strings, and the value nlattr can be one of > provider attributes that are generic, but typed, allowing the nlmessage > to ve validated. Currently the types include string, s32, u32, x32, s64, > u64, and x64. The inclusion of x, s, and u variants for numeric attributes > allows the user tool to print the number in the desired format. > > More attrs can be defined as they become needed by providers. > > Signed-off-by: Steve Wise <swise@opengridcomputing.com> > --- > drivers/infiniband/core/nldev.c | 39 +++++++++++++++++++++++++++++++++++++++ > include/rdma/restrack.h | 10 ++++++++++ > include/uapi/rdma/rdma_netlink.h | 16 ++++++++++++++++ > 3 files changed, 65 insertions(+) > > diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c > index 884843e..8346ede 100644 > --- a/drivers/infiniband/core/nldev.c > +++ b/drivers/infiniband/core/nldev.c > @@ -95,8 +95,27 @@ > [RDMA_NLDEV_ATTR_RES_PD_ENTRY] = { .type = NLA_NESTED }, > [RDMA_NLDEV_ATTR_RES_LOCAL_DMA_LKEY] = { .type = NLA_U32 }, > [RDMA_NLDEV_ATTR_RES_UNSAFE_GLOBAL_RKEY] = { .type = NLA_U32 }, > + [RDMA_NLDEV_ATTR_PROVIDER] = { .type = NLA_NESTED }, > + [RDMA_NLDEV_ATTR_PROVIDER_ENTRY] = { .type = NLA_NESTED }, > + [RDMA_NLDEV_ATTR_PROVIDER_STRING] = { .type = NLA_NUL_STRING, > + .len = RDMA_NLDEV_ATTR_PROVIDER_STRLEN }, > + [RDMA_NLDEV_ATTR_PROVIDER_D32] = { .type = NLA_S32 }, > + [RDMA_NLDEV_ATTR_PROVIDER_U32] = { .type = NLA_U32 }, > + [RDMA_NLDEV_ATTR_PROVIDER_X32] = { .type = NLA_U32 }, > + [RDMA_NLDEV_ATTR_PROVIDER_D64] = { .type = NLA_S64 }, > + [RDMA_NLDEV_ATTR_PROVIDER_U64] = { .type = NLA_U64 }, > + [RDMA_NLDEV_ATTR_PROVIDER_X64] = { .type = NLA_U64 }, > }; > > +static int provider_fill_res_entry(struct rdma_restrack_root *resroot, > + struct sk_buff *msg, > + struct netlink_callback *cb, > + struct rdma_restrack_entry *res) > +{ > + return resroot->fill_res_entry ? > + resroot->fill_res_entry(msg, cb, res) : 0; > +} Please add "fill_res_entry = NULL" line into rdma_restrack_init() despite kzalloc usage in ib_alloc_device(). And I afraid that we didn't settle the PROVIDER_*64 thing. Thanks
> > On Fri, Mar 23, 2018 at 09:49:39AM -0700, Steve Wise wrote: > > Each provider can register a "fill entry" function with the restrack core. > > This function will be called when filling out a resource, allowing the > > provider to add provider-specific details. The details consist of a > > table of nested attributes, that are in the form of "key, value" tuple. > > The key nlattr must be strings, and the value nlattr can be one of > > provider attributes that are generic, but typed, allowing the nlmessage > > to ve validated. Currently the types include string, s32, u32, x32, s64, > > u64, and x64. The inclusion of x, s, and u variants for numeric attributes > > allows the user tool to print the number in the desired format. > > > > More attrs can be defined as they become needed by providers. > > > > Signed-off-by: Steve Wise <swise@opengridcomputing.com> > > --- > > drivers/infiniband/core/nldev.c | 39 > +++++++++++++++++++++++++++++++++++++++ > > include/rdma/restrack.h | 10 ++++++++++ > > include/uapi/rdma/rdma_netlink.h | 16 ++++++++++++++++ > > 3 files changed, 65 insertions(+) > > > > diff --git a/drivers/infiniband/core/nldev.c > b/drivers/infiniband/core/nldev.c > > index 884843e..8346ede 100644 > > --- a/drivers/infiniband/core/nldev.c > > +++ b/drivers/infiniband/core/nldev.c > > @@ -95,8 +95,27 @@ > > [RDMA_NLDEV_ATTR_RES_PD_ENTRY] = { .type = > NLA_NESTED }, > > [RDMA_NLDEV_ATTR_RES_LOCAL_DMA_LKEY] = { .type = NLA_U32 }, > > [RDMA_NLDEV_ATTR_RES_UNSAFE_GLOBAL_RKEY] = { .type = > NLA_U32 }, > > + [RDMA_NLDEV_ATTR_PROVIDER] = { .type = > NLA_NESTED }, > > + [RDMA_NLDEV_ATTR_PROVIDER_ENTRY] = { .type = > NLA_NESTED }, > > + [RDMA_NLDEV_ATTR_PROVIDER_STRING] = { .type = > NLA_NUL_STRING, > > + .len = > RDMA_NLDEV_ATTR_PROVIDER_STRLEN }, > > + [RDMA_NLDEV_ATTR_PROVIDER_D32] = { .type = NLA_S32 }, > > + [RDMA_NLDEV_ATTR_PROVIDER_U32] = { .type = NLA_U32 }, > > + [RDMA_NLDEV_ATTR_PROVIDER_X32] = { .type = NLA_U32 }, > > + [RDMA_NLDEV_ATTR_PROVIDER_D64] = { .type = NLA_S64 }, > > + [RDMA_NLDEV_ATTR_PROVIDER_U64] = { .type = NLA_U64 }, > > + [RDMA_NLDEV_ATTR_PROVIDER_X64] = { .type = NLA_U64 }, > > }; > > > > +static int provider_fill_res_entry(struct rdma_restrack_root *resroot, > > + struct sk_buff *msg, > > + struct netlink_callback *cb, > > + struct rdma_restrack_entry *res) > > +{ > > + return resroot->fill_res_entry ? > > + resroot->fill_res_entry(msg, cb, res) : 0; > > +} > > Please add "fill_res_entry = NULL" line into rdma_restrack_init() > despite kzalloc usage in ib_alloc_device(). Will do. > > And I afraid that we didn't settle the PROVIDER_*64 thing. > I didn't agree that your proposal was simpler, or even avoided the issues you said were problems with the self-describing-print-formt attributes. I asked if anyone else had an opinion. Nobody replied. So I chose to keep the attributes as they are. Are you NAKing this? Jason and Doug, do you have an opinion either way? Our discussion of this can be found here: https://www.spinics.net/lists/linux-rdma/msg62198.html Steve 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 Sat, Mar 24, 2018 at 02:42:15PM -0500, Steve Wise wrote: > > > > > On Fri, Mar 23, 2018 at 09:49:39AM -0700, Steve Wise wrote: > > > Each provider can register a "fill entry" function with the restrack > core. > > > This function will be called when filling out a resource, allowing the > > > provider to add provider-specific details. The details consist of a > > > table of nested attributes, that are in the form of "key, value" tuple. > > > The key nlattr must be strings, and the value nlattr can be one of > > > provider attributes that are generic, but typed, allowing the nlmessage > > > to ve validated. Currently the types include string, s32, u32, x32, > s64, > > > u64, and x64. The inclusion of x, s, and u variants for numeric > attributes > > > allows the user tool to print the number in the desired format. > > > > > > More attrs can be defined as they become needed by providers. > > > > > > Signed-off-by: Steve Wise <swise@opengridcomputing.com> > > > --- > > > drivers/infiniband/core/nldev.c | 39 > > +++++++++++++++++++++++++++++++++++++++ > > > include/rdma/restrack.h | 10 ++++++++++ > > > include/uapi/rdma/rdma_netlink.h | 16 ++++++++++++++++ > > > 3 files changed, 65 insertions(+) > > > > > > diff --git a/drivers/infiniband/core/nldev.c > > b/drivers/infiniband/core/nldev.c > > > index 884843e..8346ede 100644 > > > --- a/drivers/infiniband/core/nldev.c > > > +++ b/drivers/infiniband/core/nldev.c > > > @@ -95,8 +95,27 @@ > > > [RDMA_NLDEV_ATTR_RES_PD_ENTRY] = { .type = > > NLA_NESTED }, > > > [RDMA_NLDEV_ATTR_RES_LOCAL_DMA_LKEY] = { .type = NLA_U32 }, > > > [RDMA_NLDEV_ATTR_RES_UNSAFE_GLOBAL_RKEY] = { .type = > > NLA_U32 }, > > > + [RDMA_NLDEV_ATTR_PROVIDER] = { .type = > > NLA_NESTED }, > > > + [RDMA_NLDEV_ATTR_PROVIDER_ENTRY] = { .type = > > NLA_NESTED }, > > > + [RDMA_NLDEV_ATTR_PROVIDER_STRING] = { .type = > > NLA_NUL_STRING, > > > + .len = > > RDMA_NLDEV_ATTR_PROVIDER_STRLEN }, > > > + [RDMA_NLDEV_ATTR_PROVIDER_D32] = { .type = NLA_S32 }, > > > + [RDMA_NLDEV_ATTR_PROVIDER_U32] = { .type = NLA_U32 }, > > > + [RDMA_NLDEV_ATTR_PROVIDER_X32] = { .type = NLA_U32 }, > > > + [RDMA_NLDEV_ATTR_PROVIDER_D64] = { .type = NLA_S64 }, > > > + [RDMA_NLDEV_ATTR_PROVIDER_U64] = { .type = NLA_U64 }, > > > + [RDMA_NLDEV_ATTR_PROVIDER_X64] = { .type = NLA_U64 }, > > > }; > > > > > > +static int provider_fill_res_entry(struct rdma_restrack_root *resroot, > > > + struct sk_buff *msg, > > > + struct netlink_callback *cb, > > > + struct rdma_restrack_entry *res) > > > +{ > > > + return resroot->fill_res_entry ? > > > + resroot->fill_res_entry(msg, cb, res) : 0; > > > +} > > > > Please add "fill_res_entry = NULL" line into rdma_restrack_init() > > despite kzalloc usage in ib_alloc_device(). > > > Will do. > > > > > And I afraid that we didn't settle the PROVIDER_*64 thing. > > > > I didn't agree that your proposal was simpler, or even avoided the issues > you said were problems with the self-describing-print-formt attributes. I > asked if anyone else had an opinion. Nobody replied. So I chose to keep > the attributes as they are. Are you NAKing this? Sure not, I'm NAKing only if the code is wrong and/or breaks something. It is not the case here, the code is correct and the fact that I don't like your implementation doesn't give me any reason to use "NAK" option. > > Jason and Doug, do you have an opinion either way? Our discussion of this > can be found here: > > https://www.spinics.net/lists/linux-rdma/msg62198.html > > Steve > > Steve. >
On Fri, Mar 23, 2018 at 09:49:39AM -0700, Steve Wise wrote: > Each provider can register a "fill entry" function with the restrack core. > This function will be called when filling out a resource, allowing the > provider to add provider-specific details. The details consist of a > table of nested attributes, that are in the form of "key, value" tuple. > The key nlattr must be strings, and the value nlattr can be one of > provider attributes that are generic, but typed, allowing the nlmessage > to ve validated. Currently the types include string, s32, u32, x32, s64, > u64, and x64. The inclusion of x, s, and u variants for numeric attributes > allows the user tool to print the number in the desired format. > > More attrs can be defined as they become needed by providers. > > Signed-off-by: Steve Wise <swise@opengridcomputing.com> > --- > drivers/infiniband/core/nldev.c | 39 +++++++++++++++++++++++++++++++++++++++ > include/rdma/restrack.h | 10 ++++++++++ > include/uapi/rdma/rdma_netlink.h | 16 ++++++++++++++++ > 3 files changed, 65 insertions(+) > > diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c > index 884843e..8346ede 100644 > --- a/drivers/infiniband/core/nldev.c > +++ b/drivers/infiniband/core/nldev.c > @@ -95,8 +95,27 @@ > [RDMA_NLDEV_ATTR_RES_PD_ENTRY] = { .type = NLA_NESTED }, > [RDMA_NLDEV_ATTR_RES_LOCAL_DMA_LKEY] = { .type = NLA_U32 }, > [RDMA_NLDEV_ATTR_RES_UNSAFE_GLOBAL_RKEY] = { .type = NLA_U32 }, > + [RDMA_NLDEV_ATTR_PROVIDER] = { .type = NLA_NESTED }, > + [RDMA_NLDEV_ATTR_PROVIDER_ENTRY] = { .type = NLA_NESTED }, > + [RDMA_NLDEV_ATTR_PROVIDER_STRING] = { .type = NLA_NUL_STRING, > + .len = RDMA_NLDEV_ATTR_PROVIDER_STRLEN }, > + [RDMA_NLDEV_ATTR_PROVIDER_D32] = { .type = NLA_S32 }, > + [RDMA_NLDEV_ATTR_PROVIDER_U32] = { .type = NLA_U32 }, > + [RDMA_NLDEV_ATTR_PROVIDER_X32] = { .type = NLA_U32 }, > + [RDMA_NLDEV_ATTR_PROVIDER_D64] = { .type = NLA_S64 }, > + [RDMA_NLDEV_ATTR_PROVIDER_U64] = { .type = NLA_U64 }, > + [RDMA_NLDEV_ATTR_PROVIDER_X64] = { .type = NLA_U64 }, > }; > > +static int provider_fill_res_entry(struct rdma_restrack_root *resroot, > + struct sk_buff *msg, > + struct netlink_callback *cb, > + struct rdma_restrack_entry *res) > +{ > + return resroot->fill_res_entry ? > + resroot->fill_res_entry(msg, cb, res) : 0; > +} > + > static int fill_nldev_handle(struct sk_buff *msg, struct ib_device *device) > { > if (nla_put_u32(msg, RDMA_NLDEV_ATTR_DEV_INDEX, device->index)) > @@ -264,6 +283,7 @@ static int fill_res_qp_entry(struct sk_buff *msg, struct netlink_callback *cb, > struct rdma_restrack_entry *res, uint32_t port) > { > struct ib_qp *qp = container_of(res, struct ib_qp, res); > + struct rdma_restrack_root *resroot = &qp->device->res; > struct ib_qp_init_attr qp_init_attr; > struct nlattr *entry_attr; > struct ib_qp_attr qp_attr; > @@ -313,6 +333,9 @@ static int fill_res_qp_entry(struct sk_buff *msg, struct netlink_callback *cb, > if (fill_res_name_pid(msg, res)) > goto err; > > + if (provider_fill_res_entry(resroot, msg, cb, res)) > + goto err; > + > nla_nest_end(msg, entry_attr); > return 0; > > @@ -328,6 +351,7 @@ static int fill_res_cm_id_entry(struct sk_buff *msg, > { > struct rdma_id_private *id_priv = > container_of(res, struct rdma_id_private, res); > + struct rdma_restrack_root *resroot = &id_priv->id.device->res; > struct rdma_cm_id *cm_id = &id_priv->id; > struct nlattr *entry_attr; > > @@ -369,6 +393,9 @@ static int fill_res_cm_id_entry(struct sk_buff *msg, > if (fill_res_name_pid(msg, res)) > goto err; > > + if (provider_fill_res_entry(resroot, msg, cb, res)) > + goto err; > + > nla_nest_end(msg, entry_attr); > return 0; > > @@ -382,6 +409,7 @@ static int fill_res_cq_entry(struct sk_buff *msg, struct netlink_callback *cb, > struct rdma_restrack_entry *res, uint32_t port) > { > struct ib_cq *cq = container_of(res, struct ib_cq, res); > + struct rdma_restrack_root *resroot = &cq->device->res; > struct nlattr *entry_attr; > > entry_attr = nla_nest_start(msg, RDMA_NLDEV_ATTR_RES_CQ_ENTRY); > @@ -402,6 +430,9 @@ static int fill_res_cq_entry(struct sk_buff *msg, struct netlink_callback *cb, > if (fill_res_name_pid(msg, res)) > goto err; > > + if (provider_fill_res_entry(resroot, msg, cb, res)) > + goto err; > + > nla_nest_end(msg, entry_attr); > return 0; > > @@ -415,6 +446,7 @@ static int fill_res_mr_entry(struct sk_buff *msg, struct netlink_callback *cb, > struct rdma_restrack_entry *res, uint32_t port) > { > struct ib_mr *mr = container_of(res, struct ib_mr, res); > + struct rdma_restrack_root *resroot = &mr->pd->device->res; > struct nlattr *entry_attr; > > entry_attr = nla_nest_start(msg, RDMA_NLDEV_ATTR_RES_MR_ENTRY); > @@ -438,6 +470,9 @@ static int fill_res_mr_entry(struct sk_buff *msg, struct netlink_callback *cb, > if (fill_res_name_pid(msg, res)) > goto err; > > + if (provider_fill_res_entry(resroot, msg, cb, res)) > + goto err; > + > nla_nest_end(msg, entry_attr); > return 0; > > @@ -451,6 +486,7 @@ static int fill_res_pd_entry(struct sk_buff *msg, struct netlink_callback *cb, > struct rdma_restrack_entry *res, uint32_t port) > { > struct ib_pd *pd = container_of(res, struct ib_pd, res); > + struct rdma_restrack_root *resroot = &pd->device->res; > struct nlattr *entry_attr; > > entry_attr = nla_nest_start(msg, RDMA_NLDEV_ATTR_RES_PD_ENTRY); > @@ -477,6 +513,9 @@ static int fill_res_pd_entry(struct sk_buff *msg, struct netlink_callback *cb, > if (fill_res_name_pid(msg, res)) > goto err; > > + if (provider_fill_res_entry(resroot, msg, cb, res)) > + goto err; > + > nla_nest_end(msg, entry_attr); > return 0; > > diff --git a/include/rdma/restrack.h b/include/rdma/restrack.h > index f3b3e35..bd3cd9a 100644 > --- a/include/rdma/restrack.h > +++ b/include/rdma/restrack.h > @@ -44,6 +44,8 @@ enum rdma_restrack_type { > }; > > #define RDMA_RESTRACK_HASH_BITS 8 > +struct rdma_restrack_entry; > + > /** > * struct rdma_restrack_root - main resource tracking management > * entity, per-device > @@ -57,6 +59,14 @@ struct rdma_restrack_root { > * @hash: global database for all resources per-device > */ > DECLARE_HASHTABLE(hash, RDMA_RESTRACK_HASH_BITS); > + /** > + * @fill_res_entry: provider-specific fill function > + * > + * Allows rdma providers to add their own restrack attributes. > + */ > + int (*fill_res_entry)(struct sk_buff *msg, > + struct netlink_callback *cb, > + struct rdma_restrack_entry *entry); > }; > > /** > diff --git a/include/uapi/rdma/rdma_netlink.h b/include/uapi/rdma/rdma_netlink.h > index 84b3f63..70443d4 100644 > --- a/include/uapi/rdma/rdma_netlink.h > +++ b/include/uapi/rdma/rdma_netlink.h > @@ -249,6 +249,10 @@ enum rdma_nldev_command { > RDMA_NLDEV_NUM_OPS > }; > > +enum { > + RDMA_NLDEV_ATTR_PROVIDER_STRLEN = 16, > +}; Let's call it to be something more generic, because we used hardcoded value for RDMA_NLDEV_ATTR_RES_SUMMARY_ENTRY_NAME. What about RDMA_NLDEV_ATTR_ENTRY_STRLEN? Thanks
> > On Fri, Mar 23, 2018 at 09:49:39AM -0700, Steve Wise wrote: > > Each provider can register a "fill entry" function with the restrack core. > > This function will be called when filling out a resource, allowing the > > provider to add provider-specific details. The details consist of a > > table of nested attributes, that are in the form of "key, value" tuple. > > The key nlattr must be strings, and the value nlattr can be one of > > provider attributes that are generic, but typed, allowing the nlmessage > > to ve validated. Currently the types include string, s32, u32, x32, s64, > > u64, and x64. The inclusion of x, s, and u variants for numeric attributes > > allows the user tool to print the number in the desired format. > > > > More attrs can be defined as they become needed by providers. > > > > Signed-off-by: Steve Wise <swise@opengridcomputing.com> > > --- > > drivers/infiniband/core/nldev.c | 39 > +++++++++++++++++++++++++++++++++++++++ > > include/rdma/restrack.h | 10 ++++++++++ > > include/uapi/rdma/rdma_netlink.h | 16 ++++++++++++++++ > > 3 files changed, 65 insertions(+) > > > > diff --git a/drivers/infiniband/core/nldev.c > b/drivers/infiniband/core/nldev.c > > index 884843e..8346ede 100644 > > --- a/drivers/infiniband/core/nldev.c > > +++ b/drivers/infiniband/core/nldev.c > > @@ -95,8 +95,27 @@ > > [RDMA_NLDEV_ATTR_RES_PD_ENTRY] = { .type = > NLA_NESTED }, > > [RDMA_NLDEV_ATTR_RES_LOCAL_DMA_LKEY] = { .type = NLA_U32 }, > > [RDMA_NLDEV_ATTR_RES_UNSAFE_GLOBAL_RKEY] = { .type = > NLA_U32 }, > > + [RDMA_NLDEV_ATTR_PROVIDER] = { .type = > NLA_NESTED }, > > + [RDMA_NLDEV_ATTR_PROVIDER_ENTRY] = { .type = > NLA_NESTED }, > > + [RDMA_NLDEV_ATTR_PROVIDER_STRING] = { .type = > NLA_NUL_STRING, > > + .len = > RDMA_NLDEV_ATTR_PROVIDER_STRLEN }, > > + [RDMA_NLDEV_ATTR_PROVIDER_D32] = { .type = NLA_S32 }, > > + [RDMA_NLDEV_ATTR_PROVIDER_U32] = { .type = NLA_U32 }, > > + [RDMA_NLDEV_ATTR_PROVIDER_X32] = { .type = NLA_U32 }, > > + [RDMA_NLDEV_ATTR_PROVIDER_D64] = { .type = NLA_S64 }, > > + [RDMA_NLDEV_ATTR_PROVIDER_U64] = { .type = NLA_U64 }, > > + [RDMA_NLDEV_ATTR_PROVIDER_X64] = { .type = NLA_U64 }, > > }; > > > > +static int provider_fill_res_entry(struct rdma_restrack_root *resroot, > > + struct sk_buff *msg, > > + struct netlink_callback *cb, > > + struct rdma_restrack_entry *res) > > +{ > > + return resroot->fill_res_entry ? > > + resroot->fill_res_entry(msg, cb, res) : 0; > > +} > > + > > static int fill_nldev_handle(struct sk_buff *msg, struct ib_device *device) > > { > > if (nla_put_u32(msg, RDMA_NLDEV_ATTR_DEV_INDEX, device- > >index)) > > @@ -264,6 +283,7 @@ static int fill_res_qp_entry(struct sk_buff *msg, > struct netlink_callback *cb, > > struct rdma_restrack_entry *res, uint32_t port) > > { > > struct ib_qp *qp = container_of(res, struct ib_qp, res); > > + struct rdma_restrack_root *resroot = &qp->device->res; > > struct ib_qp_init_attr qp_init_attr; > > struct nlattr *entry_attr; > > struct ib_qp_attr qp_attr; > > @@ -313,6 +333,9 @@ static int fill_res_qp_entry(struct sk_buff *msg, > struct netlink_callback *cb, > > if (fill_res_name_pid(msg, res)) > > goto err; > > > > + if (provider_fill_res_entry(resroot, msg, cb, res)) > > + goto err; > > + > > nla_nest_end(msg, entry_attr); > > return 0; > > > > @@ -328,6 +351,7 @@ static int fill_res_cm_id_entry(struct sk_buff *msg, > > { > > struct rdma_id_private *id_priv = > > container_of(res, struct rdma_id_private, > res); > > + struct rdma_restrack_root *resroot = &id_priv->id.device->res; > > struct rdma_cm_id *cm_id = &id_priv->id; > > struct nlattr *entry_attr; > > > > @@ -369,6 +393,9 @@ static int fill_res_cm_id_entry(struct sk_buff *msg, > > if (fill_res_name_pid(msg, res)) > > goto err; > > > > + if (provider_fill_res_entry(resroot, msg, cb, res)) > > + goto err; > > + > > nla_nest_end(msg, entry_attr); > > return 0; > > > > @@ -382,6 +409,7 @@ static int fill_res_cq_entry(struct sk_buff *msg, > struct netlink_callback *cb, > > struct rdma_restrack_entry *res, uint32_t port) > > { > > struct ib_cq *cq = container_of(res, struct ib_cq, res); > > + struct rdma_restrack_root *resroot = &cq->device->res; > > struct nlattr *entry_attr; > > > > entry_attr = nla_nest_start(msg, > RDMA_NLDEV_ATTR_RES_CQ_ENTRY); > > @@ -402,6 +430,9 @@ static int fill_res_cq_entry(struct sk_buff *msg, > struct netlink_callback *cb, > > if (fill_res_name_pid(msg, res)) > > goto err; > > > > + if (provider_fill_res_entry(resroot, msg, cb, res)) > > + goto err; > > + > > nla_nest_end(msg, entry_attr); > > return 0; > > > > @@ -415,6 +446,7 @@ static int fill_res_mr_entry(struct sk_buff *msg, > struct netlink_callback *cb, > > struct rdma_restrack_entry *res, uint32_t port) > > { > > struct ib_mr *mr = container_of(res, struct ib_mr, res); > > + struct rdma_restrack_root *resroot = &mr->pd->device->res; > > struct nlattr *entry_attr; > > > > entry_attr = nla_nest_start(msg, > RDMA_NLDEV_ATTR_RES_MR_ENTRY); > > @@ -438,6 +470,9 @@ static int fill_res_mr_entry(struct sk_buff *msg, > struct netlink_callback *cb, > > if (fill_res_name_pid(msg, res)) > > goto err; > > > > + if (provider_fill_res_entry(resroot, msg, cb, res)) > > + goto err; > > + > > nla_nest_end(msg, entry_attr); > > return 0; > > > > @@ -451,6 +486,7 @@ static int fill_res_pd_entry(struct sk_buff *msg, > struct netlink_callback *cb, > > struct rdma_restrack_entry *res, uint32_t port) > > { > > struct ib_pd *pd = container_of(res, struct ib_pd, res); > > + struct rdma_restrack_root *resroot = &pd->device->res; > > struct nlattr *entry_attr; > > > > entry_attr = nla_nest_start(msg, > RDMA_NLDEV_ATTR_RES_PD_ENTRY); > > @@ -477,6 +513,9 @@ static int fill_res_pd_entry(struct sk_buff *msg, > struct netlink_callback *cb, > > if (fill_res_name_pid(msg, res)) > > goto err; > > > > + if (provider_fill_res_entry(resroot, msg, cb, res)) > > + goto err; > > + > > nla_nest_end(msg, entry_attr); > > return 0; > > > > diff --git a/include/rdma/restrack.h b/include/rdma/restrack.h > > index f3b3e35..bd3cd9a 100644 > > --- a/include/rdma/restrack.h > > +++ b/include/rdma/restrack.h > > @@ -44,6 +44,8 @@ enum rdma_restrack_type { > > }; > > > > #define RDMA_RESTRACK_HASH_BITS 8 > > +struct rdma_restrack_entry; > > + > > /** > > * struct rdma_restrack_root - main resource tracking management > > * entity, per-device > > @@ -57,6 +59,14 @@ struct rdma_restrack_root { > > * @hash: global database for all resources per-device > > */ > > DECLARE_HASHTABLE(hash, RDMA_RESTRACK_HASH_BITS); > > + /** > > + * @fill_res_entry: provider-specific fill function > > + * > > + * Allows rdma providers to add their own restrack attributes. > > + */ > > + int (*fill_res_entry)(struct sk_buff *msg, > > + struct netlink_callback *cb, > > + struct rdma_restrack_entry *entry); > > }; > > > > /** > > diff --git a/include/uapi/rdma/rdma_netlink.h > b/include/uapi/rdma/rdma_netlink.h > > index 84b3f63..70443d4 100644 > > --- a/include/uapi/rdma/rdma_netlink.h > > +++ b/include/uapi/rdma/rdma_netlink.h > > @@ -249,6 +249,10 @@ enum rdma_nldev_command { > > RDMA_NLDEV_NUM_OPS > > }; > > > > +enum { > > + RDMA_NLDEV_ATTR_PROVIDER_STRLEN = 16, > > +}; > > Let's call it to be something more generic, because we used > hardcoded value for RDMA_NLDEV_ATTR_RES_SUMMARY_ENTRY_NAME. > > What about RDMA_NLDEV_ATTR_ENTRY_STRLEN? > Ok. Is really 16 big enough? -- 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 Sun, Mar 25, 2018 at 08:53:49AM -0500, Steve Wise wrote: > > > > On Fri, Mar 23, 2018 at 09:49:39AM -0700, Steve Wise wrote: > > > Each provider can register a "fill entry" function with the restrack > core. > > > This function will be called when filling out a resource, allowing the > > > provider to add provider-specific details. The details consist of a > > > table of nested attributes, that are in the form of "key, value" tuple. > > > The key nlattr must be strings, and the value nlattr can be one of > > > provider attributes that are generic, but typed, allowing the nlmessage > > > to ve validated. Currently the types include string, s32, u32, x32, > s64, > > > u64, and x64. The inclusion of x, s, and u variants for numeric > attributes > > > allows the user tool to print the number in the desired format. > > > > > > More attrs can be defined as they become needed by providers. > > > > > > Signed-off-by: Steve Wise <swise@opengridcomputing.com> > > > --- > > > drivers/infiniband/core/nldev.c | 39 > > +++++++++++++++++++++++++++++++++++++++ > > > include/rdma/restrack.h | 10 ++++++++++ > > > include/uapi/rdma/rdma_netlink.h | 16 ++++++++++++++++ > > > 3 files changed, 65 insertions(+) > > > > > > diff --git a/drivers/infiniband/core/nldev.c > > b/drivers/infiniband/core/nldev.c > > > index 884843e..8346ede 100644 > > > --- a/drivers/infiniband/core/nldev.c > > > +++ b/drivers/infiniband/core/nldev.c > > > @@ -95,8 +95,27 @@ > > > [RDMA_NLDEV_ATTR_RES_PD_ENTRY] = { .type = > > NLA_NESTED }, > > > [RDMA_NLDEV_ATTR_RES_LOCAL_DMA_LKEY] = { .type = NLA_U32 }, > > > [RDMA_NLDEV_ATTR_RES_UNSAFE_GLOBAL_RKEY] = { .type = > > NLA_U32 }, > > > + [RDMA_NLDEV_ATTR_PROVIDER] = { .type = > > NLA_NESTED }, > > > + [RDMA_NLDEV_ATTR_PROVIDER_ENTRY] = { .type = > > NLA_NESTED }, > > > + [RDMA_NLDEV_ATTR_PROVIDER_STRING] = { .type = > > NLA_NUL_STRING, > > > + .len = > > RDMA_NLDEV_ATTR_PROVIDER_STRLEN }, > > > + [RDMA_NLDEV_ATTR_PROVIDER_D32] = { .type = NLA_S32 }, > > > + [RDMA_NLDEV_ATTR_PROVIDER_U32] = { .type = NLA_U32 }, > > > + [RDMA_NLDEV_ATTR_PROVIDER_X32] = { .type = NLA_U32 }, > > > + [RDMA_NLDEV_ATTR_PROVIDER_D64] = { .type = NLA_S64 }, > > > + [RDMA_NLDEV_ATTR_PROVIDER_U64] = { .type = NLA_U64 }, > > > + [RDMA_NLDEV_ATTR_PROVIDER_X64] = { .type = NLA_U64 }, > > > }; > > > > > > +static int provider_fill_res_entry(struct rdma_restrack_root *resroot, > > > + struct sk_buff *msg, > > > + struct netlink_callback *cb, > > > + struct rdma_restrack_entry *res) > > > +{ > > > + return resroot->fill_res_entry ? > > > + resroot->fill_res_entry(msg, cb, res) : 0; > > > +} > > > + > > > static int fill_nldev_handle(struct sk_buff *msg, struct ib_device > *device) > > > { > > > if (nla_put_u32(msg, RDMA_NLDEV_ATTR_DEV_INDEX, device- > > >index)) > > > @@ -264,6 +283,7 @@ static int fill_res_qp_entry(struct sk_buff *msg, > > struct netlink_callback *cb, > > > struct rdma_restrack_entry *res, uint32_t port) > > > { > > > struct ib_qp *qp = container_of(res, struct ib_qp, res); > > > + struct rdma_restrack_root *resroot = &qp->device->res; > > > struct ib_qp_init_attr qp_init_attr; > > > struct nlattr *entry_attr; > > > struct ib_qp_attr qp_attr; > > > @@ -313,6 +333,9 @@ static int fill_res_qp_entry(struct sk_buff *msg, > > struct netlink_callback *cb, > > > if (fill_res_name_pid(msg, res)) > > > goto err; > > > > > > + if (provider_fill_res_entry(resroot, msg, cb, res)) > > > + goto err; > > > + > > > nla_nest_end(msg, entry_attr); > > > return 0; > > > > > > @@ -328,6 +351,7 @@ static int fill_res_cm_id_entry(struct sk_buff *msg, > > > { > > > struct rdma_id_private *id_priv = > > > container_of(res, struct rdma_id_private, > > res); > > > + struct rdma_restrack_root *resroot = &id_priv->id.device->res; > > > struct rdma_cm_id *cm_id = &id_priv->id; > > > struct nlattr *entry_attr; > > > > > > @@ -369,6 +393,9 @@ static int fill_res_cm_id_entry(struct sk_buff *msg, > > > if (fill_res_name_pid(msg, res)) > > > goto err; > > > > > > + if (provider_fill_res_entry(resroot, msg, cb, res)) > > > + goto err; > > > + > > > nla_nest_end(msg, entry_attr); > > > return 0; > > > > > > @@ -382,6 +409,7 @@ static int fill_res_cq_entry(struct sk_buff *msg, > > struct netlink_callback *cb, > > > struct rdma_restrack_entry *res, uint32_t port) > > > { > > > struct ib_cq *cq = container_of(res, struct ib_cq, res); > > > + struct rdma_restrack_root *resroot = &cq->device->res; > > > struct nlattr *entry_attr; > > > > > > entry_attr = nla_nest_start(msg, > > RDMA_NLDEV_ATTR_RES_CQ_ENTRY); > > > @@ -402,6 +430,9 @@ static int fill_res_cq_entry(struct sk_buff *msg, > > struct netlink_callback *cb, > > > if (fill_res_name_pid(msg, res)) > > > goto err; > > > > > > + if (provider_fill_res_entry(resroot, msg, cb, res)) > > > + goto err; > > > + > > > nla_nest_end(msg, entry_attr); > > > return 0; > > > > > > @@ -415,6 +446,7 @@ static int fill_res_mr_entry(struct sk_buff *msg, > > struct netlink_callback *cb, > > > struct rdma_restrack_entry *res, uint32_t port) > > > { > > > struct ib_mr *mr = container_of(res, struct ib_mr, res); > > > + struct rdma_restrack_root *resroot = &mr->pd->device->res; > > > struct nlattr *entry_attr; > > > > > > entry_attr = nla_nest_start(msg, > > RDMA_NLDEV_ATTR_RES_MR_ENTRY); > > > @@ -438,6 +470,9 @@ static int fill_res_mr_entry(struct sk_buff *msg, > > struct netlink_callback *cb, > > > if (fill_res_name_pid(msg, res)) > > > goto err; > > > > > > + if (provider_fill_res_entry(resroot, msg, cb, res)) > > > + goto err; > > > + > > > nla_nest_end(msg, entry_attr); > > > return 0; > > > > > > @@ -451,6 +486,7 @@ static int fill_res_pd_entry(struct sk_buff *msg, > > struct netlink_callback *cb, > > > struct rdma_restrack_entry *res, uint32_t port) > > > { > > > struct ib_pd *pd = container_of(res, struct ib_pd, res); > > > + struct rdma_restrack_root *resroot = &pd->device->res; > > > struct nlattr *entry_attr; > > > > > > entry_attr = nla_nest_start(msg, > > RDMA_NLDEV_ATTR_RES_PD_ENTRY); > > > @@ -477,6 +513,9 @@ static int fill_res_pd_entry(struct sk_buff *msg, > > struct netlink_callback *cb, > > > if (fill_res_name_pid(msg, res)) > > > goto err; > > > > > > + if (provider_fill_res_entry(resroot, msg, cb, res)) > > > + goto err; > > > + > > > nla_nest_end(msg, entry_attr); > > > return 0; > > > > > > diff --git a/include/rdma/restrack.h b/include/rdma/restrack.h > > > index f3b3e35..bd3cd9a 100644 > > > --- a/include/rdma/restrack.h > > > +++ b/include/rdma/restrack.h > > > @@ -44,6 +44,8 @@ enum rdma_restrack_type { > > > }; > > > > > > #define RDMA_RESTRACK_HASH_BITS 8 > > > +struct rdma_restrack_entry; > > > + > > > /** > > > * struct rdma_restrack_root - main resource tracking management > > > * entity, per-device > > > @@ -57,6 +59,14 @@ struct rdma_restrack_root { > > > * @hash: global database for all resources per-device > > > */ > > > DECLARE_HASHTABLE(hash, RDMA_RESTRACK_HASH_BITS); > > > + /** > > > + * @fill_res_entry: provider-specific fill function > > > + * > > > + * Allows rdma providers to add their own restrack attributes. > > > + */ > > > + int (*fill_res_entry)(struct sk_buff *msg, > > > + struct netlink_callback *cb, > > > + struct rdma_restrack_entry *entry); > > > }; > > > > > > /** > > > diff --git a/include/uapi/rdma/rdma_netlink.h > > b/include/uapi/rdma/rdma_netlink.h > > > index 84b3f63..70443d4 100644 > > > --- a/include/uapi/rdma/rdma_netlink.h > > > +++ b/include/uapi/rdma/rdma_netlink.h > > > @@ -249,6 +249,10 @@ enum rdma_nldev_command { > > > RDMA_NLDEV_NUM_OPS > > > }; > > > > > > +enum { > > > + RDMA_NLDEV_ATTR_PROVIDER_STRLEN = 16, > > > +}; > > > > Let's call it to be something more generic, because we used > > hardcoded value for RDMA_NLDEV_ATTR_RES_SUMMARY_ENTRY_NAME. > > > > What about RDMA_NLDEV_ATTR_ENTRY_STRLEN? > > > > Ok. Is really 16 big enough? I think so, let's save skb size. 16 is "abcdefghijklmno" (15 chars + NULL). Thanks
On Sat, Mar 24, 2018 at 02:42:15PM -0500, Steve Wise wrote: > > > > > On Fri, Mar 23, 2018 at 09:49:39AM -0700, Steve Wise wrote: > > > Each provider can register a "fill entry" function with the restrack > core. > > > This function will be called when filling out a resource, allowing the > > > provider to add provider-specific details. The details consist of a > > > table of nested attributes, that are in the form of "key, value" tuple. > > > The key nlattr must be strings, and the value nlattr can be one of > > > provider attributes that are generic, but typed, allowing the nlmessage > > > to ve validated. Currently the types include string, s32, u32, x32, > s64, > > > u64, and x64. The inclusion of x, s, and u variants for numeric > attributes > > > allows the user tool to print the number in the desired format. > > > > > > More attrs can be defined as they become needed by providers. > > > > > > Signed-off-by: Steve Wise <swise@opengridcomputing.com> > > > drivers/infiniband/core/nldev.c | 39 > > +++++++++++++++++++++++++++++++++++++++ > > > include/rdma/restrack.h | 10 ++++++++++ > > > include/uapi/rdma/rdma_netlink.h | 16 ++++++++++++++++ > > > 3 files changed, 65 insertions(+) > > > > > > diff --git a/drivers/infiniband/core/nldev.c > > b/drivers/infiniband/core/nldev.c > > > index 884843e..8346ede 100644 > > > +++ b/drivers/infiniband/core/nldev.c > > > @@ -95,8 +95,27 @@ > > > [RDMA_NLDEV_ATTR_RES_PD_ENTRY] = { .type = > > NLA_NESTED }, > > > [RDMA_NLDEV_ATTR_RES_LOCAL_DMA_LKEY] = { .type = NLA_U32 }, > > > [RDMA_NLDEV_ATTR_RES_UNSAFE_GLOBAL_RKEY] = { .type = > > NLA_U32 }, > > > + [RDMA_NLDEV_ATTR_PROVIDER] = { .type = > > NLA_NESTED }, > > > + [RDMA_NLDEV_ATTR_PROVIDER_ENTRY] = { .type = > > NLA_NESTED }, > > > + [RDMA_NLDEV_ATTR_PROVIDER_STRING] = { .type = > > NLA_NUL_STRING, > > > + .len = > > RDMA_NLDEV_ATTR_PROVIDER_STRLEN }, > > > + [RDMA_NLDEV_ATTR_PROVIDER_D32] = { .type = NLA_S32 }, > > > + [RDMA_NLDEV_ATTR_PROVIDER_U32] = { .type = NLA_U32 }, > > > + [RDMA_NLDEV_ATTR_PROVIDER_X32] = { .type = NLA_U32 }, > > > + [RDMA_NLDEV_ATTR_PROVIDER_D64] = { .type = NLA_S64 }, > > > + [RDMA_NLDEV_ATTR_PROVIDER_U64] = { .type = NLA_U64 }, > > > + [RDMA_NLDEV_ATTR_PROVIDER_X64] = { .type = NLA_U64 }, > > > }; > > > > > > +static int provider_fill_res_entry(struct rdma_restrack_root *resroot, > > > + struct sk_buff *msg, > > > + struct netlink_callback *cb, > > > + struct rdma_restrack_entry *res) > > > +{ > > > + return resroot->fill_res_entry ? > > > + resroot->fill_res_entry(msg, cb, res) : 0; > > > +} > > > > Please add "fill_res_entry = NULL" line into rdma_restrack_init() > > despite kzalloc usage in ib_alloc_device(). > > > Will do. > > > > > And I afraid that we didn't settle the PROVIDER_*64 thing. > > > > I didn't agree that your proposal was simpler, or even avoided the issues > you said were problems with the self-describing-print-formt attributes. I > asked if anyone else had an opinion. Nobody replied. So I chose to keep > the attributes as they are. Are you NAKing this? > > Jason and Doug, do you have an opinion either way? Our discussion of this > can be found here: > > https://www.spinics.net/lists/linux-rdma/msg62198.html I'm still not sure what you are two are arguing about.. Is it the inclusion of 'X'? That does seem weird. 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 Tue, Mar 27, 2018 at 08:50:22AM -0600, Jason Gunthorpe wrote: > On Sat, Mar 24, 2018 at 02:42:15PM -0500, Steve Wise wrote: > > > > > > > > On Fri, Mar 23, 2018 at 09:49:39AM -0700, Steve Wise wrote: > > > > Each provider can register a "fill entry" function with the restrack > > core. > > > > This function will be called when filling out a resource, allowing the > > > > provider to add provider-specific details. The details consist of a > > > > table of nested attributes, that are in the form of "key, value" tuple. > > > > The key nlattr must be strings, and the value nlattr can be one of > > > > provider attributes that are generic, but typed, allowing the nlmessage > > > > to ve validated. Currently the types include string, s32, u32, x32, > > s64, > > > > u64, and x64. The inclusion of x, s, and u variants for numeric > > attributes > > > > allows the user tool to print the number in the desired format. > > > > > > > > More attrs can be defined as they become needed by providers. > > > > > > > > Signed-off-by: Steve Wise <swise@opengridcomputing.com> > > > > drivers/infiniband/core/nldev.c | 39 > > > +++++++++++++++++++++++++++++++++++++++ > > > > include/rdma/restrack.h | 10 ++++++++++ > > > > include/uapi/rdma/rdma_netlink.h | 16 ++++++++++++++++ > > > > 3 files changed, 65 insertions(+) > > > > > > > > diff --git a/drivers/infiniband/core/nldev.c > > > b/drivers/infiniband/core/nldev.c > > > > index 884843e..8346ede 100644 > > > > +++ b/drivers/infiniband/core/nldev.c > > > > @@ -95,8 +95,27 @@ > > > > [RDMA_NLDEV_ATTR_RES_PD_ENTRY] = { .type = > > > NLA_NESTED }, > > > > [RDMA_NLDEV_ATTR_RES_LOCAL_DMA_LKEY] = { .type = NLA_U32 }, > > > > [RDMA_NLDEV_ATTR_RES_UNSAFE_GLOBAL_RKEY] = { .type = > > > NLA_U32 }, > > > > + [RDMA_NLDEV_ATTR_PROVIDER] = { .type = > > > NLA_NESTED }, > > > > + [RDMA_NLDEV_ATTR_PROVIDER_ENTRY] = { .type = > > > NLA_NESTED }, > > > > + [RDMA_NLDEV_ATTR_PROVIDER_STRING] = { .type = > > > NLA_NUL_STRING, > > > > + .len = > > > RDMA_NLDEV_ATTR_PROVIDER_STRLEN }, > > > > + [RDMA_NLDEV_ATTR_PROVIDER_D32] = { .type = NLA_S32 }, > > > > + [RDMA_NLDEV_ATTR_PROVIDER_U32] = { .type = NLA_U32 }, > > > > + [RDMA_NLDEV_ATTR_PROVIDER_X32] = { .type = NLA_U32 }, > > > > + [RDMA_NLDEV_ATTR_PROVIDER_D64] = { .type = NLA_S64 }, > > > > + [RDMA_NLDEV_ATTR_PROVIDER_U64] = { .type = NLA_U64 }, > > > > + [RDMA_NLDEV_ATTR_PROVIDER_X64] = { .type = NLA_U64 }, > > > > }; > > > > > > > > +static int provider_fill_res_entry(struct rdma_restrack_root *resroot, > > > > + struct sk_buff *msg, > > > > + struct netlink_callback *cb, > > > > + struct rdma_restrack_entry *res) > > > > +{ > > > > + return resroot->fill_res_entry ? > > > > + resroot->fill_res_entry(msg, cb, res) : 0; > > > > +} > > > > > > Please add "fill_res_entry = NULL" line into rdma_restrack_init() > > > despite kzalloc usage in ib_alloc_device(). > > > > > > Will do. > > > > > > > > And I afraid that we didn't settle the PROVIDER_*64 thing. > > > > > > > I didn't agree that your proposal was simpler, or even avoided the issues > > you said were problems with the self-describing-print-formt attributes. I > > asked if anyone else had an opinion. Nobody replied. So I chose to keep > > the attributes as they are. Are you NAKing this? > > > > Jason and Doug, do you have an opinion either way? Our discussion of this > > can be found here: > > > > https://www.spinics.net/lists/linux-rdma/msg62198.html > > I'm still not sure what you are two are arguing about.. > > Is it the inclusion of 'X'? That does seem weird. Yes, I'm proposing to have three attributes (RDMA_NLDEV_ATTR_PROVIDER_32, RDMA_NLDEV_ATTR_PROVIDER_64 and RDMA_NLDEV_ATTR_PROVIDER_PRINT_TYPE) instead of variants proposed by Steve. Thanks > > Jason
> -----Original Message----- > From: Jason Gunthorpe <jgg@mellanox.com> > Sent: Tuesday, March 27, 2018 9:50 AM > To: Steve Wise <swise@opengridcomputing.com> > Cc: 'Leon Romanovsky' <leon@kernel.org>; dledford@redhat.com; linux- > rdma@vger.kernel.org > Subject: Re: [PATCH v1 rdma-next 2/4] RDMA/nldev: add provider-specific > resource tracking > > On Sat, Mar 24, 2018 at 02:42:15PM -0500, Steve Wise wrote: > > > > > > > > On Fri, Mar 23, 2018 at 09:49:39AM -0700, Steve Wise wrote: > > > > Each provider can register a "fill entry" function with the restrack > > core. > > > > This function will be called when filling out a resource, allowing the > > > > provider to add provider-specific details. The details consist of a > > > > table of nested attributes, that are in the form of "key, value" tuple. > > > > The key nlattr must be strings, and the value nlattr can be one of > > > > provider attributes that are generic, but typed, allowing the > nlmessage > > > > to ve validated. Currently the types include string, s32, u32, x32, > > s64, > > > > u64, and x64. The inclusion of x, s, and u variants for numeric > > attributes > > > > allows the user tool to print the number in the desired format. > > > > > > > > More attrs can be defined as they become needed by providers. > > > > > > > > Signed-off-by: Steve Wise <swise@opengridcomputing.com> > > > > drivers/infiniband/core/nldev.c | 39 > > > +++++++++++++++++++++++++++++++++++++++ > > > > include/rdma/restrack.h | 10 ++++++++++ > > > > include/uapi/rdma/rdma_netlink.h | 16 ++++++++++++++++ > > > > 3 files changed, 65 insertions(+) > > > > > > > > diff --git a/drivers/infiniband/core/nldev.c > > > b/drivers/infiniband/core/nldev.c > > > > index 884843e..8346ede 100644 > > > > +++ b/drivers/infiniband/core/nldev.c > > > > @@ -95,8 +95,27 @@ > > > > [RDMA_NLDEV_ATTR_RES_PD_ENTRY] = { .type = > > > NLA_NESTED }, > > > > [RDMA_NLDEV_ATTR_RES_LOCAL_DMA_LKEY] = { .type = NLA_U32 }, > > > > [RDMA_NLDEV_ATTR_RES_UNSAFE_GLOBAL_RKEY] = { .type = > > > NLA_U32 }, > > > > + [RDMA_NLDEV_ATTR_PROVIDER] = { .type = > > > NLA_NESTED }, > > > > + [RDMA_NLDEV_ATTR_PROVIDER_ENTRY] = { .type = > > > NLA_NESTED }, > > > > + [RDMA_NLDEV_ATTR_PROVIDER_STRING] = { .type = > > > NLA_NUL_STRING, > > > > + .len = > > > RDMA_NLDEV_ATTR_PROVIDER_STRLEN }, > > > > + [RDMA_NLDEV_ATTR_PROVIDER_D32] = { .type = NLA_S32 }, > > > > + [RDMA_NLDEV_ATTR_PROVIDER_U32] = { .type = NLA_U32 }, > > > > + [RDMA_NLDEV_ATTR_PROVIDER_X32] = { .type = NLA_U32 }, > > > > + [RDMA_NLDEV_ATTR_PROVIDER_D64] = { .type = NLA_S64 }, > > > > + [RDMA_NLDEV_ATTR_PROVIDER_U64] = { .type = NLA_U64 }, > > > > + [RDMA_NLDEV_ATTR_PROVIDER_X64] = { .type = NLA_U64 }, > > > > }; > > > > > > > > +static int provider_fill_res_entry(struct rdma_restrack_root *resroot, > > > > + struct sk_buff *msg, > > > > + struct netlink_callback *cb, > > > > + struct rdma_restrack_entry *res) > > > > +{ > > > > + return resroot->fill_res_entry ? > > > > + resroot->fill_res_entry(msg, cb, res) : 0; > > > > +} > > > > > > Please add "fill_res_entry = NULL" line into rdma_restrack_init() > > > despite kzalloc usage in ib_alloc_device(). > > > > > > Will do. > > > > > > > > And I afraid that we didn't settle the PROVIDER_*64 thing. > > > > > > > I didn't agree that your proposal was simpler, or even avoided the issues > > you said were problems with the self-describing-print-formt attributes. I > > asked if anyone else had an opinion. Nobody replied. So I chose to keep > > the attributes as they are. Are you NAKing this? > > > > Jason and Doug, do you have an opinion either way? Our discussion of > this > > can be found here: > > > > https://www.spinics.net/lists/linux-rdma/msg62198.html > > I'm still not sure what you are two are arguing about.. > > Is it the inclusion of 'X'? That does seem weird. > > Jason Discussing, not arguing.
> On Tue, Mar 27, 2018 at 08:50:22AM -0600, Jason Gunthorpe wrote: > > On Sat, Mar 24, 2018 at 02:42:15PM -0500, Steve Wise wrote: > > > > > > > > > > > On Fri, Mar 23, 2018 at 09:49:39AM -0700, Steve Wise wrote: > > > > > Each provider can register a "fill entry" function with the restrack > > > core. > > > > > This function will be called when filling out a resource, allowing the > > > > > provider to add provider-specific details. The details consist of a > > > > > table of nested attributes, that are in the form of "key, value" tuple. > > > > > The key nlattr must be strings, and the value nlattr can be one of > > > > > provider attributes that are generic, but typed, allowing the > nlmessage > > > > > to ve validated. Currently the types include string, s32, u32, x32, > > > s64, > > > > > u64, and x64. The inclusion of x, s, and u variants for numeric > > > attributes > > > > > allows the user tool to print the number in the desired format. > > > > > > > > > > More attrs can be defined as they become needed by providers. > > > > > > > > > > Signed-off-by: Steve Wise <swise@opengridcomputing.com> > > > > > drivers/infiniband/core/nldev.c | 39 > > > > +++++++++++++++++++++++++++++++++++++++ > > > > > include/rdma/restrack.h | 10 ++++++++++ > > > > > include/uapi/rdma/rdma_netlink.h | 16 ++++++++++++++++ > > > > > 3 files changed, 65 insertions(+) > > > > > > > > > > diff --git a/drivers/infiniband/core/nldev.c > > > > b/drivers/infiniband/core/nldev.c > > > > > index 884843e..8346ede 100644 > > > > > +++ b/drivers/infiniband/core/nldev.c > > > > > @@ -95,8 +95,27 @@ > > > > > [RDMA_NLDEV_ATTR_RES_PD_ENTRY] = { .type = > > > > NLA_NESTED }, > > > > > [RDMA_NLDEV_ATTR_RES_LOCAL_DMA_LKEY] = { .type = > NLA_U32 }, > > > > > [RDMA_NLDEV_ATTR_RES_UNSAFE_GLOBAL_RKEY] = { .type > = > > > > NLA_U32 }, > > > > > + [RDMA_NLDEV_ATTR_PROVIDER] = { .type = > > > > NLA_NESTED }, > > > > > + [RDMA_NLDEV_ATTR_PROVIDER_ENTRY] = { .type = > > > > NLA_NESTED }, > > > > > + [RDMA_NLDEV_ATTR_PROVIDER_STRING] = { .type = > > > > NLA_NUL_STRING, > > > > > + .len = > > > > RDMA_NLDEV_ATTR_PROVIDER_STRLEN }, > > > > > + [RDMA_NLDEV_ATTR_PROVIDER_D32] = { .type = > NLA_S32 }, > > > > > + [RDMA_NLDEV_ATTR_PROVIDER_U32] = { .type = > NLA_U32 }, > > > > > + [RDMA_NLDEV_ATTR_PROVIDER_X32] = { .type = > NLA_U32 }, > > > > > + [RDMA_NLDEV_ATTR_PROVIDER_D64] = { .type = > NLA_S64 }, > > > > > + [RDMA_NLDEV_ATTR_PROVIDER_U64] = { .type = > NLA_U64 }, > > > > > + [RDMA_NLDEV_ATTR_PROVIDER_X64] = { .type = > NLA_U64 }, > > > > > }; > > > > > > > > > > +static int provider_fill_res_entry(struct rdma_restrack_root > *resroot, > > > > > + struct sk_buff *msg, > > > > > + struct netlink_callback *cb, > > > > > + struct rdma_restrack_entry *res) > > > > > +{ > > > > > + return resroot->fill_res_entry ? > > > > > + resroot->fill_res_entry(msg, cb, res) : 0; > > > > > +} > > > > > > > > Please add "fill_res_entry = NULL" line into rdma_restrack_init() > > > > despite kzalloc usage in ib_alloc_device(). > > > > > > > > > Will do. > > > > > > > > > > > And I afraid that we didn't settle the PROVIDER_*64 thing. > > > > > > > > > > I didn't agree that your proposal was simpler, or even avoided the > issues > > > you said were problems with the self-describing-print-formt attributes. > I > > > asked if anyone else had an opinion. Nobody replied. So I chose to > keep > > > the attributes as they are. Are you NAKing this? > > > > > > Jason and Doug, do you have an opinion either way? Our discussion of > this > > > can be found here: > > > > > > https://www.spinics.net/lists/linux-rdma/msg62198.html > > > > I'm still not sure what you are two are arguing about.. > > > > Is it the inclusion of 'X'? That does seem weird. > > Yes, I'm proposing to have three attributes > (RDMA_NLDEV_ATTR_PROVIDER_32, RDMA_NLDEV_ATTR_PROVIDER_64 and > RDMA_NLDEV_ATTR_PROVIDER_PRINT_TYPE) instead of variants proposed > by Steve. > Hey Leon, with your PRINT_TYPE string attribute proposal, how would you handle PRIu64 and friends? IE what specific strings would be passed up to the user for 64b attributes? -- 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 Tue, Mar 27, 2018 at 09:58:13AM -0500, Steve Wise wrote: > Steve's proposed attributes like BLAH_U32, BLAH_X32, and BLAH_D32 > are efficient because they convey, directly, how the user side > should display them. Leon prefers a separate string attribute that > is provided along with the value to convey the display format, and > the default would be unsigned so the display format attribute could > be excluded and the user side knows to use "%u". Signed or not should be part of the attribute type for sure, just for sanity. We should type check those things.. That just leaves X or not X, and why does that matter to anyone? 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 Tue, Mar 27, 2018 at 10:35:27PM -0600, Jason Gunthorpe wrote: > On Tue, Mar 27, 2018 at 09:58:13AM -0500, Steve Wise wrote: > > > Steve's proposed attributes like BLAH_U32, BLAH_X32, and BLAH_D32 > > are efficient because they convey, directly, how the user side > > should display them. Leon prefers a separate string attribute that > > is provided along with the value to convey the display format, and > > the default would be unsigned so the display format attribute could > > be excluded and the user side knows to use "%u". > > Signed or not should be part of the attribute type for sure, just for > sanity. We should type check those things.. There is type NLA_S64 especially for this. > > That just leaves X or not X, and why does that matter to anyone? As I posted before, I want to reuse those fields for set operations and want to ensure that drivers/core won't need to deal with users who send the same field sometimes with X64 and sometimes with U64 which for the kernel are the same. Plus it makes nla_policy looks very strange, despite the fact that we will write NLA_U64, the user will need to remember to treat it differently for prints. My print modifier makes this behaviour explicit. Thanks > > 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 Tue, Mar 27, 2018 at 04:18:39PM -0500, Steve Wise wrote: > > On Tue, Mar 27, 2018 at 08:50:22AM -0600, Jason Gunthorpe wrote: > > > On Sat, Mar 24, 2018 at 02:42:15PM -0500, Steve Wise wrote: > > > > > > > > > > > > > > On Fri, Mar 23, 2018 at 09:49:39AM -0700, Steve Wise wrote: > > > > > > Each provider can register a "fill entry" function with the > restrack > > > > core. > > > > > > This function will be called when filling out a resource, allowing > the > > > > > > provider to add provider-specific details. The details consist of > a > > > > > > table of nested attributes, that are in the form of "key, value" > tuple. > > > > > > The key nlattr must be strings, and the value nlattr can be one of > > > > > > provider attributes that are generic, but typed, allowing the > > nlmessage > > > > > > to ve validated. Currently the types include string, s32, u32, > x32, > > > > s64, > > > > > > u64, and x64. The inclusion of x, s, and u variants for numeric > > > > attributes > > > > > > allows the user tool to print the number in the desired format. > > > > > > > > > > > > More attrs can be defined as they become needed by providers. > > > > > > > > > > > > Signed-off-by: Steve Wise <swise@opengridcomputing.com> > > > > > > drivers/infiniband/core/nldev.c | 39 > > > > > +++++++++++++++++++++++++++++++++++++++ > > > > > > include/rdma/restrack.h | 10 ++++++++++ > > > > > > include/uapi/rdma/rdma_netlink.h | 16 ++++++++++++++++ > > > > > > 3 files changed, 65 insertions(+) > > > > > > > > > > > > diff --git a/drivers/infiniband/core/nldev.c > > > > > b/drivers/infiniband/core/nldev.c > > > > > > index 884843e..8346ede 100644 > > > > > > +++ b/drivers/infiniband/core/nldev.c > > > > > > @@ -95,8 +95,27 @@ > > > > > > [RDMA_NLDEV_ATTR_RES_PD_ENTRY] = { .type = > > > > > NLA_NESTED }, > > > > > > [RDMA_NLDEV_ATTR_RES_LOCAL_DMA_LKEY] = { .type = > > NLA_U32 }, > > > > > > [RDMA_NLDEV_ATTR_RES_UNSAFE_GLOBAL_RKEY] = { .type > > = > > > > > NLA_U32 }, > > > > > > + [RDMA_NLDEV_ATTR_PROVIDER] = { .type = > > > > > NLA_NESTED }, > > > > > > + [RDMA_NLDEV_ATTR_PROVIDER_ENTRY] = { .type = > > > > > NLA_NESTED }, > > > > > > + [RDMA_NLDEV_ATTR_PROVIDER_STRING] = { .type = > > > > > NLA_NUL_STRING, > > > > > > + .len = > > > > > RDMA_NLDEV_ATTR_PROVIDER_STRLEN }, > > > > > > + [RDMA_NLDEV_ATTR_PROVIDER_D32] = { .type = > > NLA_S32 }, > > > > > > + [RDMA_NLDEV_ATTR_PROVIDER_U32] = { .type = > > NLA_U32 }, > > > > > > + [RDMA_NLDEV_ATTR_PROVIDER_X32] = { .type = > > NLA_U32 }, > > > > > > + [RDMA_NLDEV_ATTR_PROVIDER_D64] = { .type = > > NLA_S64 }, > > > > > > + [RDMA_NLDEV_ATTR_PROVIDER_U64] = { .type = > > NLA_U64 }, > > > > > > + [RDMA_NLDEV_ATTR_PROVIDER_X64] = { .type = > > NLA_U64 }, > > > > > > }; > > > > > > > > > > > > +static int provider_fill_res_entry(struct rdma_restrack_root > > *resroot, > > > > > > + struct sk_buff *msg, > > > > > > + struct netlink_callback *cb, > > > > > > + struct rdma_restrack_entry *res) > > > > > > +{ > > > > > > + return resroot->fill_res_entry ? > > > > > > + resroot->fill_res_entry(msg, cb, res) : 0; > > > > > > +} > > > > > > > > > > Please add "fill_res_entry = NULL" line into rdma_restrack_init() > > > > > despite kzalloc usage in ib_alloc_device(). > > > > > > > > > > > > Will do. > > > > > > > > > > > > > > And I afraid that we didn't settle the PROVIDER_*64 thing. > > > > > > > > > > > > > I didn't agree that your proposal was simpler, or even avoided the > > issues > > > > you said were problems with the self-describing-print-formt > attributes. > > I > > > > asked if anyone else had an opinion. Nobody replied. So I chose to > > keep > > > > the attributes as they are. Are you NAKing this? > > > > > > > > Jason and Doug, do you have an opinion either way? Our discussion of > > this > > > > can be found here: > > > > > > > > https://www.spinics.net/lists/linux-rdma/msg62198.html > > > > > > I'm still not sure what you are two are arguing about.. > > > > > > Is it the inclusion of 'X'? That does seem weird. > > > > Yes, I'm proposing to have three attributes > > (RDMA_NLDEV_ATTR_PROVIDER_32, RDMA_NLDEV_ATTR_PROVIDER_64 and > > RDMA_NLDEV_ATTR_PROVIDER_PRINT_TYPE) instead of variants proposed > > by Steve. > > > > Hey Leon, with your PRINT_TYPE string attribute proposal, how would you > handle PRIu64 and friends? IE what specific strings would be passed up to > the user for 64b attributes? I will try to summarize the users behavior and their prints: PRINT_TYPE | PROVIDER_U64 | PROVIDER_S64 | PROVIDER_U32 | PROVIDER_S32 | ------------------------------------------------------------------------- unspecified | %PRIu64 | %PRId64 | %u | %d | hex | %PRIx64 | %PRIx64 | %x | %x | ------------------------------------------------------------------------- The "unspecified", it is current behaviour of libnetlink. So once, user will want to provide information for the kernel, he will fill PROVIDER_U64 with data and will send it to the kernel without need to fill PRINT_TYPE. 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 3/27/2018 11:35 PM, Jason Gunthorpe wrote: > On Tue, Mar 27, 2018 at 09:58:13AM -0500, Steve Wise wrote: > >> Steve's proposed attributes like BLAH_U32, BLAH_X32, and BLAH_D32 >> are efficient because they convey, directly, how the user side >> should display them. Leon prefers a separate string attribute that >> is provided along with the value to convey the display format, and >> the default would be unsigned so the display format attribute could >> be excluded and the user side knows to use "%u". > Signed or not should be part of the attribute type for sure, just for > sanity. We should type check those things.. > > That just leaves X or not X, and why does that matter to anyone? Typically bit arrays, iova base, memory keys, are displayed as hex. For the common attributes describing the core structs, like ib_device, ib_qp, etc, rdma tool knows which ones should be displayed as unsigned, signed, or hex. But for generic provider-specific entries, the rdma tool doesn't know anything about what is being displayed, because it is a <name, value> tuple with display format described by the provider attribute. So X32 and X64 at least provide a hint saying 'print it as hex'. So in my opinion, it makes for more readable output which is useful for debugging. 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 3/28/2018 12:10 AM, Leon Romanovsky wrote: > On Tue, Mar 27, 2018 at 04:18:39PM -0500, Steve Wise wrote: >>> On Tue, Mar 27, 2018 at 08:50:22AM -0600, Jason Gunthorpe wrote: >>>> On Sat, Mar 24, 2018 at 02:42:15PM -0500, Steve Wise wrote: >>>>>> On Fri, Mar 23, 2018 at 09:49:39AM -0700, Steve Wise wrote: >>>>>>> Each provider can register a "fill entry" function with the >> restrack >>>>> core. >>>>>>> This function will be called when filling out a resource, allowing >> the >>>>>>> provider to add provider-specific details. The details consist of >> a >>>>>>> table of nested attributes, that are in the form of "key, value" >> tuple. >>>>>>> The key nlattr must be strings, and the value nlattr can be one of >>>>>>> provider attributes that are generic, but typed, allowing the >>> nlmessage >>>>>>> to ve validated. Currently the types include string, s32, u32, >> x32, >>>>> s64, >>>>>>> u64, and x64. The inclusion of x, s, and u variants for numeric >>>>> attributes >>>>>>> allows the user tool to print the number in the desired format. >>>>>>> >>>>>>> More attrs can be defined as they become needed by providers. >>>>>>> >>>>>>> Signed-off-by: Steve Wise <swise@opengridcomputing.com> >>>>>>> drivers/infiniband/core/nldev.c | 39 >>>>>> +++++++++++++++++++++++++++++++++++++++ >>>>>>> include/rdma/restrack.h | 10 ++++++++++ >>>>>>> include/uapi/rdma/rdma_netlink.h | 16 ++++++++++++++++ >>>>>>> 3 files changed, 65 insertions(+) >>>>>>> >>>>>>> diff --git a/drivers/infiniband/core/nldev.c >>>>>> b/drivers/infiniband/core/nldev.c >>>>>>> index 884843e..8346ede 100644 >>>>>>> +++ b/drivers/infiniband/core/nldev.c >>>>>>> @@ -95,8 +95,27 @@ >>>>>>> [RDMA_NLDEV_ATTR_RES_PD_ENTRY] = { .type = >>>>>> NLA_NESTED }, >>>>>>> [RDMA_NLDEV_ATTR_RES_LOCAL_DMA_LKEY] = { .type = >>> NLA_U32 }, >>>>>>> [RDMA_NLDEV_ATTR_RES_UNSAFE_GLOBAL_RKEY] = { .type >>> = >>>>>> NLA_U32 }, >>>>>>> + [RDMA_NLDEV_ATTR_PROVIDER] = { .type = >>>>>> NLA_NESTED }, >>>>>>> + [RDMA_NLDEV_ATTR_PROVIDER_ENTRY] = { .type = >>>>>> NLA_NESTED }, >>>>>>> + [RDMA_NLDEV_ATTR_PROVIDER_STRING] = { .type = >>>>>> NLA_NUL_STRING, >>>>>>> + .len = >>>>>> RDMA_NLDEV_ATTR_PROVIDER_STRLEN }, >>>>>>> + [RDMA_NLDEV_ATTR_PROVIDER_D32] = { .type = >>> NLA_S32 }, >>>>>>> + [RDMA_NLDEV_ATTR_PROVIDER_U32] = { .type = >>> NLA_U32 }, >>>>>>> + [RDMA_NLDEV_ATTR_PROVIDER_X32] = { .type = >>> NLA_U32 }, >>>>>>> + [RDMA_NLDEV_ATTR_PROVIDER_D64] = { .type = >>> NLA_S64 }, >>>>>>> + [RDMA_NLDEV_ATTR_PROVIDER_U64] = { .type = >>> NLA_U64 }, >>>>>>> + [RDMA_NLDEV_ATTR_PROVIDER_X64] = { .type = >>> NLA_U64 }, >>>>>>> }; >>>>>>> >>>>>>> +static int provider_fill_res_entry(struct rdma_restrack_root >>> *resroot, >>>>>>> + struct sk_buff *msg, >>>>>>> + struct netlink_callback *cb, >>>>>>> + struct rdma_restrack_entry *res) >>>>>>> +{ >>>>>>> + return resroot->fill_res_entry ? >>>>>>> + resroot->fill_res_entry(msg, cb, res) : 0; >>>>>>> +} >>>>>> Please add "fill_res_entry = NULL" line into rdma_restrack_init() >>>>>> despite kzalloc usage in ib_alloc_device(). >>>>> >>>>> Will do. >>>>> >>>>>> And I afraid that we didn't settle the PROVIDER_*64 thing. >>>>>> >>>>> I didn't agree that your proposal was simpler, or even avoided the >>> issues >>>>> you said were problems with the self-describing-print-formt >> attributes. >>> I >>>>> asked if anyone else had an opinion. Nobody replied. So I chose to >>> keep >>>>> the attributes as they are. Are you NAKing this? >>>>> >>>>> Jason and Doug, do you have an opinion either way? Our discussion of >>> this >>>>> can be found here: >>>>> >>>>> https://www.spinics.net/lists/linux-rdma/msg62198.html >>>> I'm still not sure what you are two are arguing about.. >>>> >>>> Is it the inclusion of 'X'? That does seem weird. >>> Yes, I'm proposing to have three attributes >>> (RDMA_NLDEV_ATTR_PROVIDER_32, RDMA_NLDEV_ATTR_PROVIDER_64 and >>> RDMA_NLDEV_ATTR_PROVIDER_PRINT_TYPE) instead of variants proposed >>> by Steve. >>> >> Hey Leon, with your PRINT_TYPE string attribute proposal, how would you >> handle PRIu64 and friends? IE what specific strings would be passed up to >> the user for 64b attributes? > I will try to summarize the users behavior and their prints: > > PRINT_TYPE | PROVIDER_U64 | PROVIDER_S64 | PROVIDER_U32 | PROVIDER_S32 | > ------------------------------------------------------------------------- > unspecified | %PRIu64 | %PRId64 | %u | %d | > hex | %PRIx64 | %PRIx64 | %x | %x | > ------------------------------------------------------------------------- > > The "unspecified", it is current behaviour of libnetlink. > > So once, user will want to provide information for the kernel, he will > fill PROVIDER_U64 with data and will send it to the kernel without need > to fill PRINT_TYPE. > > So what strings, exactly, get put in the PRINT_TYPE attribute? It seems like now we only have hex or no hex? I'll move to this design once I understand it fully. 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 Wed, Mar 28, 2018 at 08:50:34AM -0500, Steve Wise wrote: > > > On 3/28/2018 12:10 AM, Leon Romanovsky wrote: > > On Tue, Mar 27, 2018 at 04:18:39PM -0500, Steve Wise wrote: > >>> On Tue, Mar 27, 2018 at 08:50:22AM -0600, Jason Gunthorpe wrote: > >>>> On Sat, Mar 24, 2018 at 02:42:15PM -0500, Steve Wise wrote: > >>>>>> On Fri, Mar 23, 2018 at 09:49:39AM -0700, Steve Wise wrote: > >>>>>>> Each provider can register a "fill entry" function with the > >> restrack > >>>>> core. > >>>>>>> This function will be called when filling out a resource, allowing > >> the > >>>>>>> provider to add provider-specific details. The details consist of > >> a > >>>>>>> table of nested attributes, that are in the form of "key, value" > >> tuple. > >>>>>>> The key nlattr must be strings, and the value nlattr can be one of > >>>>>>> provider attributes that are generic, but typed, allowing the > >>> nlmessage > >>>>>>> to ve validated. Currently the types include string, s32, u32, > >> x32, > >>>>> s64, > >>>>>>> u64, and x64. The inclusion of x, s, and u variants for numeric > >>>>> attributes > >>>>>>> allows the user tool to print the number in the desired format. > >>>>>>> > >>>>>>> More attrs can be defined as they become needed by providers. > >>>>>>> > >>>>>>> Signed-off-by: Steve Wise <swise@opengridcomputing.com> > >>>>>>> drivers/infiniband/core/nldev.c | 39 > >>>>>> +++++++++++++++++++++++++++++++++++++++ > >>>>>>> include/rdma/restrack.h | 10 ++++++++++ > >>>>>>> include/uapi/rdma/rdma_netlink.h | 16 ++++++++++++++++ > >>>>>>> 3 files changed, 65 insertions(+) > >>>>>>> > >>>>>>> diff --git a/drivers/infiniband/core/nldev.c > >>>>>> b/drivers/infiniband/core/nldev.c > >>>>>>> index 884843e..8346ede 100644 > >>>>>>> +++ b/drivers/infiniband/core/nldev.c > >>>>>>> @@ -95,8 +95,27 @@ > >>>>>>> [RDMA_NLDEV_ATTR_RES_PD_ENTRY] = { .type = > >>>>>> NLA_NESTED }, > >>>>>>> [RDMA_NLDEV_ATTR_RES_LOCAL_DMA_LKEY] = { .type = > >>> NLA_U32 }, > >>>>>>> [RDMA_NLDEV_ATTR_RES_UNSAFE_GLOBAL_RKEY] = { .type > >>> = > >>>>>> NLA_U32 }, > >>>>>>> + [RDMA_NLDEV_ATTR_PROVIDER] = { .type = > >>>>>> NLA_NESTED }, > >>>>>>> + [RDMA_NLDEV_ATTR_PROVIDER_ENTRY] = { .type = > >>>>>> NLA_NESTED }, > >>>>>>> + [RDMA_NLDEV_ATTR_PROVIDER_STRING] = { .type = > >>>>>> NLA_NUL_STRING, > >>>>>>> + .len = > >>>>>> RDMA_NLDEV_ATTR_PROVIDER_STRLEN }, > >>>>>>> + [RDMA_NLDEV_ATTR_PROVIDER_D32] = { .type = > >>> NLA_S32 }, > >>>>>>> + [RDMA_NLDEV_ATTR_PROVIDER_U32] = { .type = > >>> NLA_U32 }, > >>>>>>> + [RDMA_NLDEV_ATTR_PROVIDER_X32] = { .type = > >>> NLA_U32 }, > >>>>>>> + [RDMA_NLDEV_ATTR_PROVIDER_D64] = { .type = > >>> NLA_S64 }, > >>>>>>> + [RDMA_NLDEV_ATTR_PROVIDER_U64] = { .type = > >>> NLA_U64 }, > >>>>>>> + [RDMA_NLDEV_ATTR_PROVIDER_X64] = { .type = > >>> NLA_U64 }, > >>>>>>> }; > >>>>>>> > >>>>>>> +static int provider_fill_res_entry(struct rdma_restrack_root > >>> *resroot, > >>>>>>> + struct sk_buff *msg, > >>>>>>> + struct netlink_callback *cb, > >>>>>>> + struct rdma_restrack_entry *res) > >>>>>>> +{ > >>>>>>> + return resroot->fill_res_entry ? > >>>>>>> + resroot->fill_res_entry(msg, cb, res) : 0; > >>>>>>> +} > >>>>>> Please add "fill_res_entry = NULL" line into rdma_restrack_init() > >>>>>> despite kzalloc usage in ib_alloc_device(). > >>>>> > >>>>> Will do. > >>>>> > >>>>>> And I afraid that we didn't settle the PROVIDER_*64 thing. > >>>>>> > >>>>> I didn't agree that your proposal was simpler, or even avoided the > >>> issues > >>>>> you said were problems with the self-describing-print-formt > >> attributes. > >>> I > >>>>> asked if anyone else had an opinion. Nobody replied. So I chose to > >>> keep > >>>>> the attributes as they are. Are you NAKing this? > >>>>> > >>>>> Jason and Doug, do you have an opinion either way? Our discussion of > >>> this > >>>>> can be found here: > >>>>> > >>>>> https://www.spinics.net/lists/linux-rdma/msg62198.html > >>>> I'm still not sure what you are two are arguing about.. > >>>> > >>>> Is it the inclusion of 'X'? That does seem weird. > >>> Yes, I'm proposing to have three attributes > >>> (RDMA_NLDEV_ATTR_PROVIDER_32, RDMA_NLDEV_ATTR_PROVIDER_64 and > >>> RDMA_NLDEV_ATTR_PROVIDER_PRINT_TYPE) instead of variants proposed > >>> by Steve. > >>> > >> Hey Leon, with your PRINT_TYPE string attribute proposal, how would you > >> handle PRIu64 and friends? IE what specific strings would be passed up to > >> the user for 64b attributes? > > I will try to summarize the users behavior and their prints: > > > > PRINT_TYPE | PROVIDER_U64 | PROVIDER_S64 | PROVIDER_U32 | PROVIDER_S32 | > > ------------------------------------------------------------------------- > > unspecified | %PRIu64 | %PRId64 | %u | %d | > > hex | %PRIx64 | %PRIx64 | %x | %x | > > ------------------------------------------------------------------------- > > > > The "unspecified", it is current behaviour of libnetlink. > > > > So once, user will want to provide information for the kernel, he will > > fill PROVIDER_U64 with data and will send it to the kernel without need > > to fill PRINT_TYPE. > > > > > > So what strings, exactly, get put in the PRINT_TYPE attribute? It seems > like now we only have hex or no hex? You don't need to put anything for strings., because there is only one nla_type -> NLA_NUL_STRING. Regarding hex, you don't need to limit yourself, PRINT_TYPE is supposed to be u8, and it will allow us to introduce new PRINT_TYPEs. The following two changes will be added to uapi/rdma/rdma_netlink.h: 1. RDMA_NLDEV_ATTR_PRINT_TYPE 2. enum with print types enum print_type { "unspecified", "hex", "bin", ... }; And in RDMAtool, If you get unknown PRINT_TYPE, you will fallback to default variant according to nla_type. > > I'll move to this design once I understand it fully. Thanks > > Steve.
On Wed, Mar 28, 2018 at 07:51:58AM +0300, Leon Romanovsky wrote: > On Tue, Mar 27, 2018 at 10:35:27PM -0600, Jason Gunthorpe wrote: > > On Tue, Mar 27, 2018 at 09:58:13AM -0500, Steve Wise wrote: > > > > > Steve's proposed attributes like BLAH_U32, BLAH_X32, and BLAH_D32 > > > are efficient because they convey, directly, how the user side > > > should display them. Leon prefers a separate string attribute that > > > is provided along with the value to convey the display format, and > > > the default would be unsigned so the display format attribute could > > > be excluded and the user side knows to use "%u". > > > > Signed or not should be part of the attribute type for sure, just for > > sanity. We should type check those things.. > > There is type NLA_S64 especially for this. > > > > > That just leaves X or not X, and why does that matter to anyone? > > As I posted before, I want to reuse those fields for set operations and > want to ensure that drivers/core won't need to deal with users who send > the same field sometimes with X64 and sometimes with U64 which for the > kernel are the same. Okay.. Can't you just encode the hex/!hex in the name string somehow? Start with ! or something for hex. Seems like a big wast to include another attribute just for this. 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 3/28/2018 3:26 PM, Jason Gunthorpe wrote: > On Wed, Mar 28, 2018 at 07:51:58AM +0300, Leon Romanovsky wrote: >> On Tue, Mar 27, 2018 at 10:35:27PM -0600, Jason Gunthorpe wrote: >>> On Tue, Mar 27, 2018 at 09:58:13AM -0500, Steve Wise wrote: >>> >>>> Steve's proposed attributes like BLAH_U32, BLAH_X32, and BLAH_D32 >>>> are efficient because they convey, directly, how the user side >>>> should display them. Leon prefers a separate string attribute that >>>> is provided along with the value to convey the display format, and >>>> the default would be unsigned so the display format attribute could >>>> be excluded and the user side knows to use "%u". >>> Signed or not should be part of the attribute type for sure, just for >>> sanity. We should type check those things.. >> There is type NLA_S64 especially for this. >> >>> That just leaves X or not X, and why does that matter to anyone? >> As I posted before, I want to reuse those fields for set operations and >> want to ensure that drivers/core won't need to deal with users who send >> the same field sometimes with X64 and sometimes with U64 which for the >> kernel are the same. > Okay.. Can't you just encode the hex/!hex in the name string somehow? > Start with ! or something for hex. > Seems like a big wast to include another attribute just for this. Leon? -- 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, Mar 29, 2018 at 09:48:08AM -0500, Steve Wise wrote: > > > On 3/28/2018 3:26 PM, Jason Gunthorpe wrote: > > On Wed, Mar 28, 2018 at 07:51:58AM +0300, Leon Romanovsky wrote: > >> On Tue, Mar 27, 2018 at 10:35:27PM -0600, Jason Gunthorpe wrote: > >>> On Tue, Mar 27, 2018 at 09:58:13AM -0500, Steve Wise wrote: > >>> > >>>> Steve's proposed attributes like BLAH_U32, BLAH_X32, and BLAH_D32 > >>>> are efficient because they convey, directly, how the user side > >>>> should display them. Leon prefers a separate string attribute that > >>>> is provided along with the value to convey the display format, and > >>>> the default would be unsigned so the display format attribute could > >>>> be excluded and the user side knows to use "%u". > >>> Signed or not should be part of the attribute type for sure, just for > >>> sanity. We should type check those things.. > >> There is type NLA_S64 especially for this. > >> > >>> That just leaves X or not X, and why does that matter to anyone? > >> As I posted before, I want to reuse those fields for set operations and > >> want to ensure that drivers/core won't need to deal with users who send > >> the same field sometimes with X64 and sometimes with U64 which for the > >> kernel are the same. > > Okay.. Can't you just encode the hex/!hex in the name string somehow? > > Start with ! or something for hex. > > Seems like a big wast to include another attribute just for this. > > Leon? I didn't understand that it was for me. First, we have upto 64K attributes till we will exhaust them, second it is not only hex and not hex, but binary too. Thanks
diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c index 884843e..8346ede 100644 --- a/drivers/infiniband/core/nldev.c +++ b/drivers/infiniband/core/nldev.c @@ -95,8 +95,27 @@ [RDMA_NLDEV_ATTR_RES_PD_ENTRY] = { .type = NLA_NESTED }, [RDMA_NLDEV_ATTR_RES_LOCAL_DMA_LKEY] = { .type = NLA_U32 }, [RDMA_NLDEV_ATTR_RES_UNSAFE_GLOBAL_RKEY] = { .type = NLA_U32 }, + [RDMA_NLDEV_ATTR_PROVIDER] = { .type = NLA_NESTED }, + [RDMA_NLDEV_ATTR_PROVIDER_ENTRY] = { .type = NLA_NESTED }, + [RDMA_NLDEV_ATTR_PROVIDER_STRING] = { .type = NLA_NUL_STRING, + .len = RDMA_NLDEV_ATTR_PROVIDER_STRLEN }, + [RDMA_NLDEV_ATTR_PROVIDER_D32] = { .type = NLA_S32 }, + [RDMA_NLDEV_ATTR_PROVIDER_U32] = { .type = NLA_U32 }, + [RDMA_NLDEV_ATTR_PROVIDER_X32] = { .type = NLA_U32 }, + [RDMA_NLDEV_ATTR_PROVIDER_D64] = { .type = NLA_S64 }, + [RDMA_NLDEV_ATTR_PROVIDER_U64] = { .type = NLA_U64 }, + [RDMA_NLDEV_ATTR_PROVIDER_X64] = { .type = NLA_U64 }, }; +static int provider_fill_res_entry(struct rdma_restrack_root *resroot, + struct sk_buff *msg, + struct netlink_callback *cb, + struct rdma_restrack_entry *res) +{ + return resroot->fill_res_entry ? + resroot->fill_res_entry(msg, cb, res) : 0; +} + static int fill_nldev_handle(struct sk_buff *msg, struct ib_device *device) { if (nla_put_u32(msg, RDMA_NLDEV_ATTR_DEV_INDEX, device->index)) @@ -264,6 +283,7 @@ static int fill_res_qp_entry(struct sk_buff *msg, struct netlink_callback *cb, struct rdma_restrack_entry *res, uint32_t port) { struct ib_qp *qp = container_of(res, struct ib_qp, res); + struct rdma_restrack_root *resroot = &qp->device->res; struct ib_qp_init_attr qp_init_attr; struct nlattr *entry_attr; struct ib_qp_attr qp_attr; @@ -313,6 +333,9 @@ static int fill_res_qp_entry(struct sk_buff *msg, struct netlink_callback *cb, if (fill_res_name_pid(msg, res)) goto err; + if (provider_fill_res_entry(resroot, msg, cb, res)) + goto err; + nla_nest_end(msg, entry_attr); return 0; @@ -328,6 +351,7 @@ static int fill_res_cm_id_entry(struct sk_buff *msg, { struct rdma_id_private *id_priv = container_of(res, struct rdma_id_private, res); + struct rdma_restrack_root *resroot = &id_priv->id.device->res; struct rdma_cm_id *cm_id = &id_priv->id; struct nlattr *entry_attr; @@ -369,6 +393,9 @@ static int fill_res_cm_id_entry(struct sk_buff *msg, if (fill_res_name_pid(msg, res)) goto err; + if (provider_fill_res_entry(resroot, msg, cb, res)) + goto err; + nla_nest_end(msg, entry_attr); return 0; @@ -382,6 +409,7 @@ static int fill_res_cq_entry(struct sk_buff *msg, struct netlink_callback *cb, struct rdma_restrack_entry *res, uint32_t port) { struct ib_cq *cq = container_of(res, struct ib_cq, res); + struct rdma_restrack_root *resroot = &cq->device->res; struct nlattr *entry_attr; entry_attr = nla_nest_start(msg, RDMA_NLDEV_ATTR_RES_CQ_ENTRY); @@ -402,6 +430,9 @@ static int fill_res_cq_entry(struct sk_buff *msg, struct netlink_callback *cb, if (fill_res_name_pid(msg, res)) goto err; + if (provider_fill_res_entry(resroot, msg, cb, res)) + goto err; + nla_nest_end(msg, entry_attr); return 0; @@ -415,6 +446,7 @@ static int fill_res_mr_entry(struct sk_buff *msg, struct netlink_callback *cb, struct rdma_restrack_entry *res, uint32_t port) { struct ib_mr *mr = container_of(res, struct ib_mr, res); + struct rdma_restrack_root *resroot = &mr->pd->device->res; struct nlattr *entry_attr; entry_attr = nla_nest_start(msg, RDMA_NLDEV_ATTR_RES_MR_ENTRY); @@ -438,6 +470,9 @@ static int fill_res_mr_entry(struct sk_buff *msg, struct netlink_callback *cb, if (fill_res_name_pid(msg, res)) goto err; + if (provider_fill_res_entry(resroot, msg, cb, res)) + goto err; + nla_nest_end(msg, entry_attr); return 0; @@ -451,6 +486,7 @@ static int fill_res_pd_entry(struct sk_buff *msg, struct netlink_callback *cb, struct rdma_restrack_entry *res, uint32_t port) { struct ib_pd *pd = container_of(res, struct ib_pd, res); + struct rdma_restrack_root *resroot = &pd->device->res; struct nlattr *entry_attr; entry_attr = nla_nest_start(msg, RDMA_NLDEV_ATTR_RES_PD_ENTRY); @@ -477,6 +513,9 @@ static int fill_res_pd_entry(struct sk_buff *msg, struct netlink_callback *cb, if (fill_res_name_pid(msg, res)) goto err; + if (provider_fill_res_entry(resroot, msg, cb, res)) + goto err; + nla_nest_end(msg, entry_attr); return 0; diff --git a/include/rdma/restrack.h b/include/rdma/restrack.h index f3b3e35..bd3cd9a 100644 --- a/include/rdma/restrack.h +++ b/include/rdma/restrack.h @@ -44,6 +44,8 @@ enum rdma_restrack_type { }; #define RDMA_RESTRACK_HASH_BITS 8 +struct rdma_restrack_entry; + /** * struct rdma_restrack_root - main resource tracking management * entity, per-device @@ -57,6 +59,14 @@ struct rdma_restrack_root { * @hash: global database for all resources per-device */ DECLARE_HASHTABLE(hash, RDMA_RESTRACK_HASH_BITS); + /** + * @fill_res_entry: provider-specific fill function + * + * Allows rdma providers to add their own restrack attributes. + */ + int (*fill_res_entry)(struct sk_buff *msg, + struct netlink_callback *cb, + struct rdma_restrack_entry *entry); }; /** diff --git a/include/uapi/rdma/rdma_netlink.h b/include/uapi/rdma/rdma_netlink.h index 84b3f63..70443d4 100644 --- a/include/uapi/rdma/rdma_netlink.h +++ b/include/uapi/rdma/rdma_netlink.h @@ -249,6 +249,10 @@ enum rdma_nldev_command { RDMA_NLDEV_NUM_OPS }; +enum { + RDMA_NLDEV_ATTR_PROVIDER_STRLEN = 16, +}; + enum rdma_nldev_attr { /* don't change the order or add anything between, this is ABI! */ RDMA_NLDEV_ATTR_UNSPEC, @@ -390,6 +394,18 @@ enum rdma_nldev_attr { RDMA_NLDEV_ATTR_RES_PD_ENTRY, /* nested table */ RDMA_NLDEV_ATTR_RES_LOCAL_DMA_LKEY, /* u32 */ RDMA_NLDEV_ATTR_RES_UNSAFE_GLOBAL_RKEY, /* u32 */ + /* + * provider-specific attributes. + */ + RDMA_NLDEV_ATTR_PROVIDER, /* nested table */ + RDMA_NLDEV_ATTR_PROVIDER_ENTRY, /* nested table */ + RDMA_NLDEV_ATTR_PROVIDER_STRING, /* string */ + RDMA_NLDEV_ATTR_PROVIDER_D32, /* u32 */ + RDMA_NLDEV_ATTR_PROVIDER_U32, /* u32 */ + RDMA_NLDEV_ATTR_PROVIDER_X32, /* u32 */ + RDMA_NLDEV_ATTR_PROVIDER_D64, /* u64 */ + RDMA_NLDEV_ATTR_PROVIDER_U64, /* u64 */ + RDMA_NLDEV_ATTR_PROVIDER_X64, /* u64 */ RDMA_NLDEV_ATTR_MAX };
Each provider can register a "fill entry" function with the restrack core. This function will be called when filling out a resource, allowing the provider to add provider-specific details. The details consist of a table of nested attributes, that are in the form of "key, value" tuple. The key nlattr must be strings, and the value nlattr can be one of provider attributes that are generic, but typed, allowing the nlmessage to ve validated. Currently the types include string, s32, u32, x32, s64, u64, and x64. The inclusion of x, s, and u variants for numeric attributes allows the user tool to print the number in the desired format. More attrs can be defined as they become needed by providers. Signed-off-by: Steve Wise <swise@opengridcomputing.com> --- drivers/infiniband/core/nldev.c | 39 +++++++++++++++++++++++++++++++++++++++ include/rdma/restrack.h | 10 ++++++++++ include/uapi/rdma/rdma_netlink.h | 16 ++++++++++++++++ 3 files changed, 65 insertions(+)