rpma: add rpma_mr_advise()

[ewanglong]

This change is 

[janekmi]

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ewanglong)

---

src/mr.c, line 564 at r1 (raw file):

/*
 * rpma_mr_advise -- give advice about an address range in a memory region

region -> registration

I'm trying to keep things consistent. It seems that we are calling MR a memory registration instead of a memory region which seems to be a name of any not registered memory region. Up to discussion.

---

src/include/librpma.h, line 1150 at r1 (raw file):

/** 3
 * rpma_mr_advise - give advice about an address range in a memory region

.

---

src/include/librpma.h, line 1174 at r1 (raw file):

 * the flags
 * - IBV_ADVISE_MR_FLAG_FLUSH - the underlying pages are guaranteed to be
 *   updated in the HCA before returning SUCCESS

Assuming, we can just pass advice and flags directly to ibv_advise_mr(3) I prefer to refer the end-user directly to the ibv_advise_mr(3) manual for details. Instead, here I think we should describe what is the impact of this API call to FileSystemDAX performance. But this part of the documentation may appear after collecting some performance numbers.

Credit: @grom72

---

src/include/librpma.h, line 1189 at r1 (raw file):

 */
int rpma_mr_advise(struct rpma_mr_local *mr, size_t offset, size_t len,
		int flags, bool nofault);

I have just realized we can implement all of these a lot easier (thank you @grom72 for supporting my thinking process) by using libibverbs enums and flags directly as follows:

int
rpma_mr_advise(struct rpma_mr_local *mr, size_t offset, enum ibv_advise_mr_advice advice, uint32_t flags);

Where both advice and flags are exactly the same as for ibv_advise_mr(3).
So, you just have to construct a sg_list and call ibv_advise_mr(3). What do you think?

[ewanglong]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ewanglong and @grom72)

---

src/include/librpma.h, line 1189 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I have just realized we can implement all of these a lot easier (thank you @grom72 for supporting my thinking process) by using libibverbs enums and flags directly as follows:

int
rpma_mr_advise(struct rpma_mr_local *mr, size_t offset, enum ibv_advise_mr_advice advice, uint32_t flags);

Where both advice and flags are exactly the same as for ibv_advise_mr(3).
So, you just have to construct a sg_list and call ibv_advise_mr(3). What do you think?

If so, why need this encapulate-only API? For the code of advice determining, may we hide a few ibv logic and make things easy to call for user?

[ewanglong]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ewanglong, @grom72, and @janekmi)

---

src/mr.c, line 564 at r1 (raw file):

memory region
I saw "memory region" in those APIs of rpma_mr_remote_get_size, rpma_mr_remote_delete and rpma_mr_remote_get_flush_type and also in kernel introducing ibv_advise_mr pages. How can keey them all consistent?

[ewanglong]

Reviewable status: 1 of 3 files reviewed, 4 unresolved discussions (waiting on @grom72 and @janekmi)

---

src/mr.c, line 564 at r1 (raw file):

Previously, ewanglong wrote…

memory region
I saw "memory region" in those APIs of rpma_mr_remote_get_size, rpma_mr_remote_delete and rpma_mr_remote_get_flush_type and also in kernel introducing ibv_advise_mr pages. How can keey them all consistent?

Done.

---

src/include/librpma.h, line 1150 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

Done.

---

src/include/librpma.h, line 1174 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Assuming, we can just pass advice and flags directly to ibv_advise_mr(3) I prefer to refer the end-user directly to the ibv_advise_mr(3) manual for details. Instead, here I think we should describe what is the impact of this API call to FileSystemDAX performance. But this part of the documentation may appear after collecting some performance numbers.

Credit: @grom72

Done.

---

src/include/librpma.h, line 1189 at r1 (raw file):

Previously, ewanglong wrote…

If so, why need this encapulate-only API? For the code of advice determining, may we hide a few ibv logic and make things easy to call for user?

Done.

[janekmi]

Reviewed all commit messages.
Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @ewanglong and @janekmi)

a discussion (no related file):
I think we are on the same page now. So you can focus on polishing things up: unit tests etc.
I will think a little bit more about the documentation in the meantime.

---

---

src/mr.c, line 520 at r2 (raw file):

 */
static int
rpma_mr_usage_to_access(struct rpma_mr_local *mr)

Please remove. I belive it is not needed any more.

[janekmi]

Reviewed 1 of 2 files at r2.
Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @ewanglong and @janekmi)

