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

From: "Xiong, Jianxin" <jianxin.xiong@intel.com>
To: Jason Gunthorpe <jgg@ziepe.ca>
Cc: "linux-rdma@vger.kernel.org" <linux-rdma@vger.kernel.org>,
	"dri-devel@lists.freedesktop.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
Date: Wed, 2 Dec 2020 01:12:31 +0000	[thread overview]
Message-ID: <MW3PR11MB4555DAFD7546E762F47F5851E5F30@MW3PR11MB4555.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20201202003923.GI5487@ziepe.ca>

> -----Original Message-----
> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Tuesday, December 01, 2020 4:39 PM
> 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 Mon, Nov 30, 2020 at 05:53:39PM +0000, Xiong, Jianxin wrote:
> > > 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.
> 
> Then have rdma_cython_module call some rdam_single_cython_module() multiple times that has this code below?

Mostly like that. Here is an outline:

function(build_one_module PY_MODULE MODULE_NAME ALL_CFILES)
    string(REGEX_REPLACE "\\.so$" "" SONAME ${MODULE_NAME}${CMAKE_PYTHON_SO_SUFFIX}")
    add_library(......)
    set_target_properties(......)
    target_link_libraries(......)
    install(......)
endfunction()

function(rdma_cython_module .......)
    foreach(SRC_FILE ${ARGN})
        ...... # commands to parse file name
        If (${EXT} STREQAL ".pyx")
            If (ALL_CFILES AND MODULE_NAME)
                build_one_module(${PY_MODUE} ${MODULE_NAME} ${ALL_CFILES})
                set(ALL_CFILES "")
                set(MODULE_NAME "")
            endif()
            ...... # commands to convert .pyx to .c
            set(ALL_CFILES "${ALL_CFILES};${CFILE}")
        elseif (${EXT} STREQAL ".c")
            ......
            set(ALL_CFILES "${ALL_CFILES};${CFILE}")
        else()
            continue()
        endif()
    endforeach()
    If (ALL_CFILES AND MODULE_NAME)
        build_one_module(${PY_MODULE} ${MODULE_NAME} ${ALL_CFILES})
     endif()
endfunction()

> 
> > > 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").
> 
> Header files are made 'cross module' using the "publish_internal_headers" command
> 
> But we could also hack in a -I directive to fix up the "" include for the cython outupt..
> 
> But it should not be handled here in the cython module command

Sure. That can be fixed.

> 
> Jason