vmw_pvrdma: Add SRQ support - broke rdma-core

The commit 25f07300bfb7 ("vmw_pvrdma: Add SRQ support") causes to build failures in Travis CI [1] and on my system too [2].

Please fix.

Thanks

[1] https://travis-ci.org/linux-rdma/rdma-core/builds/301651144
[2]

-- Missing Optional Items:
-- Valgrind memcheck.h NOT enabled
-- Valgrind drd.h NOT enabled
-- Configuring done
-- Generating done
-- Build files have been written to: /home/leonro/src/rdma-core/build
[1/81] Linking C shared module lib/libhfi1verbs-rdmav16.so
[2/81] Building C object providers/vmw_pvrdma/CMakeFiles/vmw_pvrdma-rdmav16.dir/pvrdma_main.c.o
FAILED: providers/vmw_pvrdma/CMakeFiles/vmw_pvrdma-rdmav16.dir/pvrdma_main.c.o
/usr/lib/ccache/bin/cc -Dvmw_pvrdma_rdmav16_EXPORTS -Iinclude -I/usr/include/libnl3 -Wall -Wextra -Wno-sign-compare -Wno-unused-parameter -Wmissing-prototypes -Wmissing-declarations -Wwrite-strings -Wformat=2 -Wshadow -Wstrict-prototypes -Wold-style-definition -Wredundant-decls -O2 -g -fPIC -std=gnu11 -MD -MT providers/vmw_pvrdma/CMakeFiles/vmw_pvrdma-rdmav16.dir/pvrdma_main.c.o -MF providers/vmw_pvrdma/CMakeFiles/vmw_pvrdma-rdmav16.dir/pvrdma_main.c.o.d -o providers/vmw_pvrdma/CMakeFiles/vmw_pvrdma-rdmav16.dir/pvrdma_main.c.o -c ../providers/vmw_pvrdma/pvrdma_main.c
In file included from ../providers/vmw_pvrdma/pvrdma_main.c:46:0:
../providers/vmw_pvrdma/pvrdma.h: In function ‘pvrdma_write_uar_srq’:
../providers/vmw_pvrdma/pvrdma.h:244:20: error: ‘PVRDMA_UAR_SRQ_OFFSET’ undeclared (first use in this function); did you mean ‘PVRDMA_UAR_CQ_OFFSET’?

  *(__le32 *)(uar + PVRDMA_UAR_SRQ_OFFSET) = htole32(value);
                    ^~~~~~~~~~~~~~~~~~~~~
                    PVRDMA_UAR_CQ_OFFSET

../providers/vmw_pvrdma/pvrdma.h:244:20: note: each undeclared identifier is reported only once for each function it appears in [3/81] Building C object providers/vmw_pvrdma/CMakeFiles/vmw_pvrdma-rdmav16.dir/qp.c.o
FAILED: providers/vmw_pvrdma/CMakeFiles/vmw_pvrdma-rdmav16.dir/qp.c.o
/usr/lib/ccache/bin/cc -Dvmw_pvrdma_rdmav16_EXPORTS -Iinclude -I/usr/include/libnl3 -Wall -Wextra -Wno-sign-compare -Wno-unused-parameter -Wmissing-prototypes -Wmissing-declarations -Wwrite-strings -Wformat=2 -Wshadow -Wstrict-prototypes -Wold-style-definition -Wredundant-decls -O2 -g -fPIC -std=gnu11 -MD -MT providers/vmw_pvrdma/CMakeFiles/vmw_pvrdma-rdmav16.dir/qp.c.o -MF providers/vmw_pvrdma/CMakeFiles/vmw_pvrdma-rdmav16.dir/qp.c.o.d -o providers/vmw_pvrdma/CMakeFiles/vmw_pvrdma-rdmav16.dir/qp.c.o -c ../providers/vmw_pvrdma/qp.c
In file included from ../providers/vmw_pvrdma/qp.c:48:0:
../providers/vmw_pvrdma/pvrdma.h: In function ‘pvrdma_write_uar_srq’:
../providers/vmw_pvrdma/pvrdma.h:244:20: error: ‘PVRDMA_UAR_SRQ_OFFSET’ undeclared (first use in this function); did you mean ‘PVRDMA_UAR_CQ_OFFSET’?

  *(__le32 *)(uar + PVRDMA_UAR_SRQ_OFFSET) = htole32(value);
                    ^~~~~~~~~~~~~~~~~~~~~
                    PVRDMA_UAR_CQ_OFFSET