---

src/mr.c, line 573 at r2 (raw file):

	sg_list.lkey = mr->ibv_mr->lkey;
	sg_list.addr = (uint64_t)((char *)mr->ibv_mr->addr + offset);
	sg_list.length = len;

So far it seems we have used an explicit cast (uint32_t).

---

src/mr.c, line 578 at r2 (raw file):

		RPMA_LOG_ERROR_WITH_ERRNO(errno,
			"ibv_advise_mr failed: "
			"ibv_reg_mr(addr=%p, length=%zu, advice=%i, flags=%d)",

We do not log failed call arguments. Please see other uses of the RPMA_LOG_ERROR_WITH_ERRNO() macro.

[grom72]

Reviewed 1 of 3 files at r1, 1 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ewanglong)

[ewanglong]

Reviewable status: 2 of 8 files reviewed, 3 unresolved discussions (waiting on @janekmi)

---

src/mr.c, line 520 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Please remove. I belive it is not needed any more.

Done.

---

src/mr.c, line 573 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

So far it seems we have used an explicit cast (uint32_t).

Using uintptr_t now.

---

src/mr.c, line 578 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

We do not log failed call arguments. Please see other uses of the RPMA_LOG_ERROR_WITH_ERRNO() macro.

Done.

[grom72]

Reviewed 6 of 6 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ewanglong and @janekmi)

---

src/include/librpma.h, line 1180 at r3 (raw file):

memory registration failed

ibv_advise_mr() failed, the exact cause of the error can be read from the log

[ewanglong]

Reviewable status: 7 of 8 files reviewed, 4 unresolved discussions (waiting on @grom72 and @janekmi)

---

src/include/librpma.h, line 1180 at r3 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

memory registration failed

ibv_advise_mr() failed, the exact cause of the error can be read from the log

Done.

[codecov]

Codecov Report

Merging #1220 (1cc9b82) into master (fbfc8ef) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1220   +/-   ##
=======================================
  Coverage   99.84%   99.84%           
=======================================
  Files          16       16           
  Lines        1321     1330    +9     
=======================================
+ Hits         1319     1328    +9     
  Misses          2        2           

[ldorau]

Reviewed 1 of 3 files at r1, all commit messages.
Reviewable status: 5 of 8 files reviewed, 4 unresolved discussions (waiting on @grom72 and @janekmi)

[janekmi]

Reviewed 4 of 6 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @ewanglong and @grom72)

---

src/mr.c, line 573 at r2 (raw file):

Previously, ewanglong wrote…

Using uintptr_t now.

I think the change in the line above is good. 👍

But I was thinking about this one:

sg_list.length = len;

I think you should be consistent with all other places where we accept a length of a memory region.

---

src/mr.c, line 519 at r4 (raw file):

int
rpma_mr_advise(struct rpma_mr_local *mr, size_t offset, size_t len,
		uint32_t advice, uint32_t flags)

Why?

---

src/include/librpma.h, line 1180 at r3 (raw file):

Previously, ewanglong wrote…

Done.

ibv_advise_mr() failed

---

src/include/librpma.h, line 1171 at r4 (raw file):

 * the flags
 * - IBV_ADVISE_MR_FLAG_FLUSH - the underlying pages are guaranteed to be
 *   updated in the HCA before returning SUCCESS

I do not like making a copy of the ibv_advise_mr(3) manual. I think we should just refer the user to the place where it is documented. Please add to the previous paragraph e.g.:

For available advice and flags values please see ibv_advise_mr(3).

---

tests/unit/common/mocks-ibverbs.h, line 70 at r4 (raw file):

	uint32_t num_sge;
	int ret;
};

I think this structure, despite being convenient, goes against the cmocka mechanics. Please see other comments for my proposal.

---

tests/unit/common/mocks-ibverbs.c, line 352 at r4 (raw file):

