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

[Maarten Lankhorst]

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