| Message ID | 20171219211601.GA27412@ziepe.ca (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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 --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:
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.