/*
 * ibv_post_send_mock -- mock of ibv_advise_mr()

ibv_advise_mr_mock

---

tests/unit/common/mocks-ibverbs.c, line 364 at r4 (raw file):

		mock_type(struct ibv_advise_mr_mock_args *);

	assert_non_null(pd);

It seems unnecessary. Its value is checked a few lines below.

---

tests/unit/common/mocks-ibverbs.c, line 367 at r4 (raw file):

	assert_non_null(sg_list);

	assert_int_equal(pd, args->pd);

check_expected_ptr(pd);

---

tests/unit/common/mocks-ibverbs.c, line 374 at r4 (raw file):

	assert_int_equal(sg_list->addr, args->local_addr);
	assert_int_equal(sg_list->length, args->length);
	assert_int_equal(num_sge, args->num_sge);

I believe you can apply check_expected_ptr() to all of the checks above.

---

tests/unit/common/mocks-ibverbs.c, line 376 at r4 (raw file):

	assert_int_equal(num_sge, args->num_sge);

	return args->ret;

return mock_type(int);

---

tests/unit/mr/mr-advise.c, line 12 at r4 (raw file):

#include "cmocka_headers.h"
#include "mr.h"

Is this header really needed?

---

tests/unit/mr/mr-advise.c, line 30 at r4 (raw file):

	/* configure mocks */
	struct ibv_advise_mr_mock_args args;
	args.pd = MOCK_IBV_PD;

expect_value(pd, MOCK_IBV_PD);

---

tests/unit/mr/mr-advise.c, line 37 at r4 (raw file):

	args.lkey = MOCK_LKEY;
	args.num_sge = 1;
	args.ret = RPMA_E_PROVIDER;

I think you can apply expect_value() to all of them.

---

tests/unit/mr/mr-advise.c, line 38 at r4 (raw file):

	args.num_sge = 1;
	args.ret = RPMA_E_PROVIDER;
	will_return(ibv_advise_mr_mock, &args);

will_return(ibv_advise_mr_mock, RPMA_E_PROVIDER);

---

tests/unit/mr/mr-advise.c, line 45 at r4 (raw file):

				IB_UVERBS_ADVISE_MR_FLAG_FLUSH);

A redundant new line.

---

tests/unit/mr/mr-advise.c, line 68 at r4 (raw file):

	args.num_sge = 1;
	args.ret = MOCK_OK;
	will_return(ibv_advise_mr_mock, &args);

Same here.

---

tests/unit/mr/mr-advise.c, line 97 at r4 (raw file):

	 * }
	 * so we can set the advise_mr function pointer to our mock function.
	 */

👍

[ewanglong]

Reviewable status: 6 of 8 files reviewed, 16 unresolved discussions (waiting on @grom72 and @janekmi)

---

src/mr.c, line 573 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I think the change in the line above is good. 👍

But I was thinking about this one:

sg_list.length = len;

I think you should be consistent with all other places where we accept a length of a memory region.

Done.

---

src/mr.c, line 519 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Why?

Chanaged it back. But it show warning as error as below.
/rpma/src/include/librpma.h:1186:8: error: 'enum ibv_advise_mr_advice' declared inside parameter list will not be visible outside of this definition or declaration [-Werror]

---

src/include/librpma.h, line 1171 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I do not like making a copy of the ibv_advise_mr(3) manual. I think we should just refer the user to the place where it is documented. Please add to the previous paragraph e.g.:

For available advice and flags values please see ibv_advise_mr(3).

Done.

---

tests/unit/common/mocks-ibverbs.h, line 70 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I think this structure, despite being convenient, goes against the cmocka mechanics. Please see other comments for my proposal.

Done.

---

tests/unit/common/mocks-ibverbs.c, line 352 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

ibv_advise_mr_mock

Done.

---

tests/unit/common/mocks-ibverbs.c, line 364 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It seems unnecessary. Its value is checked a few lines below.

Done.

---

tests/unit/common/mocks-ibverbs.c, line 367 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

check_expected_ptr(pd);

Done.

---

tests/unit/common/mocks-ibverbs.c, line 374 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I believe you can apply check_expected_ptr() to all of the checks above.

Done.

---

tests/unit/common/mocks-ibverbs.c, line 376 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

return mock_type(int);

Done.

---

tests/unit/mr/mr-advise.c, line 12 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Is this header really needed?

Done.

---

tests/unit/mr/mr-advise.c, line 30 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

expect_value(pd, MOCK_IBV_PD);

Done.

---

tests/unit/mr/mr-advise.c, line 37 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I think you can apply expect_value() to all of them.

Done.

---

tests/unit/mr/mr-advise.c, line 38 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

will_return(ibv_advise_mr_mock, RPMA_E_PROVIDER);

Done.

---

tests/unit/mr/mr-advise.c, line 45 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

A redundant new line.

Done.

---

tests/unit/mr/mr-advise.c, line 68 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Same here.

