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
