| Message ID | 70bcad28319c90ce1699cce1a52fc80350fd5bb8.1521046858.git.swise@opengridcomputing.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
On Tue, Mar 13, 2018 at 10:30:32AM -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 | 40 ++++++++++++++++++++++++++++++++++++++++ > include/rdma/restrack.h | 10 ++++++++++ > include/uapi/rdma/rdma_netlink.h | 17 +++++++++++++++++ > 3 files changed, 67 insertions(+) > > diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c > index 192084c..933df61 100644 > --- a/drivers/infiniband/core/nldev.c > +++ b/drivers/infiniband/core/nldev.c > @@ -95,8 +95,28 @@ > [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 }, > + [RDMA_NLDEV_ATTR_PAD] = { }, RDMA_NLDEV_ATTR_PAD ???? > }; I would like to see this hook connected to dev and links too. Thanks
On Wed, Mar 14, 2018 at 07:33:53PM +0200, Leon Romanovsky wrote: > On Tue, Mar 13, 2018 at 10:30:32AM -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 | 40 ++++++++++++++++++++++++++++++++++++++++ > > include/rdma/restrack.h | 10 ++++++++++ > > include/uapi/rdma/rdma_netlink.h | 17 +++++++++++++++++ > > 3 files changed, 67 insertions(+) > > > > diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c > > index 192084c..933df61 100644 > > +++ b/drivers/infiniband/core/nldev.c > > @@ -95,8 +95,28 @@ > > [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 }, > > + [RDMA_NLDEV_ATTR_PAD] = { }, > > RDMA_NLDEV_ATTR_PAD ???? This was supposed to have been done earlier.. Seperate patch please, and change every caller of nla_put_u64_64bit to use the new constant. And I think it has to be 0 for ABI compat, doesn't it? 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 13, 2018 at 10:30:32AM -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 | 40 > ++++++++++++++++++++++++++++++++++++++++ > > include/rdma/restrack.h | 10 ++++++++++ > > include/uapi/rdma/rdma_netlink.h | 17 +++++++++++++++++ > > 3 files changed, 67 insertions(+) > > > > diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c > > index 192084c..933df61 100644 > > --- a/drivers/infiniband/core/nldev.c > > +++ b/drivers/infiniband/core/nldev.c > > @@ -95,8 +95,28 @@ > > [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 }, > > + [RDMA_NLDEV_ATTR_PAD] = { }, > > RDMA_NLDEV_ATTR_PAD ???? > Yea, I had added this but I guess we ended up using 0. I'll remove it. > > }; > > I would like to see this hook connected to dev and links too. > Ok. -- 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 14, 2018 at 07:33:53PM +0200, Leon Romanovsky wrote: > > On Tue, Mar 13, 2018 at 10:30:32AM -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 | 40 > ++++++++++++++++++++++++++++++++++++++++ > > > include/rdma/restrack.h | 10 ++++++++++ > > > include/uapi/rdma/rdma_netlink.h | 17 +++++++++++++++++ > > > 3 files changed, 67 insertions(+) > > > > > > diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c > > > index 192084c..933df61 100644 > > > +++ b/drivers/infiniband/core/nldev.c > > > @@ -95,8 +95,28 @@ > > > [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 }, > > > + [RDMA_NLDEV_ATTR_PAD] = { }, > > > > RDMA_NLDEV_ATTR_PAD ???? > > This was supposed to have been done earlier.. > It wasn't clear whether we needed it. Leon sez 0 is fine. > Seperate patch please, and change every caller of nla_put_u64_64bit to > use the new constant. > Yea ok. > And I think it has to be 0 for ABI compat, doesn't it? > I guess so, since 0 was used originally. 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 14, 2018 at 01:00:27PM -0500, Steve Wise wrote: > > On Wed, Mar 14, 2018 at 07:33:53PM +0200, Leon Romanovsky wrote: > > > On Tue, Mar 13, 2018 at 10:30:32AM -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 | 40 > > ++++++++++++++++++++++++++++++++++++++++ > > > > include/rdma/restrack.h | 10 ++++++++++ > > > > include/uapi/rdma/rdma_netlink.h | 17 +++++++++++++++++ > > > > 3 files changed, 67 insertions(+) > > > > > > > > diff --git a/drivers/infiniband/core/nldev.c > b/drivers/infiniband/core/nldev.c > > > > index 192084c..933df61 100644 > > > > +++ b/drivers/infiniband/core/nldev.c > > > > @@ -95,8 +95,28 @@ > > > > [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 }, > > > > + [RDMA_NLDEV_ATTR_PAD] = { }, > > > > > > RDMA_NLDEV_ATTR_PAD ???? > > > > This was supposed to have been done earlier.. > > > > It wasn't clear whether we needed it. Leon sez 0 is fine. > > > Seperate patch please, and change every caller of nla_put_u64_64bit to > > use the new constant. > > > > Yea ok. > > > And I think it has to be 0 for ABI compat, doesn't it? > > > > I guess so, since 0 was used originally. Yes, we are using 0, which is equal to RDMA_NLDEV_ATTR_UNSPEC. All zeros can be replaced for clarity, but from ABI perspective we are fine. Thanks > > Steve. >
On Tue, Mar 13, 2018 at 10:30:32AM -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 | 40 ++++++++++++++++++++++++++++++++++++++++ > include/rdma/restrack.h | 10 ++++++++++ > include/uapi/rdma/rdma_netlink.h | 17 +++++++++++++++++ > 3 files changed, 67 insertions(+) > > diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c > index 192084c..933df61 100644 > --- a/drivers/infiniband/core/nldev.c > +++ b/drivers/infiniband/core/nldev.c > @@ -95,8 +95,28 @@ > [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 }, Two comments here and I would like to hear other opinion too, before we are rushing to implement it. 1. I don't think that we need to distinguish between 32 and 64 and better to provide one U64 type only. 2. The type of attribute is better to be different NLA. Something like: + [RDMA_NLDEV_ATTR_PROVIDER_64] = { .type = NLA_U64 }, + [RDMA_NLDEV_ATTR_PROVIDER_PRINT_TYPE] = { .type = NLA_NUL_STRING }, and RDMA_NLDEV_ATTR_PROVIDER_PRINT_TYPE can be x64 or u64 if it is not set. Thanks
> > On Tue, Mar 13, 2018 at 10:30:32AM -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 | 40 > ++++++++++++++++++++++++++++++++++++++++ > > include/rdma/restrack.h | 10 ++++++++++ > > include/uapi/rdma/rdma_netlink.h | 17 +++++++++++++++++ > > 3 files changed, 67 insertions(+) > > > > diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c > > index 192084c..933df61 100644 > > --- a/drivers/infiniband/core/nldev.c > > +++ b/drivers/infiniband/core/nldev.c > > @@ -95,8 +95,28 @@ > > [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 }, > > Two comments here and I would like to hear other opinion too, before we > are rushing to implement it. > > 1. I don't think that we need to distinguish between 32 and 64 and > better to provide one U64 type only. Why? Seems wasteful to pass 8, 16, and 32b quantities via 64b. I originally had U16 and U8, but you advised against that. So lemme know why supporting only 64 is a good idea. I don't think "simplicity" is a good answer, by the way. Have u8-u64 doesn't make it much more complex... > 2. The type of attribute is better to be different NLA. > Something like: > > + [RDMA_NLDEV_ATTR_PROVIDER_64] = { .type = NLA_U64 }, > + [RDMA_NLDEV_ATTR_PROVIDER_PRINT_TYPE] = { .type = > NLA_NUL_STRING }, > > and RDMA_NLDEV_ATTR_PROVIDER_PRINT_TYPE can be x64 or u64 if it is not > set. So then each attribute would be a 3-tuple: string print name, string print type, and value. Seems wasteful of skb space. I'm concerned because there is a lot of data to pass up for cxgb4 data structures, and I'm afraid we'll run out of room. And the current design doesn't handle a single table spanning many skbs... Why do you dislike the attributes having the print type described in the attribute containing the value? It seems clean to me. Steve. > > 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 Thu, Mar 15, 2018 at 11:09:42AM -0500, Steve Wise wrote: > > > > On Tue, Mar 13, 2018 at 10:30:32AM -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 | 40 > > ++++++++++++++++++++++++++++++++++++++++ > > > include/rdma/restrack.h | 10 ++++++++++ > > > include/uapi/rdma/rdma_netlink.h | 17 +++++++++++++++++ > > > 3 files changed, 67 insertions(+) > > > > > > diff --git a/drivers/infiniband/core/nldev.c > b/drivers/infiniband/core/nldev.c > > > index 192084c..933df61 100644 > > > --- a/drivers/infiniband/core/nldev.c > > > +++ b/drivers/infiniband/core/nldev.c > > > @@ -95,8 +95,28 @@ > > > [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 }, > > > > Two comments here and I would like to hear other opinion too, before we > > are rushing to implement it. > > > > 1. I don't think that we need to distinguish between 32 and 64 and > > better to provide one U64 type only. > > Why? Seems wasteful to pass 8, 16, and 32b quantities via 64b. I > originally had U16 and U8, but you advised against that. So lemme know why > supporting only 64 is a good idea. I don't think "simplicity" is a good > answer, by the way. Have u8-u64 doesn't make it much more complex... > > > > 2. The type of attribute is better to be different NLA. > > Something like: > > > > + [RDMA_NLDEV_ATTR_PROVIDER_64] = { .type = NLA_U64 }, > > + [RDMA_NLDEV_ATTR_PROVIDER_PRINT_TYPE] = { .type = > > NLA_NUL_STRING }, > > > > and RDMA_NLDEV_ATTR_PROVIDER_PRINT_TYPE can be x64 or u64 if it is not > > set. > > So then each attribute would be a 3-tuple: string print name, string print > type, and value. Seems wasteful of skb space. I'm concerned because there > is a lot of data to pass up for cxgb4 data structures, and I'm afraid we'll > run out of room. And the current design doesn't handle a single table > spanning many skbs... > > Why do you dislike the attributes having the print type described in the > attribute containing the value? It seems clean to me. In most cases, you won't need all three attributes, but only two of them because the default will be decimal print and it is usual case. I don't like the proposed attributes because they mix output modifier with data property. The netlink attributes are supposed to be 2-way communication channel and while the *_X* maybe makes sense for kernel->user flows, for sure it makes no sense for user->kernel flows. More on that, it creates ambiguity for the user application which will use this flow (rdmatool is not the only one way to use those netlink attributes). More on that, I see those attributes connected to dev and link objects, so we will be able to use "rdma dev cxgb4 set vendor_knob new_data ..." semantics seamlessly without changing user space. This is additional reason why print information is not needed together with data. Regarding u32,u64 separation, let's do it and save skb space. Thanks > > Steve. > > > > > Thanks >
On 3/18/2018 2:55 AM, Leon Romanovsky wrote: > On Thu, Mar 15, 2018 at 11:09:42AM -0500, Steve Wise wrote: >>> On Tue, Mar 13, 2018 at 10:30:32AM -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 | 40 >>> ++++++++++++++++++++++++++++++++++++++++ >>>> include/rdma/restrack.h | 10 ++++++++++ >>>> include/uapi/rdma/rdma_netlink.h | 17 +++++++++++++++++ >>>> 3 files changed, 67 insertions(+) >>>> >>>> diff --git a/drivers/infiniband/core/nldev.c >> b/drivers/infiniband/core/nldev.c >>>> index 192084c..933df61 100644 >>>> --- a/drivers/infiniband/core/nldev.c >>>> +++ b/drivers/infiniband/core/nldev.c >>>> @@ -95,8 +95,28 @@ >>>> [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 }, >>> Two comments here and I would like to hear other opinion too, before we >>> are rushing to implement it. >>> >>> 1. I don't think that we need to distinguish between 32 and 64 and >>> better to provide one U64 type only. >> Why? Seems wasteful to pass 8, 16, and 32b quantities via 64b. I >> originally had U16 and U8, but you advised against that. So lemme know why >> supporting only 64 is a good idea. I don't think "simplicity" is a good >> answer, by the way. Have u8-u64 doesn't make it much more complex... >> >> >>> 2. The type of attribute is better to be different NLA. >>> Something like: >>> >>> + [RDMA_NLDEV_ATTR_PROVIDER_64] = { .type = NLA_U64 }, >>> + [RDMA_NLDEV_ATTR_PROVIDER_PRINT_TYPE] = { .type = >>> NLA_NUL_STRING }, >>> >>> and RDMA_NLDEV_ATTR_PROVIDER_PRINT_TYPE can be x64 or u64 if it is not >>> set. >> So then each attribute would be a 3-tuple: string print name, string print >> type, and value. Seems wasteful of skb space. I'm concerned because there >> is a lot of data to pass up for cxgb4 data structures, and I'm afraid we'll >> run out of room. And the current design doesn't handle a single table >> spanning many skbs... >> >> Why do you dislike the attributes having the print type described in the >> attribute containing the value? It seems clean to me. > In most cases, you won't need all three attributes, but only two of them > because the default will be decimal print and it is usual case. > > I don't like the proposed attributes because they mix output modifier > with data property. The netlink attributes are supposed to be 2-way > communication channel and while the *_X* maybe makes sense for kernel->user > flows, for sure it makes no sense for user->kernel flows. Even with your proposal, the PRINT_TYPE attrs will only be for kernel->user. So there's not difference in that respect... > More on that, it creates ambiguity for the user application which will > use this flow (rdmatool is not the only one way to use those netlink > attributes). How so? It really doesn't matter if the user->kernel message uses _X32 or _U32. Both should work. > More on that, I see those attributes connected to dev and link objects, > so we will be able to use "rdma dev cxgb4 set vendor_knob new_data ..." > semantics seamlessly without changing user space. This is additional > reason why print information is not needed together with data. Yes, but in my scheme there is no cost for providing the print info, even if it is not needed in the user->kern messages. Does anyone else have an opinion on this? Jason? > Regarding u32,u64 separation, let's do it and save skb space. Thanks, 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
diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c index 192084c..933df61 100644 --- a/drivers/infiniband/core/nldev.c +++ b/drivers/infiniband/core/nldev.c @@ -95,8 +95,28 @@ [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 }, + [RDMA_NLDEV_ATTR_PAD] = { }, }; +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->provider_fill_res_entry ? + resroot->provider_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)) @@ -261,6 +281,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; @@ -310,6 +331,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; @@ -325,6 +349,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; @@ -366,6 +391,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; @@ -379,6 +407,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); @@ -399,6 +428,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; @@ -412,6 +444,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); @@ -434,6 +467,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; @@ -447,6 +483,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); @@ -473,6 +510,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 a56f4f2..8159395 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); + /** + * @provider_fill_res_entry: provider-specific fill function + * + * Allows rdma providers to add their own restrack attributes. + */ + int (*provider_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 351139c..576d2cc 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, @@ -387,6 +391,19 @@ 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_PAD, /* for 64b alignment */ 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 | 40 ++++++++++++++++++++++++++++++++++++++++ include/rdma/restrack.h | 10 ++++++++++ include/uapi/rdma/rdma_netlink.h | 17 +++++++++++++++++ 3 files changed, 67 insertions(+)