verbs: Allow query only device support for QP data in order

[dkkranz]

EFA can support 128 bytes blocks write in-order for some Grace platforms. Move the check on x86 architecture to the mlx5 provider implementation.

Reviewed-by: Michael Margolin [email protected]
Reviewed-by: Yonatan Nachum [email protected]

[jgunthorpe]

I don't think this is right, it would break things like PPC. The platform check is supposed to be in the common code, and the driver check is only about how the device works on PCI.

You should probably just add ARM to the inclusion list, though I imagine there are some wonky ARMs that don't work, they may not matter in practice.

[dkkranz]

Applied your suggestion, thanks

[dkkranz]

@jgunthorpe Kind reminder

[jgunthorpe]

I don't know.. I just feel uneasy about this. ARM is such a wide and varied architecture I don't know if we can really say it is actually reliably in order. Grace might be just because it has 128 byte lines, but that doesn't mean a different ARM chip will be. This could break some of the MPIs running on ARM supercomputers.. Do you think you could limit it just to Grace? Did someone from NVIDIA confirm that this is actually true on Grace?

[jgunthorpe]

I consulted with people here and unfortunately I don't think using ARM platform as a condition is sufficient. Even looking at NVIDIA CPUs this only works on certain CPUs, with certain configurations.

I guess EFA could be special since it really only exists in AWS instances and you guys will do the validation. I'd still strongly recommend that you query your device FW to see if it is supported as I'm not sure you will find all future systems will work this way.

So maybe the right thing is to add a new op query_qp_data_in_order_force() or something like that that skips the architecture check in the core code. I don't really want to have drivers doing architecture checks..

[dkkranz]

@jgunthorpe Please review the last revision that adds a flag to skip the architecture check.

[jgunthorpe]

I didn't mean a flag to userspace, there is nothing userspace can do with this. I ment some kind of flag from EFA to the core code to disable the arch checks in the core code just for EFA.

[mrgolin]

I don't really want to have drivers doing architecture checks..

But this is exactly what you are suggesting now, and I'm not sure why we need to have architecture checks in libibverbs either.

I think there is use for a common way to query whether data is written in order from device perspective only, for instance data polling from GPU isn't necessarily affected by the CPU architecture. Don't you agree?

[dkkranz]

@jgunthorpe Was your concern addressed by @mrgolin on his last comment? Do we have any blocker for merging this?

[jgunthorpe]

No, I don't want architecture checks in drivers. EFA is being very weird and special here by saying that since EFA is only part of a fully engineered cloud system it can know that it only exists in conjunction with, otherwise architecture undefined, behavior.

I think what you want is to just make EFA do this unconditionally so EFA and only EFA would skip the architecture checks in the core code. If you ever deploy EFA on some implementation that doesn't work that way then EFA should report this to the driver through the FW.

[jgunthorpe]

Maybe you could reorganize things so the driver calls out some kind of core helper to test if the CPU/platform is OK for some properties and then just not call that on EFA.

[dkkranz]

The EFA capability will be exposed via direct EFA device attribute, without changing the general API.
Please review the changes.

[jgunthorpe]

Honestly I like this less, we try to reserve DV to be only for cases that are not general. Re-exposing the capability through DV to avoid refactoring the core code does not seem like good justification. Did you have a another reason to use DV?

[mrgolin]

We might not explained the need well, I think what we tried to do is to expose a bit different capability than what ibv_query_qp_data_in_order, that is limited to CPU perspective, currently gives. What we want is something to tell NCCL whether it can use LL128 on GPU regardless of what CPU can assume on data ordering. This isn't specific to EFA and I guess other providers will need it sooner or later, and that's why @dkkranz previously suggested adding a flag to the common verb. I wouldn't want to change ibv_query_qp_data_in_order meaning only for EFA.

[jgunthorpe]

The ability to do last data polling is largely a property of the receiving unit. Yes the verb is intended to cover CPU memory only.

If you want to expose a property it should be if the RDMA device issues PCIe TLPs strictly in order and strictly with non-relaxed ordering. ie it is a PCIe behavior property only. Then you could cross that with some GPU property if it can do last data polling with such TLPs. That could be a general purpose verb.

[mrgolin]

If you want to expose a property it should be if the RDMA device issues PCIe TLPs strictly in order and strictly with non-relaxed ordering.

Yes, this is something RDMA device can tell and it's what I would like to expose as a general purpose verb (or an extension of existing one).

Then you could cross that with some GPU property if it can do last data polling with such TLPs.

I think we better leave this part to upper layers that can be aware of GPU behavior or system topology.

[jgunthorpe]

If this is what you want to do I think a new general verb flag would be fine - report the PCI device behavior only, and explain in the manual this is only half the problem and any software using this must also consider the behavior of the receiving device.

[dkkranz]

Done.

[jgunthorpe]

This function sure has become weird, flags is less of a flags and more of a selector now..

Still, flags has to be validated.. It can only be 0, IBV_QUERY_QP_DATA_IN_ORDER_RETURN_CAPS or IBV_QUERY_QP_DATA_IN_ORDER_DEVICE_ONLY, everything else is a failure and returns 0.

[jgunthorpe]

The commit to have EFA implement this is missing??

[dkkranz]

I fixed the flags check, thanks.
There is no change required in the EFA implementation. The API was only blocked in the common verb check on the host architecture.

[jgunthorpe]

Something is wrong here, you can't just add new API with new driver semantics without changing the existing drivers, somehow. What is the story?

[dkkranz]

This change is backwards compatible. API calls to ibv_query_qp_data_in_order without the new IBV_QUERY_QP_DATA_IN_ORDER_DEVICE_ONLY flag will behave the same way as before. For NCCL to use this API to determine support for the LL128 protocol on arm platforms, its EFA plugin will have to pass the new flag.
I don't understand your concern.

[dkkranz]

API calls with the new flag on arm platforms will bypass the arch restriction and will call the internal EFA verb, which already returns directly the device support for data-in-order write, without any host arch restrictions. Thus, there is no necessary change in the EFA verb.

[mrgolin]

Providers' implementations of query_qp_data_in_order op are expected to return device level only support (network, device internals and PCIe TLPs ordering), what is exactly what ibv_query_qp_data_in_order with the new IBV_QUERY_QP_DATA_IN_ORDER_DEVICE_ONLY flag promises. Am I missing something?

[mrgolin]

Providers' implementations of query_qp_data_in_order op are expected to return device level only support (network, device internals and PCIe TLPs ordering), what is exactly what ibv_query_qp_data_in_order with the new IBV_QUERY_QP_DATA_IN_ORDER_DEVICE_ONLY flag promises. Am I missing something?

@jgunthorpe?