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.
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.
Second: if you're duplicating struct ib_uverbs_qp_attr, why not include
it in struct ib_uverbs_qp_attr_ex
it will reduce maintenance burden, clutter, etc.
Or drop the unused fields in the _ex version while taking care of being
at least equal size than
the original version.
The extension verbs approach should be an evolution. Instead of using
a cumbersome extended.basic.field notation, extended.field is more
compact and readable. Using this notation will allow us to deprecate
the basic structures when they won't be in use anymore.
Since the basic structures shouldn't change anymore as user
applications relay on them, I don't see a burden in maintainability
doing it this way.
Oh ... I'm waiting to see qp_attr being deprecated</sarcasm>
Wasn't 386 arch deprecated after ~20 years?
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.
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
