In the face of upcoming #1087 the possibility of changing the struct rpma_completion is not theoretical anymore. Considering that:

• struct rpma_completion does not provide the initially assumed value and
• the upcoming changes revolts around transferring more and more values from struct ibv_wc to struct rpma_completion where
• extending struct rpma_completion breaks backwards compatibility and may introduce hard to debug issues in the future

the easiest way of addressing both these issues is simply replacing struct rpma_completion with struct ibv_wc.

Idea 1

/* deprecate */
int rpma_cq_get_completion(struct rpma_cq *cq, struct rpma_completion *cmpl);

int rpma_cq_get_wc(struct rpma_cq *cq, struct ibv_wc *wc);

int rpma_wc_get_imm(struct ibv_wc *wc, uint32_t *imm);

from rpma.

Hi @janekmi

extending struct rpma_completion with a connection which the completion comes from

We can talk it on #977.

banning direct access to struct rpma_completion fields and provide getters/setters

Is it necessary to ban direct access to struct rpma_completion fields？ I am worried about that it becomes complex to access all members in struct rpma_completion when struct rpma_completion has a lot of members.

the value of having enum rpma_op

Agreed. I have tried to simplify the conversion from (enum ibv_wc_opcode) opcode to (enum rpma_op) op by PR #979.

Best Regards,
Xiao Yang

from rpma.

Is it necessary to ban direct access to struct rpma_completion fields？ I am worried about that it becomes complex to access all members in struct rpma_completion when struct rpma_completion has a lot of members.

I know it would be less convenient. But if we would agree that changes to this structure in the future might be necessary then we can not keep it open. We have to consider both scenarios carefully.

Agreed. I have tried to simplify the conversion from (enum ibv_wc_opcode) opcode to (enum rpma_op) op by PR #979.

Thanks for the effort. I think if we won't do any advanced conversion we should drop the enum rpma_op entirely since it introduces no value. So the completion structure would look as follows:

struct rpma_completion {
	void *op_context;
	enum ibv_wc_opcode opcode; /* instead of enum rpma_op op */
	uint32_t byte_len;
	enum ibv_wc_status op_status;
	unsigned flags;
	uint32_t imm;
};

Additionally, I think we should put into the documentation what enum ibv_wc_opcode values may one expect when using the librpma API to provide guidelines for the developers.

Alternatively

We can discuss finding a way to have a consistent enum rpma_op output no matter what ibverbs are in use e.g. translating IBV_WC_RDMA_READ to RPMA_OP_FLUSH when the IBV_WC_RDMA_READ completion comes from posted rpma_flush() operation. Note: The solution has to take into account possible user-provided op_context and posting rpma_read() at the same time.

from rpma.

Additionally, I think we should put into the documentation what enum ibv_wc_opcode values may one expect when using the librpma API to provide guidelines for the developers.

Hi @janekmi

1. If enum ibv_wc_opcode is used directly, it is not clear for user to know which rpma operation is completed and all existing examples/upper applications have to be rewritten.
2. If enum ibv_wc_opcode is used directly, RPMA has no chance to report RPMA_OP_FLUSH currently until RDMA has been extended to support FLUSH operation(e.g. IBV_WC_RDMA_FLUSH).

Alternatively

We can discuss finding a way to have a consistent enum rpma_op output no matter what ibverbs are in use e.g. translating IBV_WC_RDMA_READ to RPMA_OP_FLUSH when the IBV_WC_RDMA_READ completion comes from posted rpma_flush() operation. Note: The solution has to take into account possible user-provided op_context and posting rpma_read() at the same time.

Yes, it is necessary to report RPMA_OP_FLUSH properly but hard to distinguish between read and flush completion events.

Best Regards,
Xiao Yang

from rpma.

1. If enum ibv_wc_opcode is used directly, it is not clear for user to know which rpma operation is completed and all existing examples/upper applications have to be rewritten.

Yes. It is a tough decision. But keeping a no-value function in the library is unbearable for me. ;-)

