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