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

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)
Something looks a bit strange here. The list is modified under a lock,
so there should be no need for the second if above. Did you have
something else in mind ?

-Emil

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)
> Something looks a bit strange here. The list is modified under a lock,
> so there should be no need for the second if above. Did you have
> something else in mind ?
The unlocked if check could be removed, but checking is cheaper than locking. :P
I guess the bo_make_global call is not particularly sensitive, so removing's
fine with me.

~Maarten

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>