Done.

[janekmi]

Reviewed 5 of 5 files at r6, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ewanglong, @grom72, and @janekmi)

---

src/mr.c, line 573 at r2 (raw file):

Previously, ewanglong wrote…

Done.

I thought about something like this:

sge.length = (uint32_t)len;

---

tests/unit/common/mocks-ibverbs.c, line 371 at r6 (raw file):

	check_expected(num_sge);

	return mock_type(int); /* errno */

It is not an errno. I think we can drop this comment at all.

[janekmi]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ewanglong, @grom72, @ldorau, and @osalyk)

---

src/mr.c, line 519 at r4 (raw file):

Previously, ewanglong wrote…

Chanaged it back. But it show warning as error as below.
/rpma/src/include/librpma.h:1186:8: error: 'enum ibv_advise_mr_advice' declared inside parameter list will not be visible outside of this definition or declaration [-Werror]

It seems enum ibv_advise_mr_advice is not available on all our docker images because it was introduced along the way. So, I think it was a very good idea to replace it with an appropriate integer replacement. 👏

After my short research, I am pretty sure that enum can be as large as uint32_t but not bigger.

@grom72 @ldorau @osalyk Any thoughts on this?

[ewanglong]

Reviewable status: 5 of 8 files reviewed, 4 unresolved discussions (waiting on @grom72, @janekmi, @ldorau, and @osalyk)

---

src/mr.c, line 573 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I thought about something like this:

sge.length = (uint32_t)len;

Done.

---

src/mr.c, line 519 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It seems enum ibv_advise_mr_advice is not available on all our docker images because it was introduced along the way. So, I think it was a very good idea to replace it with an appropriate integer replacement. 👏

After my short research, I am pretty sure that enum can be as large as uint32_t but not bigger.

@grom72 @ldorau @osalyk Any thoughts on this?

Seems it's better to replace it with "int". There is enum definition from the current C Standard (C99)(http://www.open-std.org/JTC1/SC22/WG14/www/docs/n1256.pdf)
6.7.2.2 Enumeration specifiers
The expression that defines the value of an enumeration constant shall be an integer
constant expression that has a value representable as an int.

---

tests/unit/common/mocks-ibverbs.c, line 371 at r6 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It is not an errno. I think we can drop this comment at all.

Done.

[ldorau]

Reviewed 1 of 3 files at r4, all commit messages.
Reviewable status: 5 of 8 files reviewed, 6 unresolved discussions (waiting on @ewanglong, @grom72, @janekmi, and @osalyk)

---

src/mr.c, line 519 at r4 (raw file):

Previously, ewanglong wrote…

Seems it's better to replace it with "int". There is enum definition from the current C Standard (C99)(http://www.open-std.org/JTC1/SC22/WG14/www/docs/n1256.pdf)
6.7.2.2 Enumeration specifiers
The expression that defines the value of an enumeration constant shall be an integer
constant expression that has a value representable as an int.

Yes, according to C99 Standard it should be int.

---

src/mr.c, line 526 at r7 (raw file):

	sg_list.length = (uint32_t)length;

	int ret = ibv_advise_mr(mr->ibv_mr->pd, advice, flags, &sg_list, 1);

/rpma/src/mr.c:526:42: error: implicit conversion changes signedness: 'int' to 'enum ib_uverbs_advise_mr_advice' [-Werror,-Wsign-conversion]
        int ret = ibv_advise_mr(mr->ibv_mr->pd, advice, flags, &sg_list, 1);

https://github.com/ldorau/rpma/runs/3541507648

---

src/mr.c, line 526 at r7 (raw file):

	sg_list.length = (uint32_t)length;

	int ret = ibv_advise_mr(mr->ibv_mr->pd, advice, flags, &sg_list, 1);

/rpma/src/mr.c:526:12: error: implicit declaration of function 'ibv_advise_mr' [-Werror=implicit-function-declaration]
  int ret = ibv_advise_mr(mr->ibv_mr->pd, advice, flags, &sg_list, 1);

https://github.com/ldorau/rpma/runs/3541507648

[ldorau]

Reviewable status: 5 of 8 files reviewed, 7 unresolved discussions (waiting on @ewanglong, @grom72, @janekmi, and @osalyk)

a discussion (no related file):
Please rebase on the current master - there are new versions of OSes now.

---

[grom72]

