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