srp_daemon: Install signal handler for ibsrpdm

From: Honggang Li <[email protected]>

[root@localhost ~]# /usr/sbin/ibsrpdm -c -d /dev/infiniband/umad0 id_ext=0002c90300317810,ioc_guid=0002c90300317810,dgid=fe800000000000000002c90300317811,pkey=ffff,service_id=0002c90300317810

[root@localhost ~]# echo $?
130

fd3005f0cd34 moves the signal handler setup from ibsrpdm path. So, default signal handler will be used when it receives signal SIGINT. ibsrpdm will exit with non-zero exit code as default signal handler killed it. ibsrpdm should return with exit code zero, if no error emerged.

main->ibsrpdm->free_res->pthread_kill(res->async_ev_thread, SIGINT)

Beside the wrong exit code, there is an extra blank line at the end of output of ibsrpdm.

---
 srp_daemon/srp_daemon.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

index cec36db2..0d00d712 100644
@@ -1996,7 +1996,6 @@ static void cleanup_wakeup_fd(void)
 
 static int setup_wakeup_fd(void)
 {
-	struct sigaction sa = {};
 	int ret;
 
 	ret = pipe2(wakeup_pipe, O_NONBLOCK | O_CLOEXEC);

@@ -2005,11 +2004,6 @@ static int setup_wakeup_fd(void)
 		return -1;
 	}
 
-	sigemptyset(&sa.sa_mask);
-	sa.sa_handler = signal_handler;
-	sigaction(SIGINT, &sa, NULL);
-	sigaction(SIGTERM, &sa, NULL);
-	sigaction(SRP_CATAS_ERR, &sa, NULL);
 	return 0;
 }
 

@@ -2100,6 +2094,7 @@ int main(int argc, char *argv[])
 	int			lockfd = -1;
 	int			received_signal = 0;
 	bool                    systemd;
