On Mon, Jul 04, 2016 at 03:40:10PM +0300, Or Gerlitz wrote: > On Mon, Jul 4, 2016 at 7:51 AM, Leon Romanovsky 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 wrote: > >> > From: Alex Vesker > >> > > >> > >> > 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.