Reviewed 1 of 3 files at r4, 1 of 5 files at r6, 1 of 3 files at r7, all commit messages.
Reviewable status: 6 of 8 files reviewed, 7 unresolved discussions (waiting on @ewanglong, @janekmi, @ldorau, and @osalyk)

---

src/mr.c, line 526 at r7 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

/rpma/src/mr.c:526:12: error: implicit declaration of function 'ibv_advise_mr' [-Werror=implicit-function-declaration]
  int ret = ibv_advise_mr(mr->ibv_mr->pd, advice, flags, &sg_list, 1);

https://github.com/ldorau/rpma/runs/3541507648

shall we have here conditional compilation and return an error if OS does not support this feature?

[grom72]

Reviewed 2 of 6 files at r14.
Reviewable status: 8 of 10 files reviewed, 14 unresolved discussions (waiting on @ewanglong, @grom72, @janekmi, @ldorau, and @osalyk)

---

src/mr.c, line 535 at r12 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

+1 good idea

+1

[grom72]

Reviewed 1 of 2 files at r15, all commit messages.
Reviewable status: 9 of 10 files reviewed, 14 unresolved discussions (waiting on @ewanglong, @janekmi, @ldorau, and @osalyk)

[grom72]

Reviewed 1 of 2 files at r15.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @ewanglong, @janekmi, and @osalyk)

[ewanglong]

Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @janekmi, @ldorau, and @osalyk)

a discussion (no related file):

Previously, ldorau (Lukasz Dorau) wrote…

CI builds fail

Done.

---

---

CMakeLists.txt, line 121 at r12 (raw file):

Previously, janekmi (Jan Michalski) wrote…

ibv_advise_mr() I think it may increase comprehensibility if you point out it is a function.

Done.

---

cmake/functions.cmake, line 198 at r12 (raw file):

Previously, janekmi (Jan Michalski) wrote…

()

Done.

---

cmake/functions.cmake, line 202 at r12 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

Should be ibv_advise_mr() anyway

Done.

---

src/mr.c, line 573 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

xD So far, we are using len instead of length. I know it is not so much important but I very like things to be consistent. Please rename.

Done.

---

src/mr.c, line 519 at r4 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

... but if the 'ifdef' is moved, we have to go back to 'int' :-) Right :-)

Done.

---

src/mr.c, line 535 at r12 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

+1

Done.

---

src/mr.c, line 527 at r13 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

nt ret = ibv_advise_mr(mr->ibv_mr->pd, advice, flags, &sg_list, 1);

/rpma/src/mr.c:527:42: error: implicit conversion changes signedness: 'int' to 'enum ib_uverbs_advise_mr_advice' [-Werror,-Wsign-conversion]
        int ret = ibv_advise_mr(mr->ibv_mr->pd, advice, flags, &sg_list, 1);

Done.

---

src/include/librpma.h, line 1180 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Nope.

Done.

---

src/include/librpma.h, line 1149 at r12 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Please remove. Please see my other comments for the justification.

Done.

---

src/include/librpma.h, line 1174 at r12 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Please also add:

- RPMA_E_NOSUPP - the operation is not supported by the system

Done.

---

tests/unit/common/mocks-ibverbs.h, line 76 at r12 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

Done.

---

tests/unit/common/mocks-ibverbs.c, line 351 at r12 (raw file):

fdef IBV_ADVISE_MR_SUPPORTED

---

tests/unit/common/mocks-ibverbs.c, line 351 at r12 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

Done.

---

tests/unit/mr/CMakeLists.txt, line 29 at r12 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Please remove this if.

Additionally, I think we need one additional test to test the RPMA_E_NOSUPP condition.

Done.

[grom72]

Reviewed 1 of 6 files at r14.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @janekmi, @ldorau, and @osalyk)

[grom72]

Reviewed 3 of 4 files at r13, 2 of 6 files at r14.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @janekmi, @ldorau, and @osalyk)

[ldorau]

Reviewed 2 of 2 files at r15, all commit messages.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @ewanglong, @janekmi, @ldorau, and @osalyk)

---

tests/unit/common/mocks-ibverbs.h, line 82 at r15 (raw file):

Quoted 7 lines of code…

#ifdef IBV_ADVISE_MR_SUPPORTED
int ibv_advise_mr_mock(struct ibv_pd *pd, enum ibv_advise_mr_advice advice,
		uint32_t flags, struct ibv_sge *sg_list, uint32_t num_sge);
