| Message ID | 20231009071801.10210-17-guoqing.jiang@linux.dev (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Cleanup for siw | expand |
> -----Original Message----- > From: Guoqing Jiang <guoqing.jiang@linux.dev> > Sent: Monday, October 9, 2023 9:18 AM > To: Bernard Metzler <BMT@zurich.ibm.com>; jgg@ziepe.ca; leon@kernel.org > Cc: linux-rdma@vger.kernel.org > Subject: [EXTERNAL] [PATCH 16/19] RDMA/siw: Remove siw_sk_assign_cm_upcalls > > Let's move it into siw_sk_save_upcalls, then we only need to > get sk_callback_lock once. Also rename siw_sk_save_upcalls > to better align with the new code. > > Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev> > --- > drivers/infiniband/sw/siw/siw_cm.c | 19 ++++++------------- > 1 file changed, 6 insertions(+), 13 deletions(-) > > diff --git a/drivers/infiniband/sw/siw/siw_cm.c > b/drivers/infiniband/sw/siw/siw_cm.c > index c3aa5533e75d..6866ec80473c 100644 > --- a/drivers/infiniband/sw/siw/siw_cm.c > +++ b/drivers/infiniband/sw/siw/siw_cm.c > @@ -39,17 +39,7 @@ static void siw_cm_llp_error_report(struct sock *s); > static int siw_cm_upcall(struct siw_cep *cep, enum iw_cm_event_type > reason, > int status); > > -static void siw_sk_assign_cm_upcalls(struct sock *sk) > -{ > - write_lock_bh(&sk->sk_callback_lock); > - sk->sk_state_change = siw_cm_llp_state_change; > - sk->sk_data_ready = siw_cm_llp_data_ready; > - sk->sk_write_space = siw_cm_llp_write_space; > - sk->sk_error_report = siw_cm_llp_error_report; > - write_unlock_bh(&sk->sk_callback_lock); > -} > - > -static void siw_sk_save_upcalls(struct sock *sk) To simplify, I'd suggest doing it the other way around, so having siw_sk_assign_cm_upcalls() including the functionality of siw_sk_save_upcalls() first. There is another function siw_sk_assign_rtr_upcalls(), which re-assigns the upcalls for special handling of an explicit RTR->RTS handshake if requested later during connection setup. > +static void siw_sk_save_and_assign_upcalls(struct sock *sk) > { > struct siw_cep *cep = sk_to_cep(sk); > > @@ -58,6 +48,10 @@ static void siw_sk_save_upcalls(struct sock *sk) > cep->sk_data_ready = sk->sk_data_ready; > cep->sk_write_space = sk->sk_write_space; > cep->sk_error_report = sk->sk_error_report; > + sk->sk_state_change = siw_cm_llp_state_change; > + sk->sk_data_ready = siw_cm_llp_data_ready; > + sk->sk_write_space = siw_cm_llp_write_space; > + sk->sk_error_report = siw_cm_llp_error_report; > write_unlock_bh(&sk->sk_callback_lock); > } > > @@ -156,8 +150,7 @@ static void siw_cep_socket_assoc(struct siw_cep *cep, > struct socket *s) > siw_cep_get(cep); > s->sk->sk_user_data = cep; > > - siw_sk_save_upcalls(s->sk); > - siw_sk_assign_cm_upcalls(s->sk); > + siw_sk_save_and_assign_upcalls(s->sk); > } > > static struct siw_cep *siw_cep_alloc(struct siw_device *sdev) > -- > 2.35.3
On 10/25/23 21:19, Bernard Metzler wrote: >> -----Original Message----- >> From: Guoqing Jiang<guoqing.jiang@linux.dev> >> Sent: Monday, October 9, 2023 9:18 AM >> To: Bernard Metzler<BMT@zurich.ibm.com>;jgg@ziepe.ca;leon@kernel.org >> Cc:linux-rdma@vger.kernel.org >> Subject: [EXTERNAL] [PATCH 16/19] RDMA/siw: Remove siw_sk_assign_cm_upcalls >> >> Let's move it into siw_sk_save_upcalls, then we only need to >> get sk_callback_lock once. Also rename siw_sk_save_upcalls >> to better align with the new code. >> >> Signed-off-by: Guoqing Jiang<guoqing.jiang@linux.dev> >> --- >> drivers/infiniband/sw/siw/siw_cm.c | 19 ++++++------------- >> 1 file changed, 6 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/infiniband/sw/siw/siw_cm.c >> b/drivers/infiniband/sw/siw/siw_cm.c >> index c3aa5533e75d..6866ec80473c 100644 >> --- a/drivers/infiniband/sw/siw/siw_cm.c >> +++ b/drivers/infiniband/sw/siw/siw_cm.c >> @@ -39,17 +39,7 @@ static void siw_cm_llp_error_report(struct sock *s); >> static int siw_cm_upcall(struct siw_cep *cep, enum iw_cm_event_type >> reason, >> int status); >> >> -static void siw_sk_assign_cm_upcalls(struct sock *sk) >> -{ >> - write_lock_bh(&sk->sk_callback_lock); >> - sk->sk_state_change = siw_cm_llp_state_change; >> - sk->sk_data_ready = siw_cm_llp_data_ready; >> - sk->sk_write_space = siw_cm_llp_write_space; >> - sk->sk_error_report = siw_cm_llp_error_report; >> - write_unlock_bh(&sk->sk_callback_lock); >> -} >> - >> -static void siw_sk_save_upcalls(struct sock *sk) > To simplify, I'd suggest doing it the other way around, > so having siw_sk_assign_cm_upcalls() including the > functionality of siw_sk_save_upcalls() first. I guess you mean below. If so, I will update it in v3. static void siw_sk_assign_cm_upcalls(struct sock *sk) -{ - write_lock_bh(&sk->sk_callback_lock); - sk->sk_state_change = siw_cm_llp_state_change; - sk->sk_data_ready = siw_cm_llp_data_ready; - sk->sk_write_space = siw_cm_llp_write_space; - sk->sk_error_report = siw_cm_llp_error_report; - write_unlock_bh(&sk->sk_callback_lock); -} - -static void siw_sk_save_upcalls(struct sock *sk) { struct siw_cep *cep = sk_to_cep(sk); @@ -58,6 +48,11 @@ static void siw_sk_save_upcalls(struct sock *sk) cep->sk_data_ready = sk->sk_data_ready; cep->sk_write_space = sk->sk_write_space; cep->sk_error_report = sk->sk_error_report; + + sk->sk_state_change = siw_cm_llp_state_change; + sk->sk_data_ready = siw_cm_llp_data_ready; + sk->sk_write_space = siw_cm_llp_write_space; + sk->sk_error_report = siw_cm_llp_error_report; write_unlock_bh(&sk->sk_callback_lock); } > There is another function siw_sk_assign_rtr_upcalls(), > which re-assigns the upcalls for special handling of > an explicit RTR->RTS handshake if requested later during > connection setup. Ah, one is assign rtr upcall and another is assign cm upcall. Thanks for the explanation. Guoqing
> -----Original Message----- > From: Guoqing Jiang <guoqing.jiang@linux.dev> > Sent: Thursday, October 26, 2023 8:46 AM > To: Bernard Metzler <BMT@zurich.ibm.com>; jgg@ziepe.ca; leon@kernel.org > Cc: linux-rdma@vger.kernel.org > Subject: [EXTERNAL] Re: [PATCH 16/19] RDMA/siw: Remove > siw_sk_assign_cm_upcalls > > > > On 10/25/23 21:19, Bernard Metzler wrote: > >> -----Original Message----- > >> From: Guoqing Jiang<guoqing.jiang@linux.dev> > >> Sent: Monday, October 9, 2023 9:18 AM > >> To: Bernard Metzler<BMT@zurich.ibm.com>;jgg@ziepe.ca;leon@kernel.org > >> Cc:linux-rdma@vger.kernel.org > >> Subject: [EXTERNAL] [PATCH 16/19] RDMA/siw: Remove > siw_sk_assign_cm_upcalls > >> > >> Let's move it into siw_sk_save_upcalls, then we only need to > >> get sk_callback_lock once. Also rename siw_sk_save_upcalls > >> to better align with the new code. > >> > >> Signed-off-by: Guoqing Jiang<guoqing.jiang@linux.dev> > >> --- > >> drivers/infiniband/sw/siw/siw_cm.c | 19 ++++++------------- > >> 1 file changed, 6 insertions(+), 13 deletions(-) > >> > >> diff --git a/drivers/infiniband/sw/siw/siw_cm.c > >> b/drivers/infiniband/sw/siw/siw_cm.c > >> index c3aa5533e75d..6866ec80473c 100644 > >> --- a/drivers/infiniband/sw/siw/siw_cm.c > >> +++ b/drivers/infiniband/sw/siw/siw_cm.c > >> @@ -39,17 +39,7 @@ static void siw_cm_llp_error_report(struct sock *s); > >> static int siw_cm_upcall(struct siw_cep *cep, enum iw_cm_event_type > >> reason, > >> int status); > >> > >> -static void siw_sk_assign_cm_upcalls(struct sock *sk) > >> -{ > >> - write_lock_bh(&sk->sk_callback_lock); > >> - sk->sk_state_change = siw_cm_llp_state_change; > >> - sk->sk_data_ready = siw_cm_llp_data_ready; > >> - sk->sk_write_space = siw_cm_llp_write_space; > >> - sk->sk_error_report = siw_cm_llp_error_report; > >> - write_unlock_bh(&sk->sk_callback_lock); > >> -} > >> - > >> -static void siw_sk_save_upcalls(struct sock *sk) > > To simplify, I'd suggest doing it the other way around, > > so having siw_sk_assign_cm_upcalls() including the > > functionality of siw_sk_save_upcalls() first. > > I guess you mean below. If so, I will update it in v3. right. assigning the cm upcalls is the first step and needs the original socket callbacks saved. Later, maybe we have to reassign the upcalls to execute the additional RTS handshake. > > static void siw_sk_assign_cm_upcalls(struct sock *sk) > -{ > - write_lock_bh(&sk->sk_callback_lock); > - sk->sk_state_change = siw_cm_llp_state_change; > - sk->sk_data_ready = siw_cm_llp_data_ready; > - sk->sk_write_space = siw_cm_llp_write_space; > - sk->sk_error_report = siw_cm_llp_error_report; > - write_unlock_bh(&sk->sk_callback_lock); > -} > - > -static void siw_sk_save_upcalls(struct sock *sk) > { > struct siw_cep *cep = sk_to_cep(sk); > > @@ -58,6 +48,11 @@ static void siw_sk_save_upcalls(struct sock *sk) > cep->sk_data_ready = sk->sk_data_ready; > cep->sk_write_space = sk->sk_write_space; > cep->sk_error_report = sk->sk_error_report; > + > + sk->sk_state_change = siw_cm_llp_state_change; > + sk->sk_data_ready = siw_cm_llp_data_ready; > + sk->sk_write_space = siw_cm_llp_write_space; > + sk->sk_error_report = siw_cm_llp_error_report; > write_unlock_bh(&sk->sk_callback_lock); > } > > > There is another function siw_sk_assign_rtr_upcalls(), > > which re-assigns the upcalls for special handling of > > an explicit RTR->RTS handshake if requested later during > > connection setup. > > Ah, one is assign rtr upcall and another is assign cm upcall. > Thanks for the explanation. > > Guoqing
diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c index c3aa5533e75d..6866ec80473c 100644 --- a/drivers/infiniband/sw/siw/siw_cm.c +++ b/drivers/infiniband/sw/siw/siw_cm.c @@ -39,17 +39,7 @@ static void siw_cm_llp_error_report(struct sock *s); static int siw_cm_upcall(struct siw_cep *cep, enum iw_cm_event_type reason, int status); -static void siw_sk_assign_cm_upcalls(struct sock *sk) -{ - write_lock_bh(&sk->sk_callback_lock); - sk->sk_state_change = siw_cm_llp_state_change; - sk->sk_data_ready = siw_cm_llp_data_ready; - sk->sk_write_space = siw_cm_llp_write_space; - sk->sk_error_report = siw_cm_llp_error_report; - write_unlock_bh(&sk->sk_callback_lock); -} - -static void siw_sk_save_upcalls(struct sock *sk) +static void siw_sk_save_and_assign_upcalls(struct sock *sk) { struct siw_cep *cep = sk_to_cep(sk); @@ -58,6 +48,10 @@ static void siw_sk_save_upcalls(struct sock *sk) cep->sk_data_ready = sk->sk_data_ready; cep->sk_write_space = sk->sk_write_space; cep->sk_error_report = sk->sk_error_report; + sk->sk_state_change = siw_cm_llp_state_change; + sk->sk_data_ready = siw_cm_llp_data_ready; + sk->sk_write_space = siw_cm_llp_write_space; + sk->sk_error_report = siw_cm_llp_error_report; write_unlock_bh(&sk->sk_callback_lock); } @@ -156,8 +150,7 @@ static void siw_cep_socket_assoc(struct siw_cep *cep, struct socket *s) siw_cep_get(cep); s->sk->sk_user_data = cep; - siw_sk_save_upcalls(s->sk); - siw_sk_assign_cm_upcalls(s->sk); + siw_sk_save_and_assign_upcalls(s->sk); } static struct siw_cep *siw_cep_alloc(struct siw_device *sdev)
Let's move it into siw_sk_save_upcalls, then we only need to get sk_callback_lock once. Also rename siw_sk_save_upcalls to better align with the new code. Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev> --- drivers/infiniband/sw/siw/siw_cm.c | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-)