rpma: make gpspm server use separate RCQ with epoll

[yangx-jy]

Also use epoll APIs when busy_wait_polling == 0.

Signed-off-by: Xiao Yang yangx.jy@fujitsu.com

---

This change is 

[ldorau]

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @ldorau)

[ldorau]

@yangx-jy Thanks for the new pull request! We will benchmark it during this weekend.

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @ldorau)

[ldorau]

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @ldorau)

[ldorau]

@yangx-jy We have to repeat the benchmarks, because the performance was also degraded in the o->busy_wait_polling == 1 case.

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @yangx-jy)

---

engines/librpma_gpspm.c, line 649 at r3 (raw file):

	if (o->busy_wait_polling == 0)
		ret = epoll_cleanup(td);

Maybe ret |= epoll_cleanup(td); ?

Code quote:

ret = epoll_cleanup(td);

---

engines/librpma_gpspm.c, line 760 at r3 (raw file):

		librpma_td_verror(td, ret, "rpma_cq_get_wc");
		goto err_terminate;
	}

else is not needed after return:

	if (ret == RPMA_E_NO_COMPLETION) {
		/* lack of completion is not an error */
		return 0;
	} 
	if (ret) {
		librpma_td_verror(td, ret, "rpma_cq_get_wc");
		goto err_terminate;
	}

Code quote:

	if (ret == RPMA_E_NO_COMPLETION) {
		/* lack of completion is not an error */
		return 0;
	} else if (ret) {


		librpma_td_verror(td, ret, "rpma_cq_get_wc");
		goto err_terminate;
	}

---

engines/librpma_gpspm.c, line 784 at r3 (raw file):

 *   -1      - in case of an error
 */
static int server_cmpl_wait(struct thread_data *td, struct rpma_cq *cq,

server_cmpl_wait_and_poll

The current name is misleading, it suggests the function only waits.

Code quote:

 server_cmpl_wait(

---

engines/librpma_gpspm.c, line 795 at r3 (raw file):

	} else if (ret) {
		librpma_td_verror(td, ret, "rpma_cq_wait");
		td->terminate = true;

No else after return or goto, please:

	if (ret == RPMA_E_NO_COMPLETION) {
		/* lack of completion is not an error */
		return 0;
	}
	if (ret) {
		librpma_td_verror(td, ret, "rpma_cq_wait");
		td->terminate = true;
		return -1;
	}

Code quote:

	if (ret == RPMA_E_NO_COMPLETION) {
		/* lack of completion is not an error */
		return 0;
	} else if (ret) {
		librpma_td_verror(td, ret, "rpma_cq_wait");
		td->terminate = true;
		return -1;
	}

---

engines/librpma_gpspm.c, line 799 at r3 (raw file):

	}

	ret = server_cmpl_poll(td, cq, wc);

	return server_cmpl_poll(td, cq, wc);

Code quote:

	ret = server_cmpl_poll(td, cq, wc);

	return ret;

---

engines/librpma_gpspm.c, line 832 at r3 (raw file):

				if (rv == 0) {
					rv = server_cmpl_wait(td, csd->cq, &cq_wc);
					if (rv < 0)

				if (sd->msg_sqe_available)
					break;

				rv = server_cmpl_wait_and_poll(td, csd->cq, &cq_wc);
				if (rv < 0)
					return rv;

Code quote:

				if (rv == 0) {
					rv = server_cmpl_wait(td, csd->cq, &cq_wc);
					if (rv < 0)
						return rv;
				}

---

engines/librpma_gpspm.c, line 846 at r3 (raw file):

		rv = server_cmpl_wait(td, csd->cq, &cq_wc);
		if (rv < 0)
			return rv;

Maybe we could try to empty the send CQ here?

		/* process as many send completions as possible */
		rv = server_cmpl_wait_and_poll(td, csd->cq, &cq_wc);
		if (rv < 0)
			return rv;

		while ((rv = server_cmpl_poll(td, csd->cq, &cq_wc))) {
			if (rv < 0)
				return rv;
		}

Code quote:

		/* process the send completion */
		rv = server_cmpl_wait(td, csd->cq, &cq_wc);
		if (rv < 0)
			return rv;

---

engines/librpma_gpspm.c, line 866 at r3 (raw file):

	if (ret == 1 && rcq_wc.opcode == IBV_WC_RECV) {
		/* ensure that at least one SQ slot is available */
		while (sd->msg_sqe_available == 0) {

We could do that exactly like in #272:

	/* process as many send completions as possible */
	while ((ret = server_cmpl_poll(td, csd->cq, &cq_wc))) {
		if (ret < 0)
			return ret;
	}

	/* ensure that at least one SQ slot is available */
	while (sd->msg_sqe_available == 0) {
		ret = server_cmpl_poll(td, csd->cq, &cq_wc);
		if (ret < 0)
			return ret;
	}

	/* poll the receive completion */
	ret = server_cmpl_poll(td, sd->rcq, &rcq_wc);
	if (ret < 0)
		return ret;

	if (ret == 1 && rcq_wc.opcode == IBV_WC_RECV) {
		if ((ret = server_qe_process(td, &rcq_wc)))
			return ret;
	}

	return 0;

Code quote:

	/* process the receive completion */
	ret = server_cmpl_poll(td, sd->rcq, &rcq_wc);
	if (ret < 0)
		return ret;

	if (ret == 1 && rcq_wc.opcode == IBV_WC_RECV) {
		/* ensure that at least one SQ slot is available */
		while (sd->msg_sqe_available == 0) {
			/* process the send completion */
			ret = server_cmpl_poll(td, csd->cq, &cq_wc);
			if (ret < 0)
				return ret;
		}

		if ((ret = server_qe_process(td, &rcq_wc)))
			return ret;
	}


	return 0;

[ldorau]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @yangx-jy)

---

engines/librpma_gpspm.c, line 804 at r3 (raw file):

}

static int server_queue_wait(struct thread_data *td)

server_queue_epoll_wait

Code quote:

server_queue_wait

[yangx-jy]

Reviewable status: 0 of 1 files reviewed, 8 unresolved discussions (waiting on @ldorau)

---

engines/librpma_gpspm.c, line 649 at r3 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

Maybe ret |= epoll_cleanup(td); ?

It seems unnecessary becase the invalid return values of librpma_fio_server_close_file() and epoll_cleanup are same (-1).
I kept the logic for now.

---

engines/librpma_gpspm.c, line 784 at r3 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

server_cmpl_wait_and_poll

The current name is misleading, it suggests the function only waits.

Done.

---

engines/librpma_gpspm.c, line 795 at r3 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

No else after return or goto, please:

	if (ret == RPMA_E_NO_COMPLETION) {
		/* lack of completion is not an error */
		return 0;
	}
	if (ret) {
		librpma_td_verror(td, ret, "rpma_cq_wait");
		td->terminate = true;
		return -1;
	}

Done.

---

engines/librpma_gpspm.c, line 799 at r3 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

	return server_cmpl_poll(td, cq, wc);

Done.

---

engines/librpma_gpspm.c, line 804 at r3 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

server_queue_epoll_wait

Done.

---

engines/librpma_gpspm.c, line 832 at r3 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

				if (sd->msg_sqe_available)
					break;

				rv = server_cmpl_wait_and_poll(td, csd->cq, &cq_wc);
				if (rv < 0)
					return rv;

Done.

---

engines/librpma_gpspm.c, line 846 at r3 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

Maybe we could try to empty the send CQ here?

		/* process as many send completions as possible */
		rv = server_cmpl_wait_and_poll(td, csd->cq, &cq_wc);
		if (rv < 0)
			return rv;

		while ((rv = server_cmpl_poll(td, csd->cq, &cq_wc))) {
			if (rv < 0)
				return rv;
		}

epoll_wait() is introduced to process the send completions which are placed in send CQ so I think we don't need to empty send CQ here.
BTW: we can add it if the latest logic still degrade the performance. ^_^

---

engines/librpma_gpspm.c, line 866 at r3 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

We could do that exactly like in #272:

	/* process as many send completions as possible */
	while ((ret = server_cmpl_poll(td, csd->cq, &cq_wc))) {
		if (ret < 0)
			return ret;
	}

	/* ensure that at least one SQ slot is available */
	while (sd->msg_sqe_available == 0) {
		ret = server_cmpl_poll(td, csd->cq, &cq_wc);
		if (ret < 0)
			return ret;
	}

	/* poll the receive completion */
	ret = server_cmpl_poll(td, sd->rcq, &rcq_wc);
	if (ret < 0)
		return ret;

	if (ret == 1 && rcq_wc.opcode == IBV_WC_RECV) {
		if ((ret = server_qe_process(td, &rcq_wc)))
			return ret;
	}

	return 0;

Done.

[ldorau]

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @ldorau)

[ldorau]

Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yangx-jy)

