rpma: make gpspm server use separate RCQ with epoll #282

yangx-jy · 2022-03-03T15:43:34Z

Also use epoll APIs when busy_wait_polling == 0.

Signed-off-by: Xiao Yang [email protected]

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?

Also use epoll APIs when busy_wait_polling == 0. Signed-off-by: Xiao Yang <[email protected]>

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 · 2022-03-16T14:30:41Z

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:

yangx-jy mentioned this pull request Mar 3, 2022

rpma: make gpspm server use seperate RCQ #272

Closed

ldorau reviewed Mar 4, 2022

View reviewed changes

ldorau added the sprint goal label Mar 4, 2022

ldorau reviewed Mar 4, 2022

View reviewed changes

yangx-jy force-pushed the rcq_with_epoll branch from ee1deeb to bcf5244 Compare March 9, 2022 13:27

ldorau reviewed Mar 9, 2022

View reviewed changes

yangx-jy force-pushed the rcq_with_epoll branch from bcf5244 to af98291 Compare March 10, 2022 14:44

ldorau requested changes Mar 15, 2022

View reviewed changes

yangx-jy force-pushed the rcq_with_epoll branch from af98291 to 99c1b31 Compare March 16, 2022 03:13

yangx-jy commented Mar 16, 2022

View reviewed changes

yangx-jy force-pushed the rcq_with_epoll branch 3 times, most recently from 4c91e26 to 21c045a Compare March 16, 2022 06:46

ldorau reviewed Mar 16, 2022

View reviewed changes

ldorau requested changes Mar 16, 2022

View reviewed changes

yangx-jy force-pushed the rcq_with_epoll branch from 21c045a to ad8a967 Compare March 16, 2022 13:36

yangx-jy commented Mar 16, 2022

View reviewed changes

rpma: make gpspm server use separate RCQ

c2e9fdd

Also use epoll APIs when busy_wait_polling == 0. Signed-off-by: Xiao Yang <[email protected]>

ldorau requested changes Mar 16, 2022

View reviewed changes

yangx-jy force-pushed the rcq_with_epoll branch from ad8a967 to c2e9fdd Compare March 16, 2022 14:29

ldorau approved these changes Mar 16, 2022

View reviewed changes

ldorau requested changes Mar 17, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpma: make gpspm server use separate RCQ with epoll #282

rpma: make gpspm server use separate RCQ with epoll #282

yangx-jy commented Mar 3, 2022 •

edited by janekmi

Loading

ldorau left a comment

ldorau left a comment

ldorau left a comment

ldorau left a comment

ldorau left a comment

yangx-jy left a comment

ldorau left a comment

ldorau left a comment

yangx-jy left a comment

ldorau left a comment

yangx-jy commented Mar 16, 2022

ldorau left a comment

ldorau left a comment

ldorau left a comment

rpma: make gpspm server use separate RCQ with epoll #282

Are you sure you want to change the base?

rpma: make gpspm server use separate RCQ with epoll #282

Conversation

yangx-jy commented Mar 3, 2022 • edited by janekmi Loading

ldorau left a comment

Choose a reason for hiding this comment

ldorau left a comment

Choose a reason for hiding this comment

ldorau left a comment

Choose a reason for hiding this comment

ldorau left a comment

Choose a reason for hiding this comment

ldorau left a comment

Choose a reason for hiding this comment

yangx-jy left a comment

Choose a reason for hiding this comment

ldorau left a comment

Choose a reason for hiding this comment

ldorau left a comment

Choose a reason for hiding this comment

yangx-jy left a comment

Choose a reason for hiding this comment

ldorau left a comment

Choose a reason for hiding this comment

yangx-jy commented Mar 16, 2022

ldorau left a comment

Choose a reason for hiding this comment

ldorau left a comment

Choose a reason for hiding this comment

ldorau left a comment

Choose a reason for hiding this comment

yangx-jy commented Mar 3, 2022 •

edited by janekmi

Loading