[PATCH rdma-core] srp_daemon: Remove duplicate sync_res shutdown

[PATCH rdma-core] srp_daemon: Remove duplicate sync_res shutdown

[Jason Gunthorpe]

The very first thing free_res does is the same lock/signal/unlock
sequence, so there is no reason to open code it before calling
free_res.

Signed-off-by: Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 srp_daemon/srp_daemon.c | 4 ----
 1 file changed, 4 deletions(-)

Noticed while looking at honli's patch.. Didn't make a PR.

diff --git a/srp_daemon/srp_daemon.c b/srp_daemon/srp_daemon.c
index cec36db2e0f12e..a1798558373490 100644
--- a/srp_daemon/srp_daemon.c
+++ b/srp_daemon/srp_daemon.c
@@ -2074,10 +2074,6 @@ static int ibsrpdm(int argc, char *argv[])
 		pr_err("Querying SRP targets failed\n");
 
 	assert(res->sync_res);
-	pthread_mutex_lock(&res->sync_res->retry_mutex);
-	res->sync_res->stop_threads = 1;
-	pthread_cond_signal(&res->sync_res->retry_cond);
-	pthread_mutex_unlock(&res->sync_res->retry_mutex);
 
 	free_res(res);
 umad_done:
-- 
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-core] srp_daemon: Remove duplicate sync_res shutdown

[Bart Van Assche]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1267 bytes --]

On Tue, 2017-12-19 at 14:16 -0700, Jason Gunthorpe wrote:
> The very first thing free_res does is the same lock/signal/unlock
> sequence, so there is no reason to open code it before calling
> free_res.
> 
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> ---
>  srp_daemon/srp_daemon.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> Noticed while looking at honli's patch.. Didn't make a PR.
> 
> diff --git a/srp_daemon/srp_daemon.c b/srp_daemon/srp_daemon.c
> index cec36db2e0f12e..a1798558373490 100644
> --- a/srp_daemon/srp_daemon.c
> +++ b/srp_daemon/srp_daemon.c
> @@ -2074,10 +2074,6 @@ static int ibsrpdm(int argc, char *argv[])
>  		pr_err("Querying SRP targets failed\n");
>  
>  	assert(res->sync_res);
> -	pthread_mutex_lock(&res->sync_res->retry_mutex);
> -	res->sync_res->stop_threads = 1;
> -	pthread_cond_signal(&res->sync_res->retry_cond);
> -	pthread_mutex_unlock(&res->sync_res->retry_mutex);

Hello Jason,

Since this patch removes all code that dereferences res->sync_res from
the ibsrpdm() function, have you considered to remove assert(res->sync_res)
too?

Thanks,

Bart.N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±­ÙšŠ{ayº\x1dʇڙë,j\a­¢f£¢·hš‹»öì\x17/oSc¾™Ú³9˜uÀ¦æå‰È&jw¨®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿïêäz¹Þ–Šàþf£¢·hšˆ§~ˆmš

Re: [PATCH rdma-core] srp_daemon: Remove duplicate sync_res shutdown

[Jason Gunthorpe]

On Tue, Dec 19, 2017 at 09:21:05PM +0000, Bart Van Assche wrote:
> On Tue, 2017-12-19 at 14:16 -0700, Jason Gunthorpe wrote:
> > The very first thing free_res does is the same lock/signal/unlock
> > sequence, so there is no reason to open code it before calling
> > free_res.
> > 
> > Signed-off-by: Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >  srp_daemon/srp_daemon.c | 4 ----
> >  1 file changed, 4 deletions(-)
> > 
> > Noticed while looking at honli's patch.. Didn't make a PR.
> > 
> > diff --git a/srp_daemon/srp_daemon.c b/srp_daemon/srp_daemon.c
> > index cec36db2e0f12e..a1798558373490 100644
> > +++ b/srp_daemon/srp_daemon.c
> > @@ -2074,10 +2074,6 @@ static int ibsrpdm(int argc, char *argv[])
> >  		pr_err("Querying SRP targets failed\n");
> >  
> >  	assert(res->sync_res);
> > -	pthread_mutex_lock(&res->sync_res->retry_mutex);
> > -	res->sync_res->stop_threads = 1;
> > -	pthread_cond_signal(&res->sync_res->retry_cond);
> > -	pthread_mutex_unlock(&res->sync_res->retry_mutex);
> 
> Hello Jason,
> 
> Since this patch removes all code that dereferences res->sync_res from
> the ibsrpdm() function, have you considered to remove assert(res->sync_res)
> too?

Well, that would be a behavior change.. free_res works OK if sync_res
is NULL, so I'm guessing that the assert had some meaning to show that
the reconnect_thread was always started for ibsrpd?

Or maybe we shouldn't care about that?

Happy to drop it if you think it makes sense.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-core] srp_daemon: Remove duplicate sync_res shutdown

[Bart Van Assche]

On Tue, 2017-12-19 at 14:28 -0700, Jason Gunthorpe wrote:
> On Tue, Dec 19, 2017 at 09:21:05PM +0000, Bart Van Assche wrote:
> > On Tue, 2017-12-19 at 14:16 -0700, Jason Gunthorpe wrote:
> > > The very first thing free_res does is the same lock/signal/unlock
> > > sequence, so there is no reason to open code it before calling
> > > free_res.
> > > 
> > > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> > >  srp_daemon/srp_daemon.c | 4 ----
> > >  1 file changed, 4 deletions(-)
> > > 
> > > Noticed while looking at honli's patch.. Didn't make a PR.
> > > 
> > > diff --git a/srp_daemon/srp_daemon.c b/srp_daemon/srp_daemon.c
> > > index cec36db2e0f12e..a1798558373490 100644
> > > +++ b/srp_daemon/srp_daemon.c
> > > @@ -2074,10 +2074,6 @@ static int ibsrpdm(int argc, char *argv[])
> > >  		pr_err("Querying SRP targets failed\n");
> > >  
> > >  	assert(res->sync_res);
> > > -	pthread_mutex_lock(&res->sync_res->retry_mutex);
> > > -	res->sync_res->stop_threads = 1;
> > > -	pthread_cond_signal(&res->sync_res->retry_cond);
> > > -	pthread_mutex_unlock(&res->sync_res->retry_mutex);
> > 
> > Hello Jason,
> > 
> > Since this patch removes all code that dereferences res->sync_res from
> > the ibsrpdm() function, have you considered to remove assert(res->sync_res)
> > too?
> 
> Well, that would be a behavior change.. free_res works OK if sync_res
> is NULL, so I'm guessing that the assert had some meaning to show that
> the reconnect_thread was always started for ibsrpd?
> 
> Or maybe we shouldn't care about that?
> 
> Happy to drop it if you think it makes sense.

Hello Jason,

If the alloc_res() return value is not NULL then that implies that res->sync_res
!= NULL. The assert() I referred to is only reached if alloc_res() succeeded. So
I think it it is safe to leave out that assert() statement.

Bart.