---

engines/librpma_gpspm.c, line 649 at r3 (raw file):

Previously, yangx-jy (Xiao Yang) wrote…

It seems unnecessary becase the invalid return values of librpma_fio_server_close_file() and epoll_cleanup are same (-1).
I kept the logic for now.

It is necessary, because if librpma_fio_server_close_file fails and epoll_cleanup succeeds, the return value will be 0 instead of -1.

---

engines/librpma_gpspm.c, line 846 at r3 (raw file):

Previously, yangx-jy (Xiao Yang) wrote…

epoll_wait() is introduced to process the send completions which are placed in send CQ so I think we don't need to empty send CQ here.
BTW: we can add it if the latest logic still degrade the performance. ^_^

OK

[yangx-jy]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @ldorau)

---

engines/librpma_gpspm.c, line 649 at r3 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

It is necessary, because if librpma_fio_server_close_file fails and epoll_cleanup succeeds, the return value will be 0 instead of -1.

You are right, I use another logic instead.
Do you think ret |= epoll_cleanup(td); can work?

[ldorau]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yangx-jy)

---

engines/librpma_gpspm.c, line 649 at r3 (raw file):

Previously, yangx-jy (Xiao Yang) wrote…

You are right, I use another logic instead.
Do you think ret |= epoll_cleanup(td); can work?

The previous version (simpler IMHO):

	ret = librpma_fio_server_close_file(td, f);

	if (o->busy_wait_polling == 0)
		ret |= epoll_cleanup(td);

	return ret;

with ret |= epoll_cleanup(td); has to work, because: x | x = x, 0 | x = x and x | 0 = x - it covers all cases.

Code quote:

c int server_close_file(struc

[yangx-jy]

---

engines/librpma_gpspm.c, line 649 at r3 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

The previous version (simpler IMHO):

	ret = librpma_fio_server_close_file(td, f);

	if (o->busy_wait_polling == 0)
		ret |= epoll_cleanup(td);

	return ret;

with ret |= epoll_cleanup(td); has to work, because: x | x = x, 0 | x = x and x | 0 = x - it covers all cases.

Done.

[ldorau]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @yangx-jy)

[ldorau]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yangx-jy)

a discussion (no related file):
Waiting for results of benchmarks ...

---

[ldorau]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yangx-jy)

a discussion (no related file):

Previously, ldorau (Lukasz Dorau) wrote…

Waiting for results of benchmarks ...

The results are following:

---