| Message ID | CAJ3xEMgErk9Gu8g98dxkdjRP6aiq6hdkAOPj-a2zKLSTwaGZ0w@mail.gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Mon, Jul 04, 2016 at 03:40:10PM +0300, Or Gerlitz wrote: > On Mon, Jul 4, 2016 at 7:51 AM, Leon Romanovsky <leonro@mellanox.com> wrote: > > On Sun, Jul 03, 2016 at 04:46:23PM +0300, Or Gerlitz wrote: > >> On Sun, Jul 3, 2016 at 3:47 PM, Leon Romanovsky <leon@kernel.org> wrote: > >> > From: Alex Vesker <valex@mellanox.com> > >> > > >> > >> > drivers/infiniband/core/cma.c | 98 +++++++++++++++++++++++++++++++++++++--- > >> > drivers/infiniband/core/ucma.c | 18 ++++++-- > >> > include/rdma/ib_sa.h | 5 ++ > >> > include/rdma/rdma_cm.h | 4 +- > >> > include/uapi/rdma/rdma_user_cm.h | 9 +++- > >> > 5 files changed, 122 insertions(+), 12 deletions(-) > >> > >> > >> For the ease/robustness of review for UAPI changes, we have a long > >> time common practice > >> to break things like this one to IB core kernel only patch, and one > >> that deals the user-space > > > You are right, this practice exists and we are following it as much as it makes sense. > > This specific case doesn't need to be separated, because it introduces one logical > > change and separate patches will be useless as standalone patches. > > Leon, > > The point is that you need to get people to be used to that practice, > and it seems we're not doing that. Otherwise I wouldn't have to chat > for 20m with 2-3 people that wonder why I made these comments. I think > we should require it from the developers, period and not argue on > that. B/c in bunch of other places, it is totally required, for > example, here you could just carve this small piece to be part of your > UAPI patch, what's wrong with that? > > --- a/include/uapi/rdma/rdma_user_cm.h > +++ b/include/uapi/rdma/rdma_user_cm.h > @@ -244,12 +244,19 @@ struct rdma_ucm_join_ip_mcast { > __u32 id; > }; > > struct rdma_ucm_join_mcast { > __u64 response; /* rdma_ucma_create_id_resp */ > __u64 uid; > __u32 id; > __u16 addr_size; > - __u16 reserved; > + __u16 join_flags; > struct sockaddr_storage addr; > }; The main issue here that new code is using this struct and new field while the old code uses reserved field.
On Tue, Jul 5, 2016 at 1:45 PM, Leon Romanovsky <leonro@mellanox.com> wrote: > On Mon, Jul 04, 2016 at 03:40:10PM +0300, Or Gerlitz wrote: >> +++ b/include/uapi/rdma/rdma_user_cm.h >> @@ -244,12 +244,19 @@ struct rdma_ucm_join_ip_mcast { >> __u32 id; >> }; >> >> struct rdma_ucm_join_mcast { >> __u64 response; /* rdma_ucma_create_id_resp */ >> __u64 uid; >> __u32 id; >> __u16 addr_size; >> - __u16 reserved; >> + __u16 join_flags; >> struct sockaddr_storage addr; >> }; > The main issue here that new code is using this struct and new field while the old > code uses reserved field. If per your judgment, in this case we can't break things for UAPI vs non-UAPI, let have an exception and move on :) Or. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- a/include/uapi/rdma/rdma_user_cm.h +++ b/include/uapi/rdma/rdma_user_cm.h @@ -244,12 +244,19 @@ struct rdma_ucm_join_ip_mcast { __u32 id; }; struct rdma_ucm_join_mcast { __u64 response; /* rdma_ucma_create_id_resp */ __u64 uid; __u32 id; __u16 addr_size; - __u16 reserved; + __u16 join_flags; struct sockaddr_storage addr; }; basically it would be good to have the ucma part there as well, but if you and Co can't come up with patch planning that allows that is bisects, lets it be just the header file. This way we come with strict message to the developers that we want them to break patches to non-UAPI and yes-UAPI + user-space facing and have them to stop argue and complain.