../providers/vmw_pvrdma/pvrdma.h:244:20: note: each undeclared identifier is reported only once for each function it appears in
../providers/vmw_pvrdma/qp.c: In function ‘pvrdma_create_srq’:
../providers/vmw_pvrdma/qp.c:141:12: error: ‘struct pvrdma_create_srq’ has no member named ‘buf_size’; did you mean ‘buf_addr’?
  cmd.udata.buf_size = srq->buf.length;
            ^~~~~~~~
            buf_addr
../providers/vmw_pvrdma/qp.c: In function ‘pvrdma_post_srq_recv’:
../providers/vmw_pvrdma/qp.c:704:10: error: ‘PVRDMA_UAR_SRQ_RECV’ undeclared (first use in this function); did you mean ‘PVRDMA_UAR_QP_RECV’?
          PVRDMA_UAR_SRQ_RECV | srq->srqn);
          ^~~~~~~~~~~~~~~~~~~
          PVRDMA_UAR_QP_RECV

[4/81] Building C object providers/vmw_pvrdma/CMakeFiles/vmw_pvrdma-rdmav16.dir/verbs.c.o
FAILED: providers/vmw_pvrdma/CMakeFiles/vmw_pvrdma-rdmav16.dir/verbs.c.o
/usr/lib/ccache/bin/cc -Dvmw_pvrdma_rdmav16_EXPORTS -Iinclude -I/usr/include/libnl3 -Wall -Wextra -Wno-sign-compare -Wno-unused-parameter -Wmissing-prototypes -Wmissing-declarations -Wwrite-strings -Wformat=2 -Wshadow -Wstrict-prototypes -Wold-style-definition -Wredundant-decls -O2 -g -fPIC -std=gnu11 -MD -MT providers/vmw_pvrdma/CMakeFiles/vmw_pvrdma-rdmav16.dir/verbs.c.o -MF providers/vmw_pvrdma/CMakeFiles/vmw_pvrdma-rdmav16.dir/verbs.c.o.d -o providers/vmw_pvrdma/CMakeFiles/vmw_pvrdma-rdmav16.dir/verbs.c.o -c ../providers/vmw_pvrdma/verbs.c
In file included from ../providers/vmw_pvrdma/verbs.c:47:0:
../providers/vmw_pvrdma/pvrdma.h: In function ‘pvrdma_write_uar_srq’:
../providers/vmw_pvrdma/pvrdma.h:244:20: error: ‘PVRDMA_UAR_SRQ_OFFSET’ undeclared (first use in this function); did you mean ‘PVRDMA_UAR_CQ_OFFSET’?

  *(__le32 *)(uar + PVRDMA_UAR_SRQ_OFFSET) = htole32(value);
                    ^~~~~~~~~~~~~~~~~~~~~
                    PVRDMA_UAR_CQ_OFFSET

../providers/vmw_pvrdma/pvrdma.h:244:20: note: each undeclared identifier is reported only once for each function it appears in [5/81] Building C object providers/vmw_pvrdma/CMakeFiles/vmw_pvrdma-rdmav16.dir/cq.c.o
FAILED: providers/vmw_pvrdma/CMakeFiles/vmw_pvrdma-rdmav16.dir/cq.c.o
/usr/lib/ccache/bin/cc -Dvmw_pvrdma_rdmav16_EXPORTS -Iinclude -I/usr/include/libnl3 -Wall -Wextra -Wno-sign-compare -Wno-unused-parameter -Wmissing-prototypes -Wmissing-declarations -Wwrite-strings -Wformat=2 -Wshadow -Wstrict-prototypes -Wold-style-definition -Wredundant-decls -O2 -g -fPIC -std=gnu11 -MD -MT providers/vmw_pvrdma/CMakeFiles/vmw_pvrdma-rdmav16.dir/cq.c.o -MF providers/vmw_pvrdma/CMakeFiles/vmw_pvrdma-rdmav16.dir/cq.c.o.d -o providers/vmw_pvrdma/CMakeFiles/vmw_pvrdma-rdmav16.dir/cq.c.o -c ../providers/vmw_pvrdma/cq.c
In file included from ../providers/vmw_pvrdma/cq.c:48:0:
../providers/vmw_pvrdma/pvrdma.h: In function ‘pvrdma_write_uar_srq’:
../providers/vmw_pvrdma/pvrdma.h:244:20: error: ‘PVRDMA_UAR_SRQ_OFFSET’ undeclared (first use in this function); did you mean ‘PVRDMA_UAR_CQ_OFFSET’?

  *(__le32 *)(uar + PVRDMA_UAR_SRQ_OFFSET) = htole32(value);
                    ^~~~~~~~~~~~~~~~~~~~~
                    PVRDMA_UAR_CQ_OFFSET

