Re: [PATCH v3 8/9] xen/gntdev: Implement dma-buf export functionality

[Boris Ostrovsky]

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(&priv->lock);
>>> +ÂÂÂ list_del(&map->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(&priv->lock);
> ÂÂÂ ÂÂÂ list_del(&map->next);
> ÂÂÂ ÂÂÂ mutex_unlock(&priv->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