On 18/9/2013 1:07 PM, Yann Droneaud wrote:
Hi,
Le 18.09.2013 10:40, Matan Barak a écrit :
On 17/9/2013 6:43 PM, Yann Droneaud wrote:
Le 17.09.2013 17:13, Matan Barak a écrit :
On 17/9/2013 1:25 PM, Yann Droneaud wrote:
Hi,
Le 17.09.2013 12:02, Matan Barak a écrit :
That's right - we're not checking anything here.
struct ib_uverbs_qp_attr_ex contains 2 user pointers:
+ /* represents: struct ib_uverbs_ah_attr_ex * __user */
+ void __user *ah_attr_ex;
+ /* represents: struct ib_uverbs_ah_attr_ex * __user */
+ void __user *alt_ah_attr_ex;
Those pointers should be given to rdma_init_qp_attr in order to
initialize them.
We are using pointers here in order to maintain future extendability
of the address handle structures.
First: you can't put pointer asis in public data structure. Look to
all
others "command" structures
declared in include/uapi/rdma/ib_user_verbs.h
Thanks for the review. Looking at other commands, I see that pointers
(such a the response) are passed as __u64 at the command structure.
Indeed. So that 32bits and 64bits binaries use the same layout.
Is that what you mean ? I think it's a bit odd to pass those
pointers as
a part of the command, as they are output only attributes. Though,
I'll change the code to use __u64 instead of the actual __user
pointers.
How can those pointers be output parameters ? Does kernel allocate some
pages
and putting the addresses of those in the ah_attr_ex and alt_ah_attr_ex
pointers ?
No, the user allocates them and passes the addresses to the kernel.
The kernel fills the user-allocated space. It works the exact same way
as the response structure.
So it's not "odd to pass those pointers as part of the command" :)
The commands structure hold all the information needed to process it.
I've seen that the proposed command has pointers to others user memory
buffers
not part of the write() operations ... just like some ucma commands :(
This make the kernel do userspace pointer chasing ... it's not good
from maintainability point of view: it's adding complexity.
The response/cmd buffer are already userspace pointers, thus the kernel
is already doing some userspace pointer chasing. Though, it might be
better to use "iovec like" behavior by putting several output buffers in
the command. What do you think about moving the
ah_attr_ex/alt_ah_attr_ex into the command ?
I don't buy the "maintain future extendability" argument for such
specific cruft.
It's not enough generic to fall in the extensible pattern.
I don't see why.
struct inner1 {
... some fields ...
};
struct inner2 {
... some fields ...
};
struct outer {
... some fields ...
struct inner1 in1;
struct inner2 in2;
... some fields ...
};
Now lets say we need to extend inner1 with a new field in its bottom.
If inner1 was inlined, outer memory layout would be changed and we'll
get problems with new user <-> old kernel. That's a general problem
that can be solved easily by using:
struct outer {
... some fields ...
struct inner1 *in1;
struct inner2 *in2;
... some fields ...
};
Since we're using __u64 to pass pointers, instead of struct
inner[1-2]* we could use __u64. That's ok.
In our case the only usage of in[1-2] is as output parameters. The
user allocates space only to let the kernel fills it for him.
That's why I think putting it as a part of the response is more
appropriate.
[ Currently "response" is write only. It's cleaner that way ]
The scheme presented, as I read here, is starting to look like a forest
of pointers,
a recursive octopus... and I don't like those.
I think of something like NETLINK and IOVEC as a better scheme (quite
like a TLV things)
But using such scheme would introduce a new ABI ... and came with its
own set of complexity problems.
Currently, we only have 2 levels of indirections. I don't think other
commands will require the usage of pointers. Anyway, moving those
pointers into the command buffer will flatten this tree into one level.
I don't think that using other methods, which currently aren't used in
uverbs, is something we should introduce without carefully examining the
possible implications.
Same apply to struct ib_uverbs_modify_qp_ex, but struct
ib_uverbs_modify_qp_ex has the comp_mask as first field (introducing a
hole).
We'll add a reserved field to fix this hole. Thanks for that catch!
Why not putting that field after the struct ib_uverbs_modify_qp field ?
(even if I don't like the comp_mask things, I waiting for a real world
example).
Since every struct starts with u32 comp_mask, I suggest putting
required reserved field right after this comp_mask. I've reviewed this
case again and I don't think a reserved field is needed here. The
largest field in struct ib_uverbs_qp_dest is u32. Hence, the struct
will be 32bit aligned. Since struct ib_uverbs_qp_dest shouldn't be
changed anymore, as it'll change the memory layout, there's no reason
to force struct ib_uverbs_qp_dest to be 64bit aligned.
Yes you're right.
Perhaps I was thinkig of ib_uverbs_create_ah_ex ?
create_ah_ex will be removed in the next version if ip based addressing
patchset :-)
Regards.
Regards
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
