[PATCH rdma-core v3 4/6] pyverbs: Add dma-buf based MR support

[Xiong, Jianxin]

> -----Original Message-----
> From: Jason Gunthorpe <jgg at ziepe.ca>
> Sent: Monday, November 30, 2020 8:08 AM
> To: Xiong, Jianxin <jianxin.xiong at intel.com>
> Cc: linux-rdma at vger.kernel.org; dri-devel at lists.freedesktop.org; Doug Ledford <dledford at redhat.com>; Leon Romanovsky
> <leon at kernel.org>; Sumit Semwal <sumit.semwal at linaro.org>; Christian Koenig <christian.koenig at amd.com>; Vetter, Daniel
> <daniel.vetter at 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