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@xxxxxxxxxxxxxxxxxxxxx>
> > > ---
> > > 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
>
Attachment:
signature.asc
Description: PGP signature