#else
int ibv_advise_mr_mock(struct ibv_pd *pd, int advice,
		uint32_t flags, struct ibv_sge *sg_list, uint32_t num_sge);
#endif

The 'else' part is not needed, please remove it.
This is enough:

#ifdef IBV_ADVISE_MR_SUPPORTED
int ibv_advise_mr_mock(struct ibv_pd *pd, enum ibv_advise_mr_advice advice,
		uint32_t flags, struct ibv_sge *sg_list, uint32_t num_sge);
#endif

See:
ldorau@90b9764
https://github.com/ldorau/rpma/actions/runs/1216804162

---

tests/unit/common/mocks-ibverbs.c, line 379 at r15 (raw file):

Quoted 10 lines of code…

#else
int
ibv_advise_mr_mock(struct ibv_pd *pd,
				int advice,
				uint32_t flags,
				struct ibv_sge *sg_list,
				uint32_t num_sge)
{
	return mock_type(int);
}

The 'else' part is not needed, please remove it.
See:
ldorau@90b9764
https://github.com/ldorau/rpma/actions/runs/1216804162

[ewanglong]

Reviewable status: 8 of 10 files reviewed, 14 unresolved discussions (waiting on @grom72, @janekmi, @ldorau, and @osalyk)

---

tests/unit/common/mocks-ibverbs.h, line 82 at r15 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

#ifdef IBV_ADVISE_MR_SUPPORTED
int ibv_advise_mr_mock(struct ibv_pd *pd, enum ibv_advise_mr_advice advice,
		uint32_t flags, struct ibv_sge *sg_list, uint32_t num_sge);
#else
int ibv_advise_mr_mock(struct ibv_pd *pd, int advice,
		uint32_t flags, struct ibv_sge *sg_list, uint32_t num_sge);
#endif

The 'else' part is not needed, please remove it.
This is enough:

#ifdef IBV_ADVISE_MR_SUPPORTED
int ibv_advise_mr_mock(struct ibv_pd *pd, enum ibv_advise_mr_advice advice,
		uint32_t flags, struct ibv_sge *sg_list, uint32_t num_sge);
#endif

See:
ldorau@90b9764
https://github.com/ldorau/rpma/actions/runs/1216804162

Done.

---

tests/unit/common/mocks-ibverbs.c, line 379 at r15 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

#else
int
ibv_advise_mr_mock(struct ibv_pd *pd,
				int advice,
				uint32_t flags,
				struct ibv_sge *sg_list,
				uint32_t num_sge)
{
	return mock_type(int);
}

The 'else' part is not needed, please remove it.
See:
ldorau@90b9764
https://github.com/ldorau/rpma/actions/runs/1216804162

Done.

[ldorau]

Reviewed 2 of 2 files at r16, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @janekmi and @osalyk)

[janekmi]

Reviewed 3 of 4 files at r13, 4 of 6 files at r14, 2 of 2 files at r16, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @ewanglong)

---

src/mr.c, line 536 at r16 (raw file):

	return 0;
#else
	RPMA_LOG_ERROR_WITH_ERRNO(RPMA_E_NOSUPP, "ibv_advise_mr()");

RPMA_LOG_ERROR("ibv_advise_mr() is not supported by the system");

---

src/mr.c, line 537 at r16 (raw file):

#else
	RPMA_LOG_ERROR_WITH_ERRNO(RPMA_E_NOSUPP, "ibv_advise_mr()");
	return RPMA_E_NOSUPP;

Please put a new line between these two new lines.

---

tests/unit/common/mocks-ibverbs.h, line 76 at r12 (raw file):

Previously, ewanglong wrote…

Done.

Please remove the ifdef.

---

tests/unit/common/mocks-ibverbs.h, line 77 at r16 (raw file):

#ifdef IBV_ADVISE_MR_SUPPORTED
int ibv_advise_mr_mock(struct ibv_pd *pd, enum ibv_advise_mr_advice advice,

enum -> int

---

tests/unit/common/mocks-ibverbs.c, line 351 at r12 (raw file):

Previously, ewanglong wrote…

Done.

Please remove the ifdef here.

---

tests/unit/mr/mr-advise.c, line 160 at r16 (raw file):

		NULL, NULL);
}
#endif

I prefer to put both test_mr_advise[] definitions under the ifdef because the main is exactly the same.
So this file will look like:

#ifdef IBV_ADVISE_MR_SUPPORTED

