Fabrics 2/2: Implementation of the fabrics API

[jonasohland]

This is step 2 of merging the fabrics functionality into the main mxl repo. Now we would like to merge the code that implements the api in fabrics.h

Enables transfer of grains and slices using remote memory access, tcp and shared memory on supported hardware.

Future improvements will include audio support and capability management.

Makes #78 obsolete.

Closes #181

NOTE: You must use the following cmake configure flag to enable the Fabric's API build : -DMXL_ENABLE_FABRICS_OFI=on

[KimonHoffmann]

Thanks for this massive contribution!
I looked through everything except for the test implementation and the demo. Most of remarks refer to syntactical aspects.

[KimonHoffmann]

Thanks for this massive contribution!
I looked through everything except for the test implementation and the demo. Most of my remarks refer to syntactical aspects.

[jonasohland]

Thanks for this massive contribution! I looked through everything except for the test implementation and the demo. Most of my remarks refer to syntactical aspects.

Thanks for looking through our code! We really appreciate the effort. I have addressed almost everything, I have only one or two questions and have left your comments regarding those unresolved.
Thanks!

[KimonHoffmann]

Looks good, thank you for addressing my comments!
I noticed two smaller things that still need addressing (use of bit_cast<> and constexpr vs. inline), but the rest looks great.

[KimonHoffmann]

Looks good to me, thanks for the updates!

[KimonHoffmann]

Two small comments on the four commits added since my last review.

[lapointejp]

Seems okay to me. Just a couple typos and wording things to check.
Thank you so very much for contributing this @jonasohland and @mlefebvre1.

[pprayle]

I've run into a couple of issues while trying to get the mxl-fabrics-demo app to stream between initiator and target using the verbs provider.

I'm running on Ubuntu 22.04 with a Mellanox ConnectX 6 DX card. The various verbs packages are at version 2510.0.11-1, installed via Nvidia's doca-roce package version 3.2.1-044000.

I'm not sure if these issues could be fixed by modifying the environment (system packages, OS settings etc). However, I'm describing here the changes I made to the code to make it work for me.

The first error is when the initiator attempts to register the shared memory:

[error] [MemoryRegion.cpp:67] fi_mr_regattr failed: Bad address, code -14

This happens because the flow reader opens the shared memory in read-only mode, but the memory registration requests write access using FI_WRITE flag in RMAGrainEgressProtocolTemplate::registerMemory.

This issue can be temporarily fixed by changing the access mode in Instance::getFlowReader to READ_WRITE.
If this really is the only way to fix the issue, then I guess an additional parameter to the getFlowReader function would be required to specify the access mode?

The second issue is the target receives a value of 0 for the number of valid slices received. The log message shows the following:

[demo.cpp:564] Comitted grain with index=106101956356 current index=106101956357 validSlices=0 flags=0

The value of valid slices is received in the completion data which is handled by RMAGrainIngressProtocol.
In the function processCompletion, a request for the next completion data with the internal buffer is being posted to the endpoint, then the data in the internal buffer is being returned (before it's arrived).
This code snippet shows the issue (comments added by me):

Target::ReadResult RMAGrainIngressProtocol::processCompletion(Endpoint& ep, Completion const& completion)
{
    if (auto data = completion.tryData(); data)
    {
        if (_immDataBuffer)
        {
            // request the next completion data with the internal buffer
            ep.recv(immDataRegion());
            // return the data in the internal buffer (before it's arrived)
            return Target::ReadResult{_immDataBuffer->data};
        }
        else
        {
            return Target::ReadResult{data->data()};
        }
    }

    return {};
}

The issue can be fixed by always returning the data from the current completion, after posting a request for the next one:

Target::ReadResult RMAGrainIngressProtocol::processCompletion(Endpoint& ep, Completion const& completion)
{
    if (auto data = completion.tryData(); data)
    {
        if (_immDataBuffer)
        {
            ep.recv(immDataRegion());
        }

        return Target::ReadResult{data->data()};
    }

    return {};
}

[jonasohland]

Hi! @pprayle

Thanks for your comment!
The second issue is clearly a bug, which we will fix.

To your first point:
We had the same issue on EFA, and their driver team is working on a fix for that. It seems like a similar problem exists for specific hardware/driver combination.
We have successfully tested initiators with read-only memory on Connect-X cards, so maybe this is fixed in an updated driver or firmware for your card?
Otherwise, we would need to file a bug report upstream.

Thanks!
Jonas

[jonasohland]

@pprayle BTW,

Memory registration in libfabric with the FI_WRITE flag should not be the issue.
From the libfabric man pages:

FI_WRITE
              The memory buffer may be used as the source buffer for RMA write and atomic operations on the initiator side.  Note that from the viewpoint of the application, the endpoint is reading from
              the memory buffer and copying the data onto the network.

[KimonHoffmann]

A few small things I stumbled upon.

[KimonHoffmann]

We could get rid of this const qualification, especially in the context of of this declaration. Feels free to keep it for the definition if it proves to be helpful there (even though I personally tend to not use them on by value arguments).

[KimonHoffmann]

We could get rid of this const qualification, especially in the context of of this declaration. Feels free to keep it for the definition if it proves to be helpful there (even though I personally tend to not use them on by value arguments).

[KimonHoffmann]

We could get rid of this const qualification of in_targetInfo, especially in the context of of this declaration. Feels free to keep it for the definition if it proves to be helpful there (even though I personally tend to not use them on by value arguments).

[KimonHoffmann]

Ist this called often? If yes this way of obtaining the token could prove quite expensive.

[KimonHoffmann]

Could use auto with right hand type auto regions = std::vector<Region>{}.

[KimonHoffmann]

Could use auto with right hand type auto regions = std::vector<Region>{}.

[KimonHoffmann]

Could use parentheses around binary subexpressions.

[KimonHoffmann]

Could use a comment as to why those fields are not fetched as numeric values directly.

[KimonHoffmann]

Please use the std:: qualified variant.