../providers/vmw_pvrdma/pvrdma.h:244:20: note: each undeclared identifier is reported only once for each function it appears in [6/81] Building C object providers/ipathverbs/CMakeFiles/ipathverbs-rdmav16.dir/ipathverbs.c.o
[7/81] Building C object providers/ipathverbs/CMakeFiles/ipathverbs-rdmav16.dir/verbs.c.o
ninja: build stopped: subcommand failed.
Thanks

I reverted the commit, please fix compilation error and resubmit.

Commit: 32bf4efb8605 ("Revert "vmw_pvrdma: Add SRQ support" because it causes build failures") Thanks

Just as a pointer, it fails on the systems with kernel's include/uapi/rdma/vmw_pvrdma-abi.h file available, so fixup is not applied but without your definition yet.

Thanks for the notice, I should have tried building on a kernel without the updated ABI header. We had a question though, what would be the best approach to fix this? We didn't bump up the ABI version after discussion on the pull request. Is there a way for the fixup to be included if there is an ABI version mismatch, or if a macro is not defined (in this case, PVRDMA_UAR_SRQ_OFFSET)? I thought that we'd use the fixup over the ABI header exposed by the kernel, but my assumption there was wrong.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

The cmake scheme always uses the distro header if the distro header passes the 'test'. Till now the test for pvrdma.h was existance only.

Now that you depend on new stuff, you have to update the test to see if the header is new enough.

Correct.

Look in buildlib/RDMA_LinuxHeaders.cmake and copy something like the rdma/rdma_netlink.h line, but test for PVRDMA_UAR_SRQ_OFFSET.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Thanks for your suggestion, Jason. I've opened up a new pull request that includes the original commit for SRQ support that has been reverted, along with the one line change you suggested. Let me know if you prefer something different. I've run the travis build on kernels both with and without the new ABI header as well.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

You should swap the order of the two patches and move the change of buildlib/fixup-include/rdma-vmw_pvrdma-abi.h to the other patch.

Then the description of the abi.h patch would just be:

'Update rdma-core to the latest kernel pvrdma-abi.h header'

Then everything is more self contained.

You can do this and force push a update to your PR.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Got it, I've done a force push with the new commits. There were two other items that were in the kernel ABI header and not in the fixup-include header, so I've also fixed that.

Unfortunately, I also missed adding the macro I am testing for in RDMA_LinuxHeaders.cmake in the kernel ABI header ): so I sent out a patch to fix that. This should not be a problem, as the test for PVRDMA_UAR_SRQ_OFFSET will make sure we use the header from fixup-include for now, until the kernel patch is accepted.

Going forward, I'll copy the ABI header file to make sure they are synced up and I don't make this mistake again.

Thanks!
Bryan

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

No, it is a problem.
You sent two patches, while the PR passes the compilation on my machine, the first patch alone doesn't.

Every patch should be standalone.

Please fix.

I've updated the PR after taking in Jason's suggestion of swapping the order of the patches that I missed, and ran travis builds on both commits. The travis build on the PR is failing, but it looks like the top of tree travis build is failing as well.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Please move the test update to the prior patch, I left a note on github to be clear.

Travis should be fixed right away.

Please force push the change and we should be able to merge it!

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html