diff mbox

[rdma-core] srp_daemon: Remove duplicate sync_res shutdown

Message ID 20171219211601.GA27412@ziepe.ca (mailing list archive)
State Superseded
Headers show

Commit Message

Jason Gunthorpe Dec. 19, 2017, 9:16 p.m. UTC
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.

Comments

Bart Van Assche Dec. 19, 2017, 9:21 p.m. UTC | #1
T24gVHVlLCAyMDE3LTEyLTE5IGF0IDE0OjE2IC0wNzAwLCBKYXNvbiBHdW50aG9ycGUgd3JvdGU6
DQo+IFRoZSB2ZXJ5IGZpcnN0IHRoaW5nIGZyZWVfcmVzIGRvZXMgaXMgdGhlIHNhbWUgbG9jay9z
aWduYWwvdW5sb2NrDQo+IHNlcXVlbmNlLCBzbyB0aGVyZSBpcyBubyByZWFzb24gdG8gb3BlbiBj
b2RlIGl0IGJlZm9yZSBjYWxsaW5nDQo+IGZyZWVfcmVzLg0KPiANCj4gU2lnbmVkLW9mZi1ieTog
SmFzb24gR3VudGhvcnBlIDxqZ2dAbWVsbGFub3guY29tPg0KPiAtLS0NCj4gIHNycF9kYWVtb24v
c3JwX2RhZW1vbi5jIHwgNCAtLS0tDQo+ICAxIGZpbGUgY2hhbmdlZCwgNCBkZWxldGlvbnMoLSkN
Cj4gDQo+IE5vdGljZWQgd2hpbGUgbG9va2luZyBhdCBob25saSdzIHBhdGNoLi4gRGlkbid0IG1h
a2UgYSBQUi4NCj4gDQo+IGRpZmYgLS1naXQgYS9zcnBfZGFlbW9uL3NycF9kYWVtb24uYyBiL3Ny
cF9kYWVtb24vc3JwX2RhZW1vbi5jDQo+IGluZGV4IGNlYzM2ZGIyZTBmMTJlLi5hMTc5ODU1ODM3
MzQ5MCAxMDA2NDQNCj4gLS0tIGEvc3JwX2RhZW1vbi9zcnBfZGFlbW9uLmMNCj4gKysrIGIvc3Jw
X2RhZW1vbi9zcnBfZGFlbW9uLmMNCj4gQEAgLTIwNzQsMTAgKzIwNzQsNiBAQCBzdGF0aWMgaW50
IGlic3JwZG0oaW50IGFyZ2MsIGNoYXIgKmFyZ3ZbXSkNCj4gIAkJcHJfZXJyKCJRdWVyeWluZyBT
UlAgdGFyZ2V0cyBmYWlsZWRcbiIpOw0KPiAgDQo+ICAJYXNzZXJ0KHJlcy0+c3luY19yZXMpOw0K
PiAtCXB0aHJlYWRfbXV0ZXhfbG9jaygmcmVzLT5zeW5jX3Jlcy0+cmV0cnlfbXV0ZXgpOw0KPiAt
CXJlcy0+c3luY19yZXMtPnN0b3BfdGhyZWFkcyA9IDE7DQo+IC0JcHRocmVhZF9jb25kX3NpZ25h
bCgmcmVzLT5zeW5jX3Jlcy0+cmV0cnlfY29uZCk7DQo+IC0JcHRocmVhZF9tdXRleF91bmxvY2so
JnJlcy0+c3luY19yZXMtPnJldHJ5X211dGV4KTsNCg0KSGVsbG8gSmFzb24sDQoNClNpbmNlIHRo
aXMgcGF0Y2ggcmVtb3ZlcyBhbGwgY29kZSB0aGF0IGRlcmVmZXJlbmNlcyByZXMtPnN5bmNfcmVz
IGZyb20NCnRoZSBpYnNycGRtKCkgZnVuY3Rpb24sIGhhdmUgeW91IGNvbnNpZGVyZWQgdG8gcmVt
b3ZlIGFzc2VydChyZXMtPnN5bmNfcmVzKQ0KdG9vPw0KDQpUaGFua3MsDQoNCkJhcnQu
--
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
Jason Gunthorpe Dec. 19, 2017, 9:28 p.m. UTC | #2
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.

Jason
--
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
Bart Van Assche Dec. 19, 2017, 9:33 p.m. UTC | #3
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.
diff mbox

Patch

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: