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

[Oleksandr Andrushchenko]

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(&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