Nouveau - Feb 2015 - [PATCH 2/2] nouveau: Do not add most bo's to the global bo list.

On Wed, Feb 25, 2015 at 8:59 AM, Maarten Lankhorst <
maarten.lankhorst at ubuntu.com> wrote:
> Op 25-02-15 om 15:11 schreef Emil Velikov:
> > On 24 February 2015 at 09:01, Maarten Lankhorst
> > <maarten.lankhorst at ubuntu.com> wrote:
> >> Only add wrapped bo's and bo's that have been exported
through flink or
> dma-buf.
> >> This avoids a lock in the common case, and decreases traversal
needed
> for importing
> >> a dma-buf or flink.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at
ubuntu.com>
> >> ---
> >>  nouveau/nouveau.c | 47
+++++++++++++++++++++++------------------------
> >>  1 file changed, 23 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c
> >> index 1c723b9..d411523 100644
> >> --- a/nouveau/nouveau.c
> >> +++ b/nouveau/nouveau.c
> >> @@ -349,8 +349,8 @@ nouveau_bo_del(struct nouveau_bo *bo)
> >>         struct nouveau_bo_priv *nvbo = nouveau_bo(bo);
> >>         struct drm_gem_close req = { bo->handle };
> >>
> >> -       pthread_mutex_lock(&nvdev->lock);
> >> -       if (nvbo->name) {
> >> +       if (nvbo->head.next) {
> >> +               pthread_mutex_lock(&nvdev->lock);
> >>                 if (atomic_read(&nvbo->refcnt) == 0) {
> >>                         DRMLISTDEL(&nvbo->head);
> >>                         /*
> >> @@ -365,8 +365,6 @@ nouveau_bo_del(struct nouveau_bo *bo)
> >>                 }
> >>                 pthread_mutex_unlock(&nvdev->lock);
> >>         } else {
> >> -               DRMLISTDEL(&nvbo->head);
> >> -               pthread_mutex_unlock(&nvdev->lock);
> >>                 drmIoctl(bo->device->fd,
DRM_IOCTL_GEM_CLOSE, &req);
> >>         }
> >>         if (bo->map)
> >> @@ -379,7 +377,6 @@ nouveau_bo_new(struct nouveau_device *dev,
uint32_t
> flags, uint32_t align,
> >>                uint64_t size, union nouveau_bo_config *config,
> >>                struct nouveau_bo **pbo)
> >>  {
> >> -       struct nouveau_device_priv *nvdev = nouveau_device(dev);
> >>         struct nouveau_bo_priv *nvbo = calloc(1, sizeof(*nvbo));
> >>         struct nouveau_bo *bo = &nvbo->base;
> >>         int ret;
> >> @@ -397,10 +394,6 @@ nouveau_bo_new(struct nouveau_device *dev,
> uint32_t flags, uint32_t align,
> >>                 return ret;
> >>         }
> >>
> >> -       pthread_mutex_lock(&nvdev->lock);
> >> -       DRMLISTADD(&nvbo->head, &nvdev->bo_list);
> >> -       pthread_mutex_unlock(&nvdev->lock);
> >> -
> >>         *pbo = bo;
> >>         return 0;
> >>  }
> >> @@ -457,6 +450,18 @@ nouveau_bo_wrap_locked(struct nouveau_device
*dev,
> uint32_t handle,
> >>         return -ENOMEM;
> >>  }
> >>
> >> +static void
> >> +nouveau_bo_make_global(struct nouveau_bo_priv *nvbo)
> >> +{
> >> +       if (!nvbo->head.next) {
> >> +               struct nouveau_device_priv *nvdev >
nouveau_device(nvbo->base.device);
> >> +               pthread_mutex_lock(&nvdev->lock);
> >> +               if (!nvbo->head.next)
>
I guess the bo_make_global call is not particularly sensitive,
so
> removing's fine with me.
>
I would be worried about the duplicated check. It seems like a "smart"
compiler could cache the value of "nvbo->head.next" (unless marked
as
volatile), rendering the second if() useless. If the field is marked
volatile, then of course, this does not apply.

-Patrick
>
> ~Maarten
> _______________________________________________
> Nouveau mailing list
> Nouveau at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/nouveau
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20150225/f7a136b6/attachment.html>

Op 25-02-15 om 16:04 schreef Patrick Baggett:
> On Wed, Feb 25, 2015 at 8:59 AM, Maarten Lankhorst <
> maarten.lankhorst at ubuntu.com> wrote:
>
>> Op 25-02-15 om 15:11 schreef Emil Velikov:
>>> On 24 February 2015 at 09:01, Maarten Lankhorst
>>> <maarten.lankhorst at ubuntu.com> wrote:
>>>> Only add wrapped bo's and bo's that have been exported
through flink or
>> dma-buf.
>>>> This avoids a lock in the common case, and decreases traversal
needed
>> for importing
>>>> a dma-buf or flink.
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at
ubuntu.com>
>>>> ---
>>>>  nouveau/nouveau.c | 47
+++++++++++++++++++++++------------------------
>>>>  1 file changed, 23 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c
>>>> index 1c723b9..d411523 100644
>>>> --- a/nouveau/nouveau.c
>>>> +++ b/nouveau/nouveau.c
>>>> @@ -349,8 +349,8 @@ nouveau_bo_del(struct nouveau_bo *bo)
>>>>         struct nouveau_bo_priv *nvbo = nouveau_bo(bo);
>>>>         struct drm_gem_close req = { bo->handle };
>>>>
>>>> -       pthread_mutex_lock(&nvdev->lock);
>>>> -       if (nvbo->name) {
>>>> +       if (nvbo->head.next) {
>>>> +               pthread_mutex_lock(&nvdev->lock);
>>>>                 if (atomic_read(&nvbo->refcnt) == 0) {
>>>>                         DRMLISTDEL(&nvbo->head);
>>>>                         /*
>>>> @@ -365,8 +365,6 @@ nouveau_bo_del(struct nouveau_bo *bo)
>>>>                 }
>>>>                 pthread_mutex_unlock(&nvdev->lock);
>>>>         } else {
>>>> -               DRMLISTDEL(&nvbo->head);
>>>> -               pthread_mutex_unlock(&nvdev->lock);
>>>>                 drmIoctl(bo->device->fd,
DRM_IOCTL_GEM_CLOSE, &req);
>>>>         }
>>>>         if (bo->map)
>>>> @@ -379,7 +377,6 @@ nouveau_bo_new(struct nouveau_device *dev,
uint32_t
>> flags, uint32_t align,
>>>>                uint64_t size, union nouveau_bo_config *config,
>>>>                struct nouveau_bo **pbo)
>>>>  {
>>>> -       struct nouveau_device_priv *nvdev =
nouveau_device(dev);
>>>>         struct nouveau_bo_priv *nvbo = calloc(1,
sizeof(*nvbo));
>>>>         struct nouveau_bo *bo = &nvbo->base;
>>>>         int ret;
>>>> @@ -397,10 +394,6 @@ nouveau_bo_new(struct nouveau_device *dev,
>> uint32_t flags, uint32_t align,
>>>>                 return ret;
>>>>         }
>>>>
>>>> -       pthread_mutex_lock(&nvdev->lock);
>>>> -       DRMLISTADD(&nvbo->head, &nvdev->bo_list);
>>>> -       pthread_mutex_unlock(&nvdev->lock);
>>>> -
>>>>         *pbo = bo;
>>>>         return 0;
>>>>  }
>>>> @@ -457,6 +450,18 @@ nouveau_bo_wrap_locked(struct
nouveau_device *dev,
>> uint32_t handle,
>>>>         return -ENOMEM;
>>>>  }
>>>>
>>>> +static void
>>>> +nouveau_bo_make_global(struct nouveau_bo_priv *nvbo)
>>>> +{
>>>> +       if (!nvbo->head.next) {
>>>> +               struct nouveau_device_priv *nvdev >>
nouveau_device(nvbo->base.device);
>>>> +               pthread_mutex_lock(&nvdev->lock);
>>>> +               if (!nvbo->head.next)
> I guess the bo_make_global call is not particularly sensitive, so
>> removing's fine with me.
>>
> I would be worried about the duplicated check. It seems like a
"smart"
> compiler could cache the value of "nvbo->head.next" (unless
marked as
> volatile), rendering the second if() useless. If the field is marked
> volatile, then of course, this does not apply.
>
In that case I would be worried about a compiler that ignores the acquire
semantics of pthread_mutex_lock..

~Maarten

On Wed, Feb 25, 2015 at 9:07 AM, Maarten Lankhorst <
maarten.lankhorst at ubuntu.com> wrote:
> Op 25-02-15 om 16:04 schreef Patrick Baggett:
> > On Wed, Feb 25, 2015 at 8:59 AM, Maarten Lankhorst <
> > maarten.lankhorst at ubuntu.com> wrote:
> >
> >> Op 25-02-15 om 15:11 schreef Emil Velikov:
> >>> On 24 February 2015 at 09:01, Maarten Lankhorst
> >>> <maarten.lankhorst at ubuntu.com> wrote:
> >>>> Only add wrapped bo's and bo's that have been
exported through flink
> or
> >> dma-buf.
> >>>> This avoids a lock in the common case, and decreases
traversal needed
> >> for importing
> >>>> a dma-buf or flink.
> >>>>
> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at
ubuntu.com>
> >>>> ---
> >>>>  nouveau/nouveau.c | 47
> +++++++++++++++++++++++------------------------
> >>>>  1 file changed, 23 insertions(+), 24 deletions(-)
> >>>>
> >>>> diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c
> >>>> index 1c723b9..d411523 100644
> >>>> --- a/nouveau/nouveau.c
> >>>> +++ b/nouveau/nouveau.c
> >>>> @@ -349,8 +349,8 @@ nouveau_bo_del(struct nouveau_bo *bo)
> >>>>         struct nouveau_bo_priv *nvbo = nouveau_bo(bo);
> >>>>         struct drm_gem_close req = { bo->handle };
> >>>>
> >>>> -       pthread_mutex_lock(&nvdev->lock);
> >>>> -       if (nvbo->name) {
> >>>> +       if (nvbo->head.next) {
> >>>> +               pthread_mutex_lock(&nvdev->lock);
> >>>>                 if (atomic_read(&nvbo->refcnt) ==
0) {
> >>>>                         DRMLISTDEL(&nvbo->head);
> >>>>                         /*
> >>>> @@ -365,8 +365,6 @@ nouveau_bo_del(struct nouveau_bo *bo)
> >>>>                 }
> >>>>                 pthread_mutex_unlock(&nvdev->lock);
> >>>>         } else {
> >>>> -               DRMLISTDEL(&nvbo->head);
> >>>> -               pthread_mutex_unlock(&nvdev->lock);
> >>>>                 drmIoctl(bo->device->fd,
DRM_IOCTL_GEM_CLOSE, &req);
> >>>>         }
> >>>>         if (bo->map)
> >>>> @@ -379,7 +377,6 @@ nouveau_bo_new(struct nouveau_device
*dev,
> uint32_t
> >> flags, uint32_t align,
> >>>>                uint64_t size, union nouveau_bo_config
*config,
> >>>>                struct nouveau_bo **pbo)
> >>>>  {
> >>>> -       struct nouveau_device_priv *nvdev =
nouveau_device(dev);
> >>>>         struct nouveau_bo_priv *nvbo = calloc(1,
sizeof(*nvbo));
> >>>>         struct nouveau_bo *bo = &nvbo->base;
> >>>>         int ret;
> >>>> @@ -397,10 +394,6 @@ nouveau_bo_new(struct nouveau_device
*dev,
> >> uint32_t flags, uint32_t align,
> >>>>                 return ret;
> >>>>         }
> >>>>
> >>>> -       pthread_mutex_lock(&nvdev->lock);
> >>>> -       DRMLISTADD(&nvbo->head,
&nvdev->bo_list);
> >>>> -       pthread_mutex_unlock(&nvdev->lock);
> >>>> -
> >>>>         *pbo = bo;
> >>>>         return 0;
> >>>>  }
> >>>> @@ -457,6 +450,18 @@ nouveau_bo_wrap_locked(struct
nouveau_device
> *dev,
> >> uint32_t handle,
> >>>>         return -ENOMEM;
> >>>>  }
> >>>>
> >>>> +static void
> >>>> +nouveau_bo_make_global(struct nouveau_bo_priv *nvbo)
> >>>> +{
> >>>> +       if (!nvbo->head.next) {
> >>>> +               struct nouveau_device_priv *nvdev >
>> nouveau_device(nvbo->base.device);
> >>>> +               pthread_mutex_lock(&nvdev->lock);
> >>>> +               if (!nvbo->head.next)
> > I guess the bo_make_global call is not particularly sensitive, so
> >> removing's fine with me.
> >>
> > I would be worried about the duplicated check. It seems like a
"smart"
> > compiler could cache the value of "nvbo->head.next"
(unless marked as
> > volatile), rendering the second if() useless. If the field is marked
> > volatile, then of course, this does not apply.
> >
> In that case I would be worried about a compiler that ignores the acquire
> semantics of pthread_mutex_lock..
>
This isn't about the acquire semantics here, because you're reading it
on
both sides of the lock. The compiler need not reorder the reads at all,
merely just save the value of the first read. Unless the compiler knows
that the value can change after the lock is acquired, it can simply cache
the result. Consider the nearly identical transformation:

const int condition = (!nvbo->head.next);
if(condition){
   pthread_mutex_lock(...);
    if(condition){ //redundant, already can assert it is true
        ...

I just wouldn't risk it. The compiler probably makes this transformation
already, but even if it doesn't, it *can*, and that's a bad thing IMO.


Patrick
>
> ~Maarten
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20150225/d3cd53d0/attachment-0001.html>