Re: [Xen-devel] [PATCH v3 8/9] xen/gntdev: Implement dma-buf export functionality
On 06/14/2018 01:19 AM, Boris Ostrovsky wrote:
On 06/13/2018 07:57 AM, Oleksandr Andrushchenko wrote:
On 06/13/2018 05:58 AM, Boris Ostrovsky wrote:
On 06/12/2018 09:41 AM, Oleksandr Andrushchenko wrote:
+
+static struct gntdev_dmabuf *
+dmabuf_exp_wait_obj_get_by_fd(struct gntdev_dmabuf_priv *priv, int fd)
The name of this routine implies (to me) that we are getting a wait
object but IIUIC we are getting a gntdev_dmabuf that we are going to
later associate with a wait object.
How about dmabuf_exp_wait_obj_get_dmabuf_by_fd?
I would like to keep function prefixes, e.g. dmabuf_exp_wait_obj_
just to show to which functionality a routine belongs.
That's too long IMO. If you really want to keep the prefix then let's
keep this the way it is. Maybe it's just me who read it that way.
I'll rename it to dmabuf_exp_wait_obj_get_dmabuf.
As it already wants fd it seems to be clear that
the lookup will be based on it.
+#ifdef CONFIG_XEN_GNTDEV_DMABUF
+void gntdev_remove_map(struct gntdev_priv *priv, struct
gntdev_grant_map *map)
+{
+ mutex_lock(>lock);
+ list_del(>next);
+ gntdev_put_map(NULL /* already removed */, map);
Why not pass call gntdev_put_map(priv, map) and then not have this
routine at all?
Well, I wish I could, but the main difference when calling
gntdev_put_map(priv, map)
with priv != NULL and my code is that:
void gntdev_put_map(struct gntdev_priv *priv, struct gntdev_grant_map
*map)
{
[...]
if (populate_freeable_maps && priv) {
^
mutex_lock(>lock);
list_del(>next);
mutex_unlock(>lock);
}
[...]
}
and
#define populate_freeable_maps use_ptemod
^^
which means the map will never be removed from the list in my case
because use_ptemod == false for dma-buf.
This is why I do that by hand, e.g. remove the item from the list
and then pass NULL for priv.
Also, I will remove gntdev_remove_map as I can now access
priv->lock and gntdev_put_map directly form gntdev-dmabuf.c
Yes, that's a good idea.
I really dislike the fact that we are taking a lock here that
gntdev_put_map() takes as well, although not with NULL argument. (And
yes, I see that gntdev_release() does it too.)
This can be re-factored later I guess?
OK.
-boris
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 8/9] xen/gntdev: Implement dma-buf export functionality
On 06/13/2018 07:57 AM, Oleksandr Andrushchenko wrote:
> On 06/13/2018 05:58 AM, Boris Ostrovsky wrote:
>>
>>
>> On 06/12/2018 09:41 AM, Oleksandr Andrushchenko wrote:
>>>
>>> +
>>> +static struct gntdev_dmabuf *
>>> +dmabuf_exp_wait_obj_get_by_fd(struct gntdev_dmabuf_priv *priv, int fd)
>>
>>
>> The name of this routine implies (to me) that we are getting a wait
>> object but IIUIC we are getting a gntdev_dmabuf that we are going to
>> later associate with a wait object.
>>
> How about dmabuf_exp_wait_obj_get_dmabuf_by_fd?
> I would like to keep function prefixes, e.g. dmabuf_exp_wait_obj_
> just to show to which functionality a routine belongs.
That's too long IMO. If you really want to keep the prefix then let's
keep this the way it is. Maybe it's just me who read it that way.
>>
>>>
>>> +#ifdef CONFIG_XEN_GNTDEV_DMABUF
>>> +void gntdev_remove_map(struct gntdev_priv *priv, struct
>>> gntdev_grant_map *map)
>>> +{
>>> + mutex_lock(>lock);
>>> + list_del(>next);
>>> + gntdev_put_map(NULL /* already removed */, map);
>>
>>
>> Why not pass call gntdev_put_map(priv, map) and then not have this
>> routine at all?
>>
> Well, I wish I could, but the main difference when calling
> gntdev_put_map(priv, map)
> with priv != NULL and my code is that:
>
> void gntdev_put_map(struct gntdev_priv *priv, struct gntdev_grant_map
> *map)
> {
> [...]
> if (populate_freeable_maps && priv) {
> ^
> mutex_lock(>lock);
> list_del(>next);
> mutex_unlock(>lock);
> }
> [...]
> }
>
> and
>
> #define populate_freeable_maps use_ptemod
> ^^
> which means the map will never be removed from the list in my case
> because use_ptemod == false for dma-buf.
> This is why I do that by hand, e.g. remove the item from the list
> and then pass NULL for priv.
>
> Also, I will remove gntdev_remove_map as I can now access
> priv->lock and gntdev_put_map directly form gntdev-dmabuf.c
Yes, that's a good idea.
>
>> I really dislike the fact that we are taking a lock here that
>> gntdev_put_map() takes as well, although not with NULL argument. (And
>> yes, I see that gntdev_release() does it too.)
>>
> This can be re-factored later I guess?
OK.
-boris
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 8/9] xen/gntdev: Implement dma-buf export functionality
On 06/13/2018 05:58 AM, Boris Ostrovsky wrote:
On 06/12/2018 09:41 AM, Oleksandr Andrushchenko wrote:
From: Oleksandr Andrushchenko
1. Create a dma-buf from grant references provided by the foreign
domain. By default dma-buf is backed by system memory pages, but
by providing GNTDEV_DMA_FLAG_XXX flags it can also be created
as a DMA write-combine/coherent buffer, e.g. allocated with
corresponding dma_alloc_xxx API.
Export the resulting buffer as a new dma-buf.
2. Implement waiting for the dma-buf to be released: block until the
dma-buf with the file descriptor provided is released.
If within the time-out provided the buffer is not released then
-ETIMEDOUT error is returned. If the buffer with the file descriptor
does not exist or has already been released, then -ENOENT is
returned. For valid file descriptors this must not be treated as
error.
3. Make gntdev's common code and structures available to dma-buf.
Signed-off-by: Oleksandr Andrushchenko
---
drivers/xen/gntdev-common.h | 4 +
drivers/xen/gntdev-dmabuf.c | 470 +++-
drivers/xen/gntdev.c | 10 +
3 files changed, 482 insertions(+), 2 deletions(-)
diff --git a/drivers/xen/gntdev-common.h b/drivers/xen/gntdev-common.h
index a3408fd39b07..72f80dbce861 100644
--- a/drivers/xen/gntdev-common.h
+++ b/drivers/xen/gntdev-common.h
@@ -89,4 +89,8 @@ bool gntdev_account_mapped_pages(int count);
int gntdev_map_grant_pages(struct gntdev_grant_map *map);
+#ifdef CONFIG_XEN_GNTDEV_DMABUF
+void gntdev_remove_map(struct gntdev_priv *priv, struct
gntdev_grant_map *map);
+#endif
+
#endif
diff --git a/drivers/xen/gntdev-dmabuf.c b/drivers/xen/gntdev-dmabuf.c
index dc57c6a25525..84cba67c6ad7 100644
--- a/drivers/xen/gntdev-dmabuf.c
+++ b/drivers/xen/gntdev-dmabuf.c
@@ -3,13 +3,53 @@
/*
* Xen dma-buf functionality for gntdev.
*
+ * DMA buffer implementation is based on drivers/gpu/drm/drm_prime.c.
+ *
* Copyright (c) 2018 Oleksandr Andrushchenko, EPAM Systems Inc.
*/
+#include
#include
+#include
+#include
+
+#include "gntdev-common.h"
#include "gntdev-dmabuf.h"
+struct gntdev_dmabuf {
+ struct gntdev_dmabuf_priv *priv;
+ struct dma_buf *dmabuf;
+ struct list_head next;
+ int fd;
+
+ union {
+ struct {
+ /* Exported buffers are reference counted. */
+ struct kref refcount;
+
+ struct gntdev_priv *priv;
+ struct gntdev_grant_map *map;
+ } exp;
+ } u;
+
+ /* Number of pages this buffer has. */
+ int nr_pages;
+ /* Pages of this buffer. */
+ struct page **pages;
+};
+
+struct gntdev_dmabuf_wait_obj {
+ struct list_head next;
+ struct gntdev_dmabuf *gntdev_dmabuf;
+ struct completion completion;
+};
+
+struct gntdev_dmabuf_attachment {
+ struct sg_table *sgt;
+ enum dma_data_direction dir;
+};
+
struct gntdev_dmabuf_priv {
/* List of exported DMA buffers. */
struct list_head exp_list;
@@ -23,17 +63,439 @@ struct gntdev_dmabuf_priv {
/* Implementation of wait for exported DMA buffer to be released. */
+static void dmabuf_exp_release(struct kref *kref);
+
+static struct gntdev_dmabuf_wait_obj *
+dmabuf_exp_wait_obj_new(struct gntdev_dmabuf_priv *priv,
+ struct gntdev_dmabuf *gntdev_dmabuf)
+{
+ struct gntdev_dmabuf_wait_obj *obj;
+
+ obj = kzalloc(sizeof(*obj), GFP_KERNEL);
+ if (!obj)
+ return ERR_PTR(-ENOMEM);
+
+ init_completion(>completion);
+ obj->gntdev_dmabuf = gntdev_dmabuf;
+
+ mutex_lock(>lock);
+ list_add(>next, >exp_wait_list);
+ /* Put our reference and wait for gntdev_dmabuf's release to
fire. */
+ kref_put(_dmabuf->u.exp.refcount, dmabuf_exp_release);
+ mutex_unlock(>lock);
+ return obj;
+}
+
+static void dmabuf_exp_wait_obj_free(struct gntdev_dmabuf_priv *priv,
+ struct gntdev_dmabuf_wait_obj *obj)
+{
+ struct gntdev_dmabuf_wait_obj *cur_obj, *q;
+
+ mutex_lock(>lock);
+ list_for_each_entry_safe(cur_obj, q, >exp_wait_list, next)
+ if (cur_obj == obj) {
+ list_del(>next);
+ kfree(obj);
+ break;
+ }
+ mutex_unlock(>lock);
Do we really need to walk the list?
It can be deleted without walking the list and no reason to do that walk.
Just an example of over-engineering here, thank you for spotting this.
And if we do, do we need the safe variant of the walk? We are holding
the lock. Here and elsewhere.
You are perfectly right. I will not use safe variant of the walk, no
need for that
+}
+
+static int dmabuf_exp_wait_obj_wait(struct gntdev_dmabuf_wait_obj *obj,
+ u32 wait_to_ms)
+{
+ if (wait_for_completion_timeout(>completion,
+ msecs_to_jiffies(wait_to_ms)) <= 0)
+ return -ETIMEDOUT;
+
+ return 0;
+}
+
+static void dmabuf_exp_wait_obj_signal(struct gntdev_dmabuf_priv *priv,
+ struct gntdev_dmabuf
Re: [Xen-devel] [PATCH v3 8/9] xen/gntdev: Implement dma-buf export functionality
On 06/12/2018 09:41 AM, Oleksandr Andrushchenko wrote:
From: Oleksandr Andrushchenko
1. Create a dma-buf from grant references provided by the foreign
domain. By default dma-buf is backed by system memory pages, but
by providing GNTDEV_DMA_FLAG_XXX flags it can also be created
as a DMA write-combine/coherent buffer, e.g. allocated with
corresponding dma_alloc_xxx API.
Export the resulting buffer as a new dma-buf.
2. Implement waiting for the dma-buf to be released: block until the
dma-buf with the file descriptor provided is released.
If within the time-out provided the buffer is not released then
-ETIMEDOUT error is returned. If the buffer with the file descriptor
does not exist or has already been released, then -ENOENT is
returned. For valid file descriptors this must not be treated as
error.
3. Make gntdev's common code and structures available to dma-buf.
Signed-off-by: Oleksandr Andrushchenko
---
drivers/xen/gntdev-common.h | 4 +
drivers/xen/gntdev-dmabuf.c | 470 +++-
drivers/xen/gntdev.c| 10 +
3 files changed, 482 insertions(+), 2 deletions(-)
diff --git a/drivers/xen/gntdev-common.h b/drivers/xen/gntdev-common.h
index a3408fd39b07..72f80dbce861 100644
--- a/drivers/xen/gntdev-common.h
+++ b/drivers/xen/gntdev-common.h
@@ -89,4 +89,8 @@ bool gntdev_account_mapped_pages(int count);
int gntdev_map_grant_pages(struct gntdev_grant_map *map);
+#ifdef CONFIG_XEN_GNTDEV_DMABUF
+void gntdev_remove_map(struct gntdev_priv *priv, struct gntdev_grant_map *map);
+#endif
+
#endif
diff --git a/drivers/xen/gntdev-dmabuf.c b/drivers/xen/gntdev-dmabuf.c
index dc57c6a25525..84cba67c6ad7 100644
--- a/drivers/xen/gntdev-dmabuf.c
+++ b/drivers/xen/gntdev-dmabuf.c
@@ -3,13 +3,53 @@
/*
* Xen dma-buf functionality for gntdev.
*
+ * DMA buffer implementation is based on drivers/gpu/drm/drm_prime.c.
+ *
* Copyright (c) 2018 Oleksandr Andrushchenko, EPAM Systems Inc.
*/
+#include
#include
+#include
+#include
+
+#include "gntdev-common.h"
#include "gntdev-dmabuf.h"
+struct gntdev_dmabuf {
+ struct gntdev_dmabuf_priv *priv;
+ struct dma_buf *dmabuf;
+ struct list_head next;
+ int fd;
+
+ union {
+ struct {
+ /* Exported buffers are reference counted. */
+ struct kref refcount;
+
+ struct gntdev_priv *priv;
+ struct gntdev_grant_map *map;
+ } exp;
+ } u;
+
+ /* Number of pages this buffer has. */
+ int nr_pages;
+ /* Pages of this buffer. */
+ struct page **pages;
+};
+
+struct gntdev_dmabuf_wait_obj {
+ struct list_head next;
+ struct gntdev_dmabuf *gntdev_dmabuf;
+ struct completion completion;
+};
+
+struct gntdev_dmabuf_attachment {
+ struct sg_table *sgt;
+ enum dma_data_direction dir;
+};
+
struct gntdev_dmabuf_priv {
/* List of exported DMA buffers. */
struct list_head exp_list;
@@ -23,17 +63,439 @@ struct gntdev_dmabuf_priv {
/* Implementation of wait for exported DMA buffer to be released. */
+static void dmabuf_exp_release(struct kref *kref);
+
+static struct gntdev_dmabuf_wait_obj *
+dmabuf_exp_wait_obj_new(struct gntdev_dmabuf_priv *priv,
+ struct gntdev_dmabuf *gntdev_dmabuf)
+{
+ struct gntdev_dmabuf_wait_obj *obj;
+
+ obj = kzalloc(sizeof(*obj), GFP_KERNEL);
+ if (!obj)
+ return ERR_PTR(-ENOMEM);
+
+ init_completion(>completion);
+ obj->gntdev_dmabuf = gntdev_dmabuf;
+
+ mutex_lock(>lock);
+ list_add(>next, >exp_wait_list);
+ /* Put our reference and wait for gntdev_dmabuf's release to fire. */
+ kref_put(_dmabuf->u.exp.refcount, dmabuf_exp_release);
+ mutex_unlock(>lock);
+ return obj;
+}
+
+static void dmabuf_exp_wait_obj_free(struct gntdev_dmabuf_priv *priv,
+struct gntdev_dmabuf_wait_obj *obj)
+{
+ struct gntdev_dmabuf_wait_obj *cur_obj, *q;
+
+ mutex_lock(>lock);
+ list_for_each_entry_safe(cur_obj, q, >exp_wait_list, next)
+ if (cur_obj == obj) {
+ list_del(>next);
+ kfree(obj);
+ break;
+ }
+ mutex_unlock(>lock);
Do we really need to walk the list?
And if we do, do we need the safe variant of the walk? We are holding
the lock. Here and elsewhere.
+}
+
+static int dmabuf_exp_wait_obj_wait(struct gntdev_dmabuf_wait_obj *obj,
+ u32 wait_to_ms)
+{
+ if (wait_for_completion_timeout(>completion,
+ msecs_to_jiffies(wait_to_ms)) <= 0)
+ return -ETIMEDOUT;
+
+ return 0;
+}
+
+static void dmabuf_exp_wait_obj_signal(struct gntdev_dmabuf_priv *priv,
+ struct
[Xen-devel] [PATCH v3 8/9] xen/gntdev: Implement dma-buf export functionality
From: Oleksandr Andrushchenko
1. Create a dma-buf from grant references provided by the foreign
domain. By default dma-buf is backed by system memory pages, but
by providing GNTDEV_DMA_FLAG_XXX flags it can also be created
as a DMA write-combine/coherent buffer, e.g. allocated with
corresponding dma_alloc_xxx API.
Export the resulting buffer as a new dma-buf.
2. Implement waiting for the dma-buf to be released: block until the
dma-buf with the file descriptor provided is released.
If within the time-out provided the buffer is not released then
-ETIMEDOUT error is returned. If the buffer with the file descriptor
does not exist or has already been released, then -ENOENT is
returned. For valid file descriptors this must not be treated as
error.
3. Make gntdev's common code and structures available to dma-buf.
Signed-off-by: Oleksandr Andrushchenko
---
drivers/xen/gntdev-common.h | 4 +
drivers/xen/gntdev-dmabuf.c | 470 +++-
drivers/xen/gntdev.c| 10 +
3 files changed, 482 insertions(+), 2 deletions(-)
diff --git a/drivers/xen/gntdev-common.h b/drivers/xen/gntdev-common.h
index a3408fd39b07..72f80dbce861 100644
--- a/drivers/xen/gntdev-common.h
+++ b/drivers/xen/gntdev-common.h
@@ -89,4 +89,8 @@ bool gntdev_account_mapped_pages(int count);
int gntdev_map_grant_pages(struct gntdev_grant_map *map);
+#ifdef CONFIG_XEN_GNTDEV_DMABUF
+void gntdev_remove_map(struct gntdev_priv *priv, struct gntdev_grant_map *map);
+#endif
+
#endif
diff --git a/drivers/xen/gntdev-dmabuf.c b/drivers/xen/gntdev-dmabuf.c
index dc57c6a25525..84cba67c6ad7 100644
--- a/drivers/xen/gntdev-dmabuf.c
+++ b/drivers/xen/gntdev-dmabuf.c
@@ -3,13 +3,53 @@
/*
* Xen dma-buf functionality for gntdev.
*
+ * DMA buffer implementation is based on drivers/gpu/drm/drm_prime.c.
+ *
* Copyright (c) 2018 Oleksandr Andrushchenko, EPAM Systems Inc.
*/
+#include
#include
+#include
+#include
+
+#include "gntdev-common.h"
#include "gntdev-dmabuf.h"
+struct gntdev_dmabuf {
+ struct gntdev_dmabuf_priv *priv;
+ struct dma_buf *dmabuf;
+ struct list_head next;
+ int fd;
+
+ union {
+ struct {
+ /* Exported buffers are reference counted. */
+ struct kref refcount;
+
+ struct gntdev_priv *priv;
+ struct gntdev_grant_map *map;
+ } exp;
+ } u;
+
+ /* Number of pages this buffer has. */
+ int nr_pages;
+ /* Pages of this buffer. */
+ struct page **pages;
+};
+
+struct gntdev_dmabuf_wait_obj {
+ struct list_head next;
+ struct gntdev_dmabuf *gntdev_dmabuf;
+ struct completion completion;
+};
+
+struct gntdev_dmabuf_attachment {
+ struct sg_table *sgt;
+ enum dma_data_direction dir;
+};
+
struct gntdev_dmabuf_priv {
/* List of exported DMA buffers. */
struct list_head exp_list;
@@ -23,17 +63,439 @@ struct gntdev_dmabuf_priv {
/* Implementation of wait for exported DMA buffer to be released. */
+static void dmabuf_exp_release(struct kref *kref);
+
+static struct gntdev_dmabuf_wait_obj *
+dmabuf_exp_wait_obj_new(struct gntdev_dmabuf_priv *priv,
+ struct gntdev_dmabuf *gntdev_dmabuf)
+{
+ struct gntdev_dmabuf_wait_obj *obj;
+
+ obj = kzalloc(sizeof(*obj), GFP_KERNEL);
+ if (!obj)
+ return ERR_PTR(-ENOMEM);
+
+ init_completion(>completion);
+ obj->gntdev_dmabuf = gntdev_dmabuf;
+
+ mutex_lock(>lock);
+ list_add(>next, >exp_wait_list);
+ /* Put our reference and wait for gntdev_dmabuf's release to fire. */
+ kref_put(_dmabuf->u.exp.refcount, dmabuf_exp_release);
+ mutex_unlock(>lock);
+ return obj;
+}
+
+static void dmabuf_exp_wait_obj_free(struct gntdev_dmabuf_priv *priv,
+struct gntdev_dmabuf_wait_obj *obj)
+{
+ struct gntdev_dmabuf_wait_obj *cur_obj, *q;
+
+ mutex_lock(>lock);
+ list_for_each_entry_safe(cur_obj, q, >exp_wait_list, next)
+ if (cur_obj == obj) {
+ list_del(>next);
+ kfree(obj);
+ break;
+ }
+ mutex_unlock(>lock);
+}
+
+static int dmabuf_exp_wait_obj_wait(struct gntdev_dmabuf_wait_obj *obj,
+ u32 wait_to_ms)
+{
+ if (wait_for_completion_timeout(>completion,
+ msecs_to_jiffies(wait_to_ms)) <= 0)
+ return -ETIMEDOUT;
+
+ return 0;
+}
+
+static void dmabuf_exp_wait_obj_signal(struct gntdev_dmabuf_priv *priv,
+ struct gntdev_dmabuf *gntdev_dmabuf)
+{
+ struct gntdev_dmabuf_wait_obj *obj, *q;
+
+ list_for_each_entry_safe(obj, q, >exp_wait_list, next)
+ if (obj->gntdev_dmabuf == gntdev_dmabuf) {
+ pr_debug("Found