/* tests when ibv_advise_mr() is available */

#else

/* tests when ibv_advise_mr() is NOT available */

#endif

#ifdef IBV_ADVISE_MR_SUPPORTED
static const struct CMUnitTest test_mr_advise[] = {
	/* rpma_mr_adivse() unit tests */
	cmocka_unit_test_setup_teardown(advise__failed_E_PROVIDER,
			setup__mr_local_and_remote,
			teardown__mr_local_and_remote),
	cmocka_unit_test_setup_teardown(advise__success,
			setup__mr_local_and_remote,
			teardown__mr_local_and_remote),
	cmocka_unit_test(NULL)
};
#else
static const struct CMUnitTest test_mr_advise[] = {
	/* rpma_mr_adivse() unit tests */
	cmocka_unit_test_setup_teardown(advise__failed_E_NOSUPP,
			setup__mr_local_and_remote,
			teardown__mr_local_and_remote),
	cmocka_unit_test(NULL)
};
#endif

int
main(int argc, char *argv[])
{
	return cmocka_run_group_tests(test_mr_advise,
		NULL, NULL);
}

---

tests/unit/mr/mr-common.h, line 54 at r16 (raw file):

#define MOCK_LKEY		(uint32_t)0x20212223
#define MOCK_ADVICE		1
#define MOCK_MR_FLAG		1

Please remove one tab from this line.

---

tests/unit/mr/mr-common.h, line 54 at r16 (raw file):

#define MOCK_LKEY		(uint32_t)0x20212223
#define MOCK_ADVICE		1
#define MOCK_MR_FLAG		1

#define MOCK_MR_FLAG	(1 << 1)

Just to differentiate these two.

[ldorau]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @ewanglong)

[janekmi]

Reviewed 4 of 4 files at r17, 2 of 2 files at r18, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ewanglong)

[ewanglong]

Reviewable status: 9 of 10 files reviewed, 2 unresolved discussions (waiting on @grom72, @janekmi, and @ldorau)

---

tests/unit/common/mocks-ibverbs.h, line 76 at r12 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Please remove the ifdef.

it's needed.

---

tests/unit/common/mocks-ibverbs.h, line 77 at r16 (raw file):

Previously, janekmi (Jan Michalski) wrote…

enum -> int

it's needed.

---

tests/unit/common/mocks-ibverbs.c, line 351 at r12 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Please remove the ifdef here.

it's needed.

---

tests/unit/mr/mr-common.h, line 54 at r16 (raw file):

Previously, janekmi (Jan Michalski) wrote…

#define MOCK_MR_FLAG	(1 << 1)

Just to differentiate these two.

Done.

---

tests/unit/mr/mr-common.h, line 54 at r16 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Please remove one tab from this line.

Done.

[ldorau]

Reviewed 2 of 4 files at r17, 2 of 2 files at r18, 1 of 1 files at r19, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @janekmi)

---

tests/unit/mr/mr-common.h, line 54 at r16 (raw file):

Previously, ewanglong wrote…

Done.

No, this tab is needed here.
@janekmi please check it under vim.
@ewanglong please revert.

[ldorau]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @janekmi)

---

tests/unit/mr/mr-common.h, line 54 at r16 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

No, this tab is needed here.
@janekmi please check it under vim.
@ewanglong please revert.

@janekmi only reviewable.io displays it in a wrong way.

[ldorau]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @janekmi)

---

tests/unit/mr/mr-common.h, line 54 at r16 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

@janekmi only reviewable.io displays it in a wrong way.

@ewanglong please add one tab only, (1 << 1) should stay of course.

[janekmi]

Reviewed 1 of 1 files at r19, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ldorau)

---

tests/unit/mr/mr-common.h, line 54 at r16 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

@ewanglong please add one tab only, (1 << 1) should stay of course.

I have checked it under VSC. Sorry. vim is obviously better. ;-)

[grom72]

Reviewed 2 of 4 files at r17, 2 of 2 files at r18, 1 of 1 files at r19, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ldorau)

[ewanglong]

Reviewable status: 9 of 10 files reviewed, 1 unresolved discussion (waiting on @grom72, @janekmi, and @ldorau)

---

tests/unit/mr/mr-common.h, line 54 at r16 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I have checked it under VSC. Sorry. vim is obviously better. ;-)

Done.

[ldorau]

Reviewed 1 of 1 files at r20, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ewanglong)