-----Original Message----- From: Jason Gunthorpe jgg@ziepe.ca Sent: Monday, November 30, 2020 8:08 AM To: Xiong, Jianxin jianxin.xiong@intel.com Cc: linux-rdma@vger.kernel.org; dri-devel@lists.freedesktop.org; Doug Ledford dledford@redhat.com; Leon Romanovsky leon@kernel.org; Sumit Semwal sumit.semwal@linaro.org; Christian Koenig christian.koenig@amd.com; Vetter, Daniel daniel.vetter@intel.com Subject: Re: [PATCH rdma-core v3 4/6] pyverbs: Add dma-buf based MR support
On Fri, Nov 27, 2020 at 12:55:41PM -0800, Jianxin Xiong wrote:
+function(rdma_multifile_module PY_MODULE MODULE_NAME LINKER_FLAGS)
I think just replace rdma_cython_module with this? No good reason I can see to have two APIs?
rdma_cython_module can handle many modules, but this one is for a single module. If you agree, I can merge the two by slightly tweaking the logic: each module starts with a .pyx file, followed by 0 or more .c and .h files.
- set(ALL_CFILES "")
- foreach(SRC_FILE ${ARGN})
- get_filename_component(FILENAME ${SRC_FILE} NAME_WE)
- get_filename_component(DIR ${SRC_FILE} DIRECTORY)
- get_filename_component(EXT ${SRC_FILE} EXT)
- if (DIR)
set(SRC_PATH "${CMAKE_CURRENT_SOURCE_DIR}/${DIR}")- else()
set(SRC_PATH "${CMAKE_CURRENT_SOURCE_DIR}")- endif()
- if (${EXT} STREQUAL ".pyx")
set(PYX "${SRC_PATH}/${FILENAME}.pyx")set(CFILE "${CMAKE_CURRENT_BINARY_DIR}/${FILENAME}.c")include_directories(${PYTHON_INCLUDE_DIRS})add_custom_command(OUTPUT "${CFILE}"MAIN_DEPENDENCY "${PYX}"COMMAND ${CYTHON_EXECUTABLE} "${PYX}" -o "${CFILE}""-I${PYTHON_INCLUDE_DIRS}"COMMENT "Cythonizing ${PYX}")set(ALL_CFILES "${ALL_CFILES};${CFILE}")- elseif(${EXT} STREQUAL ".c")
set(CFILE_ORIG "${SRC_PATH}/${FILENAME}.c")set(CFILE "${CMAKE_CURRENT_BINARY_DIR}/${FILENAME}.c")if (NOT ${CFILE_ORIG} STREQUAL ${CFILE})rdma_create_symlink("${CFILE_ORIG}" "${CFILE}")endif()Why does this need the create_symlink? The compiler should work OK from the source file?
You are right, the link for .c is not necessary, but the link for .h is needed.
set(ALL_CFILES "${ALL_CFILES};${CFILE}")- elseif(${EXT} STREQUAL ".h")
set(HFILE_ORIG "${SRC_PATH}/${FILENAME}.h")set(HFILE "${CMAKE_CURRENT_BINARY_DIR}/${FILENAME}.h")if (NOT ${HFILE_ORIG} STREQUAL ${HFILE})rdma_create_symlink("${HFILE_ORIG}" "${HFILE}")Here too? You probably don't need to specify h files at all, at worst they should only be used with publish_internal_headers
Without the .h link, the compiler fail to find the header file (both dmabuf_alloc.c and the generated "dmabuf.c" contain #include "dmabuf_alloc.h").
endif()- else()
continue()- endif()
- endforeach()
- string(REGEX REPLACE "\.so$" "" SONAME
- "${MODULE_NAME}${CMAKE_PYTHON_SO_SUFFIX}")
- add_library(${SONAME} SHARED ${ALL_CFILES})
- set_target_properties(${SONAME} PROPERTIES
- COMPILE_FLAGS "${CMAKE_C_FLAGS} -fPIC -fno-strict-aliasing -Wno-unused-function -Wno-redundant-decls -Wno-shadow -Wno-
cast-function-type -Wno-implicit-fallthrough -Wno-unknown-warning -Wno-unknown-warning-option -Wno-deprecated-declarations ${NO_VAR_TRACKING_FLAGS}"
Ugh, you copy and pasted this, but it shouldn't have existed in the first place. Compiler arguments like this should not be specified manually. I should fix it..
Also you should cc edward on all this pyverbs stuff, he knows it all very well
Will add Edward next time. He commented a lot on the PR at github. The current github PR is in sync with this version.
It all looks reasonable to me
Jason