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?
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
