Messages in this thread | | | Subject | Re: [PATCH v2 6/9] xen/gntdev: Add initial support for dma-buf UAPI | From | Oleksandr Andrushchenko <> | Date | Fri, 8 Jun 2018 14:38:31 +0300 |
| |
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
| |