+	struct sigaction        sa = {};
 
 #ifndef __CHECKER__
 	/*

@@ -2117,6 +2112,12 @@ int main(int argc, char *argv[])
 	BUILD_ASSERT(sizeof(struct srp_dm_iou_info) == 132);
 	BUILD_ASSERT(sizeof(struct srp_dm_ioc_prof) == 128);
 
+	sigemptyset(&sa.sa_mask);
+	sa.sa_handler = signal_handler;
+	sigaction(SIGINT, &sa, NULL);
+	sigaction(SIGTERM, &sa, NULL);
+	sigaction(SRP_CATAS_ERR, &sa, NULL);
+
 	if (strcmp(argv[0] + max_t(int, 0, strlen(argv[0]) - strlen("ibsrpdm")),
 		   "ibsrpdm") == 0) {
 		ret = ibsrpdm(argc, argv);

2.14.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

T24gVGh1LCAyMDE3LTEyLTE0IGF0IDE5OjAyICswODAwLCBIb25nZ2FuZyBMSSB3cm90ZToNCj4g ZmQzMDA1ZjBjZDM0IG1vdmVzIHRoZSBzaWduYWwgaGFuZGxlciBzZXR1cCBmcm9tIGlic3JwZG0g cGF0aC4gU28sDQo+IGRlZmF1bHQgc2lnbmFsIGhhbmRsZXIgd2lsbCBiZSB1c2VkIHdoZW4gaXQg cmVjZWl2ZXMgc2lnbmFsIFNJR0lOVC4NCj4gaWJzcnBkbSB3aWxsIGV4aXQgd2l0aCBub24temVy byBleGl0IGNvZGUgYXMgZGVmYXVsdCBzaWduYWwgaGFuZGxlcg0KPiBraWxsZWQgaXQuDQoNCkNh biB5b3UgZXhwbGFpbiB3aHkgeW91IHRoaW5rIHRoYXQgaWJzcnBkbSBuZWVkcyBhIHNpZ25hbCBo YW5kbGVyPw0KDQo+IGlic3JwZG0gc2hvdWxkIHJldHVybiB3aXRoIGV4aXQgY29kZSB6ZXJvLCBp ZiBubyBlcnJvciBlbWVyZ2VkLg0KDQpUaGF0J3MgY29ycmVjdCwgYnV0IGlmIGl0IHdhcyBpbnRl cnJ1cHRlZCBieSBhIHNpZ25hbCBoYW5kbGVyIGFuIGVycm9yDQpjb2RlIHNob3VsZCBiZSByZXR1 cm5lZC4NCg0KQmFydC4=
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

main
ibsrpdm

    alloc_res
      pthread_create(&res->res.async_ev_thread [1]
    ....
    free_res
      if (res->async_ev_thread) { pthread_kill(res->async_ev_thread, SIGINT); [2]

The ibsrpdm program create a thread in [1], and send a SIGINT in [2]. The default behavior of SIGINT is to terminate the process.

According to the man page of pthread_kill:
Signal dispositions are process-wide: if a signal handler is installed, the handler will be invoked in the thread _thread_, but if the disposition of the signal is "stop", "continue", or "terminate", this action will affect the whole process.

If we did not install a signal handler for ibsrpdm, ibsrpdm will be terminated in [2], when it free the resource. ibsrpdm expected to return to the main program after it free the resource. However, it failed as it killed by default signal handler.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Yuk, no, using signals like this is horrifyingly buggy.

Here is a sketch on how to fix it properly. All the users of pthread_kill should be eliminated.

Though overall, there is really no reason to even cleanup the threads, just call exit?

index cec36db2e0f12e..fe307705ad6a51 100644
@@ -61,6 +61,7 @@
 #include <string.h>
 #include <signal.h>
 #include <sys/syslog.h>
+#include <sys/eventfd.h>
 #include <infiniband/umad.h>
 #include <infiniband/umad_types.h>
 #include <infiniband/umad_sa.h>

@@ -1887,7 +1888,9 @@ static void free_res(struct resources *res)
 		modify_qp_to_err(res->ud_res->qp);
 
 	if (res->reconnect_thread) {
-		pthread_kill(res->reconnect_thread, SIGINT);
+		uint64_t val = 1;
+
+		write(res->sync_res->stop_event_fd, &val, sizeof(val));
 		pthread_join(res->reconnect_thread, &status);
 	}
 	if (res->async_ev_thread) {

@@ -1947,6 +1950,7 @@ static struct resources *alloc_res(void)
 			goto err;
 	}
 
+	res->sync_res.stop_event_fd = eventfd(0, EFD_CLOEXEC);
 	ret = pthread_create(&res->res.async_ev_thread, NULL,
 			     run_thread_listen_to_events, &res->res);
 	if (ret)

index 5d268ed395e17e..8cbee09d7c688b 100644
@@ -245,6 +245,7 @@ enum {
 };
 
 struct sync_resources {
+	int stop_event_fd;
 	int stop_threads;
 	int next_task;
 	struct timespec next_recalc_time;

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

T24gRnJpLCAyMDE3LTEyLTE1IGF0IDEwOjI4IC0wNzAwLCBKYXNvbiBHdW50aG9ycGUgd3JvdGU6 DQo+IFRob3VnaCBvdmVyYWxsLCB0aGVyZSBpcyByZWFsbHkgbm8gcmVhc29uIHRvIGV2ZW4gY2xl YW51cCB0aGUgdGhyZWFkcywNCj4ganVzdCBjYWxsIGV4aXQ/DQoNCk1lbW9yeSBsZWFrIGRldGVj dG9ycyBsaWtlIHRoZSBvbmUgaW4gVmFsZ3JpbmQgb25seSBwcm9kdWNlIG1lYW5pbmdmdWwgcmVz dWx0cw0KaWYgdGhyZWFkcyBhcmUgc3RvcHBlZCBjbGVhbmx5IGJlZm9yZSBleGl0KCkgaXMgY2Fs bGVkLg0KDQo+IEBAIC0xODg3LDcgKzE4ODgsOSBAQCBzdGF0aWMgdm9pZCBmcmVlX3JlcyhzdHJ1 Y3QgcmVzb3VyY2VzICpyZXMpDQo+ICAJCW1vZGlmeV9xcF90b19lcnIocmVzLT51ZF9yZXMtPnFw KTsNCj4gIA0KPiAgCWlmIChyZXMtPnJlY29ubmVjdF90aHJlYWQpIHsNCj4gLQkJcHRocmVhZF9r aWxsKHJlcy0+cmVjb25uZWN0X3RocmVhZCwgU0lHSU5UKTsNCj4gKwkJdWludDY0X3QgdmFsID0g MTsNCj4gKw0KPiArCQl3cml0ZShyZXMtPnN5bmNfcmVzLT5zdG9wX2V2ZW50X2ZkLCAmdmFsLCBz aXplb2YodmFsKSk7DQo+ICAJCXB0aHJlYWRfam9pbihyZXMtPnJlY29ubmVjdF90aHJlYWQsICZz dGF0dXMpOw0KDQpUaGlzIHBhdGNoIGRvZXMgbm90IHJlbW92ZSBhbGwgcHRocmVhZF9raWxsKCkg Y2FsbHMuIFRoZXJlIGFyZSB0d28gb3RoZXIgc3VjaA0KY2FsbHM6DQoNCiQgZ2l0IGdyZXAgLW5I dyBwdGhyZWFkX2tpbGwNCnNycF9kYWVtb24vc3JwX2RhZW1vbi5jOjE4OTc6CQlwdGhyZWFkX2tp bGwocmVzLT5hc3luY19ldl90aHJlYWQsIFNJR0lOVCk7DQpzcnBfZGFlbW9uL3NycF9kYWVtb24u YzoxOTAxOgkJcHRocmVhZF9raWxsKHJlcy0+dHJhcF90aHJlYWQsIFNJR0lOVCk7DQoNCkJhcnQu --
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Fair enough

Correct, this is why I called it a sketch :) I'm sure there are a few mistakes too..

Hopefully Honggang can finish it and test it, as he found a real bug.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

OK, I agree with you. The "signal_handler", which calls "wake_up_main_loop", is not the right solution, because wakeup_pipe never used by ibsrpdm.

<snip>
void wake_up_main_loop(char ch)
{
int res;

assert(wakeup_pipe[1] >= 0); // this 'assert' will be failed
<snip>

No, if one pthread just calls 'exit', the entire process will be terminated immediately. So, we need to cleanup the threads.

I think the source of current issue is the async_ev_thread pthread. We should *NOT* create such pthread for ibsrpdm.

I checked the old srptools git repo.

git://git.openfabrics.org/~bvanassche/srptools.git

Commit ab57a5b92eb3b8c9221f77235a028814a462d2cb merges "ibsrpdm" into "srp_daemon". The old ibsrpdm program is a single thread program. srp_daemon is multi-thread program.

As ibsrpdm always only run once, because "config->once" has been assigned to 1. I would suggest to apply this patch.

index cec36db2..4012a7db 100644
@@ -1945,12 +1945,12 @@ static struct resources *alloc_res(void)
 				     run_thread_get_trap_notices, &res->res);
 		if (ret)
 			goto err;
-	}
 
-	ret = pthread_create(&res->res.async_ev_thread, NULL,
-			     run_thread_listen_to_events, &res->res);
-	if (ret)
-		goto err;
+		ret = pthread_create(&res->res.async_ev_thread, NULL,
+				     run_thread_listen_to_events, &res->res);
+		if (ret)
+			goto err;
+	}
 
 	if (config->retry_timeout && !config->once) {
 		ret = pthread_create(&res->res.reconnect_thread, NULL,

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

T24gVHVlLCAyMDE3LTEyLTE5IGF0IDIwOjIwICswODAwLCBIb25nZ2FuZyBMSSB3cm90ZToNCj4g QXMgaWJzcnBkbSBhbHdheXMgb25seSBydW4gb25jZSwgYmVjYXVzZSAiY29uZmlnLT5vbmNlIiBo YXMgYmVlbg0KPiBhc3NpZ25lZCB0byAxLiBJIHdvdWxkIHN1Z2dlc3QgdG8gYXBwbHkgdGhpcyBw YXRjaC4NCj4gDQo+IGRpZmYgLS1naXQgYS9zcnBfZGFlbW9uL3NycF9kYWVtb24uYyBiL3NycF9k YWVtb24vc3JwX2RhZW1vbi5jDQo+IGluZGV4IGNlYzM2ZGIyLi40MDEyYTdkYiAxMDA2NDQNCj4g LS0tIGEvc3JwX2RhZW1vbi9zcnBfZGFlbW9uLmMNCj4gKysrIGIvc3JwX2RhZW1vbi9zcnBfZGFl bW9uLmMNCj4gQEAgLTE5NDUsMTIgKzE5NDUsMTIgQEAgc3RhdGljIHN0cnVjdCByZXNvdXJjZXMg KmFsbG9jX3Jlcyh2b2lkKQ0KPiAgCQkJCSAgICAgcnVuX3RocmVhZF9nZXRfdHJhcF9ub3RpY2Vz LCAmcmVzLT5yZXMpOw0KPiAgCQlpZiAocmV0KQ0KPiAgCQkJZ290byBlcnI7DQo+IC0JfQ0KPiAg DQo+IC0JcmV0ID0gcHRocmVhZF9jcmVhdGUoJnJlcy0+cmVzLmFzeW5jX2V2X3RocmVhZCwgTlVM TCwNCj4gLQkJCSAgICAgcnVuX3RocmVhZF9saXN0ZW5fdG9fZXZlbnRzLCAmcmVzLT5yZXMpOw0K PiAtCWlmIChyZXQpDQo+IC0JCWdvdG8gZXJyOw0KPiArCQlyZXQgPSBwdGhyZWFkX2NyZWF0ZSgm cmVzLT5yZXMuYXN5bmNfZXZfdGhyZWFkLCBOVUxMLA0KPiArCQkJCSAgICAgcnVuX3RocmVhZF9s aXN0ZW5fdG9fZXZlbnRzLCAmcmVzLT5yZXMpOw0KPiArCQlpZiAocmV0KQ0KPiArCQkJZ290byBl cnI7DQo+ICsJfQ0KPiAgDQo+ICAJaWYgKGNvbmZpZy0+cmV0cnlfdGltZW91dCAmJiAhY29uZmln LT5vbmNlKSB7DQo+ICAJCXJldCA9IHB0aHJlYWRfY3JlYXRlKCZyZXMtPnJlcy5yZWNvbm5lY3Rf dGhyZWFkLCBOVUxMLA0KDQpQbGVhc2Ugc3VibWl0IHRoaXMgYXMgYSBwcm9wZXIgcGF0Y2ggYW5k IGFkZCB0aGUgZm9sbG93aW5nOg0KDQpSZXZpZXdlZC1ieTogQmFydCBWYW4gQXNzY2hlIDxiYXJ0 LnZhbmFzc2NoZUB3ZGMuY29tPg0KDQpUaGFua3MsDQoNCkJhcnQuDQoNCg==
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

[rdma-core PATCH] srp_daemon: Don't create async_ev_thread if only run once

I sent the patch with that $SUBJECT.

thanks

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

FYI, this is merged into rdma-core, use
git log -p --follow srp_daemon/srp_daemon.c

and it will show you full history on a single file.

Makes sense that ibsrpdm does not need the run_thread_listen_to_events() thread. Patch looks OK to me too.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html