diff mbox

[RFC,rdma-next,1/2] RDMA/nldev: add provider-specific resource tracking

Message ID 70bcad28319c90ce1699cce1a52fc80350fd5bb8.1521046858.git.swise@opengridcomputing.com (mailing list archive)
State RFC
Headers show

Commit Message

Steve Wise March 13, 2018, 5:30 p.m. UTC
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(+)

Comments

Leon Romanovsky March 14, 2018, 5:33 p.m. UTC | #1
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
Jason Gunthorpe March 14, 2018, 5:42 p.m. UTC | #2
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
Steve Wise March 14, 2018, 5:58 p.m. UTC | #3
> 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
Steve Wise March 14, 2018, 6 p.m. UTC | #4
> 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
Leon Romanovsky March 14, 2018, 6:49 p.m. UTC | #5
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.
>
Leon Romanovsky March 15, 2018, 7:50 a.m. UTC | #6
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
Steve Wise March 15, 2018, 4:09 p.m. UTC | #7
> 
> 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
Leon Romanovsky March 18, 2018, 7:55 a.m. UTC | #8
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
>
Steve Wise March 19, 2018, 7:37 p.m. UTC | #9
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 mbox

Patch

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
 };