Patrick Baggett
2015-Feb-25 15:04 UTC
[Nouveau] [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>
Maarten Lankhorst
2015-Feb-25 15:07 UTC
[Nouveau] [PATCH 2/2] nouveau: Do not add most bo's to the global bo list.
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
Patrick Baggett
2015-Feb-25 15:28 UTC
[Nouveau] [PATCH 2/2] nouveau: Do not add most bo's to the global bo list.
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>
Seemingly Similar Threads
- [PATCH 2/2] nouveau: Do not add most bo's to the global bo list.
- [PATCH 2/2] nouveau: Do not add most bo's to the global bo list.
- [PATCH 1/2] nouveau: make nouveau importing global buffers completely thread-safe, with tests
- [PATCH v2 1/4] Add atomic_inc_return to atomics.
- [PATCH] nouveau: safen up nouveau_device list usage against concurrent access