Re: [PATCH v2 6/9] xen/gntdev: Add initial support for dma-buf UAPI

[Oleksandr Andrushchenko]

On 06/08/2018 01:26 AM, Boris Ostrovsky wrote:
> On 06/07/2018 03:17 AM, Oleksandr Andrushchenko wrote:
> > On 06/07/2018 12:32 AM, Boris Ostrovsky wrote:
> > > On 06/06/2018 05:06 AM, Oleksandr Andrushchenko wrote:
> > > > On 06/04/2018 11:49 PM, Boris Ostrovsky wrote:
> > > > > > diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> > > > > > index 9813fc440c70..7d58dfb3e5e8 100644
> > > > > > --- a/drivers/xen/gntdev.c
> > > > > > +++ b/drivers/xen/gntdev.c
> > > > > ...
> > > > >
> > > > > >  ÂÂ +#ifdef CONFIG_XEN_GNTDEV_DMABUF
> > > > > This code belongs in gntdev-dmabuf.c.
> > > > The reason I have this code here is that it is heavily
> > > > tied to gntdev's internal functionality, e.g. map/unmap.
> > > > I do not want to extend gntdev's API, so gntdev-dmabuf can
> > > > access these. What is more dma-buf doesn't need to know about
> > > > maps done by gntdev as there is no use of that information
> > > > in gntdev-dmabuf. So, it seems more naturally to have
> > > > dma-buf's related map/unmap code where it is: in gntdev.
> > > Sorry, I don't follow. Why would this require extending the API? It's
> > > just moving routines to a different file that is linked to the same
> > > module.
> > I do understand your intention here and tried to avoid dma-buf
> > related code in gntdev.c as much as possible. So, let me explain
> > my decision in more detail.
> >
> > There are 2 use-cases we have: dma-buf import and export.
> >
> > While importing a dma-buf all the dma-buf related functionality can
> > easily be kept inside gntdev-dmabuf.c w/o any issue as all we need
> > from gntdev.c is dev, dma_buf_fd, count and domid for that.
> >
> > But in case of dma-buf export we need to:
> > 1. struct grant_map *map = gntdev_alloc_map(priv, count, dmabuf_flags);
> > 2. gntdev_add_map(priv, map);
> > 3. Set map->flags
> > 4. ret = map_grant_pages(map);
> > 5. And only now we are all set to export the new dma-buf from
> > *map->pages*
> >
> > So, until 5) we use private gtndev.c's API not exported to outside world:
> > a. struct grant_map
> > b. static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv,
> > int count,
> >  ÂÂÂ ÂÂÂ ÂÂÂ ÂÂÂ ÂÂÂ Â int dma_flags)
> > c. static void gntdev_add_map(struct gntdev_priv *priv, struct
> > grant_map *add)
> > d. static int map_grant_pages(struct grant_map *map)
> >
> > Thus, all the above cannot be accessed from gntdev-dmabuf.c
> > This is why I say that gntdev.c's API will need to be extended to
> > provide the above
> > a-d if we want all dma-buf export code to leave in gntdev-dmabuf.c.
>
>
> I still don't understand why you feel this would be extending the API.
> These routines and the struct can be declared in local header file and
> this header file will not be visible to anyone but gntdev.c and
> gntdev-dmabuf.c.
Ok, this is what I meant: I will need to move private structures
and some function prototypes from gntdev.c into a header file,
thus extending its API: before the header nothing were exposed.
Sorry for not being clear here.
>   You can, for example, put this into gntdev-dmabuf.h
> (and then rename it to something else, like gntdev-common.h).
Sure, I will move all I need into that shared header
> > But that doesn't seem good to me and what is more a-d are really
> > gntdev.c's
> > functionality, not dma-buf's which only needs pages and doesn't really
> > care from
> > where those come.
> > That was the reason I partitioned export into 2 chunks: gntdev +
> > gntdev-dmabuf.
> >
> > You might also ask why importing side does Xen related things
> > (granting references+)
> > in gntdev-dmabuf, not gntdev so it is consistent with the dma-buf
> > exporter?
> > This is because importer uses grant-table's API which seems to be not
> > natural for gntdev.c,
> > so it can leave in gntdev-dmabuf.c which has a use-case for that,
> > while gntdev
> > remains the same.
>
> Yet another reason why this code should be moved: importing and
> exporting functionalities logically belong together. The fat that they
> are implemented using different methods is not relevant IMO.
>
> If you have code which is under ifdef CONFIG_GNTDEV_DMABUF and you have
> file called gntdev-dmabuf.c it sort of implies that this code should
> live in that file (unless that code is intertwined with other code,
> which is not the case here).
Ok, will move as discussed above
> -boris
Thank you,
Oleksandr
> > > Since this is under CONFIG_XEN_GNTDEV_DMABUF then why shouldn't it be in
> > > gntdev-dmabuf.c? In my view that's the file where all dma-related
> > > "stuff" lives.
> > Agree, but IMO grant_map stuff for dma-buf importer is right in its
> > place in gntdev.c
> > and all the rest of dma-buf specifics live in gntdev-dmabuf.c as they
> > should
> > > -boris
> > >
> > >
> > > -boris
> > Thank you,
> > Oleksandr