2. If enum ibv_wc_opcode is used directly, RPMA has no chance to report RPMA_OP_FLUSH currently until RDMA has been extended to support FLUSH operation(e.g. IBV_WC_RDMA_FLUSH).

Having RPMA_OP_* we are not doing it now either. This is why we are looking for a way for providing this.

Yes, it is necessary to report RPMA_OP_FLUSH properly but hard to distinguish between read and flush completion events.

A 'hard' is what keeps us running. :-)

from rpma.

/* deprecate */
int rpma_cq_get_completion(struct rpma_cq *cq, struct rpma_completion *cmpl);

Hi @janekmi

Do you want to remove it or keep it in next release?
I hope we can find a way to achieve its original value (i.e. report RPMA_OP_FLUSH -- hide the underlying read/flush operation)

int rpma_wc_get_imm(struct ibv_wc *wc, uint32_t *imm);

I think application can access all members of struct ibv_wc so is the function rpma_wc_get_imm necessary?
If it is necessary, is it ok to define it in src/cq.c ?

from rpma.

@janekmi @grom72 @kilobyte @ldorau

Idea 2

Distinguish the completions of rpma read and rpma flush by the highest bit of wr_id. Ref: #979
Note: In this case, the max value of op_context is limited to 0x7FFFFFFFFFFFFFFF.

#define OP_CONTEXT_HIGHEST_BIT	0x8000000000000000

rpma_read(void *op_context)
{
	rpma_mr_read(op_context);
}

rpma_flush(void *op_context)
{
	/* set the highest bit for rpma flush */
	rpma_mr_read((op_context | OP_CONTEXT_HIGHEST_BIT);
}

rpma_cq_get_completion()
{
	case IBV_WC_RDMA_READ:
		cmpl->op = RPMA_OP_READ;
		cmpl->op = wc.wr_id & OP_CONTEXT_HIGHEST_BIT ?
				RPMA_OP_FLUSH : RPMA_OP_READ;
		break;
}

BTW: If you approve the idea2, I will also distinguish the completions of rpma write and rpma atomic write by the highest bit of wr_id.

from rpma.

Do you want to remove it or keep it in next release?

No. I think it should be removed.

I hope we can find a way to achieve its original value (i.e. report RPMA_OP_FLUSH -- hide the underlying read/flush operation)

We have already discussed the Idea 2 many times internally. Sadly, it is the only light way of achieving the RPMA_OP_FLUSH purpose of existence. But I think it is not worth it. Currently, the op_context size exactly matches the size of a pointer which allows easily querying an unlimited number of information related to the operation. Whereas, when we decide to reserve a single bit out of the op_context you have to build some kind of dictionary or whatnot. The same result can be easily achieved by the end-user without compromising the fundamental role of the op_context which is fully reserved to the end-user.

TLDR: If it can be achieved by the end-user without the drawbacks we would introduce if we decide to help him with this matter I do not see a reason to do it.

int rpma_wc_get_imm(struct ibv_wc *wc, uint32_t *imm);

I think application can access all members of struct ibv_wc so is the function rpma_wc_get_imm necessary?

To-be-discussed. The same result the end-user can achieve simply by using the ntohl() directly. Which maybe we should just recommend.

from rpma.

TLDR: If it can be achieved by the end-user without the drawbacks we would introduce if we decide to help him with this matter I do not see a reason to do it.

@janekmi

Thanks for your detailed explanation. In my opinion, librpma instead of end-user is responsible for distinguishing rpma flush and rpma read. Maybe I'm wrong.

To-be-discussed. The same result the end-user can achieve simply by using the ntohl() directly. Which maybe we should just recommend.

Agreed. It is better to leave it to end-user.

from rpma.

Hi @grom72 @ldorau

I think you still perfer idea1(replace struct rpma_completion with struct ibv_wc). Right?
If right, I will update my PR in this way.

from rpma.

Hi @grom72 @ldorau

Let's look at the latest PR #979

from rpma.

fixed by #1080
fixed by #979

from rpma.