[Mesa-dev] [PATCH 3/4] i965/drm: Searching for a cached buffer for reuse

[Chris Wilson]

Quoting James Xiong (2018-05-11 16:35:04)
> On Thu, 10 May 2018 13:56:12 -0700
> Kenneth Graunke <kenneth at whitecape.org> wrote:
> 
> > On Friday, May 4, 2018 5:56:04 PM PDT James Xiong wrote:
> > > From: "Xiong, James" <james.xiong at intel.com>
> > > 
> > > Now that a bucket contains cached buffers with different sizes, go
> > > through its list and search for a cached buffer with enough size.
> > > 
> > > Signed-off-by: Xiong, James <james.xiong at intel.com>
> > > ---
> > >  src/mesa/drivers/dri/i965/brw_bufmgr.c | 21 +++++++++++++++------
> > >  src/util/list.h                        |  5 +++++
> > >  2 files changed, 20 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/src/mesa/drivers/dri/i965/brw_bufmgr.c
> > > b/src/mesa/drivers/dri/i965/brw_bufmgr.c index 6a9b005..5235aa6
> > > 100644 --- a/src/mesa/drivers/dri/i965/brw_bufmgr.c
> > > +++ b/src/mesa/drivers/dri/i965/brw_bufmgr.c
> > > @@ -281,7 +281,7 @@ cached_bo_for_size(struct brw_bufmgr *bufmgr,
> > >     assert(!(busy && zeroed));
> > >  
> > >     if(bucket != NULL && !list_empty(&bucket->head)) {
> > > -      struct brw_bo *bo;
> > > +      struct brw_bo *bo, *temp_bo;
> > >  retry:
> > >        bo = NULL;
> > >  
> > > @@ -292,8 +292,13 @@ retry:
> > >            * asked us to zero the buffer, we don't want this
> > >            * because we are going to mmap it.
> > >            */
> > > -         bo = LIST_ENTRY(struct brw_bo, bucket->head.prev, head);
> > > -         list_del(&bo->head);
> > > +         LIST_FOR_EACH_ENTRY_REV(temp_bo, &bucket->head, head) {
> > > +            if (temp_bo->size >= size) {
> > > +               bo = temp_bo;
> > > +               list_del(&bo->head);
> > > +               break;
> > > +            }
> > > +         }
> > >        } else {
> > >           /* For non-render-target BOs (where we're probably
> > >            * going to map it first thing in order to fill it
> > > @@ -302,9 +307,13 @@ retry:
> > >            * allocating a new buffer is probably faster than
> > >            * waiting for the GPU to finish.
> > >            */
> > > -         bo = LIST_ENTRY(struct brw_bo, bucket->head.next, head);
> > > -         if (!brw_bo_busy(bo)) {
> > > -            list_del(&bo->head);
> > > +         LIST_FOR_EACH_ENTRY(temp_bo, &bucket->head, head) {
> > > +            if (temp_bo->size >= size &&
> > > +                !brw_bo_busy(temp_bo)) {
> > > +               bo = temp_bo;
> > > +               list_del(&bo->head);
> > > +               break;
> > > +            }
> > >           }
> > >        }
> > >  
> > > diff --git a/src/util/list.h b/src/util/list.h
> > > index 6edb750..9362072 100644
> > > --- a/src/util/list.h
> > > +++ b/src/util/list.h
> > > @@ -189,6 +189,11 @@ static inline void list_validate(struct
> > > list_head *list) &pos->member !=
> > > (head);                                             \ pos =
> > > container_of(pos->member.next, pos, member)) 
> > > +#define LIST_FOR_EACH_ENTRY_REV(pos, head,
> > > member)                      \
> > > +   for (pos = NULL, pos = container_of((head)->prev, pos,
> > > member);      \
> > > +        &pos->member !=
> > > (head);                                         \
> > > +        pos = container_of(pos->member.prev, pos, member))
> > > +
> > >  #define LIST_FOR_EACH_ENTRY_SAFE(pos, storage, head,
> > > member)     \ for (pos = NULL, pos = container_of((head)->next,
> > > pos, member),       \ storage = container_of(pos->member.next,
> > > pos, member);       \ 
> > 
> > Hi James,
> > 
> > I don't particularly like the idea of introducing linear linked list
> > walks to find a buffer.  It's a really nice property to have every
> > buffer in the bucket be suitable and be able to grab one immediately.
> > It sounds like purging things more often in patch 4 reduces the cost
> > of this new list walk, but...it'd be nice to not add it in the first
> > place.
> Yes, the purge, as well as the cleanup routine which kicks in from time

It should not be time to time, it should be whenever the cache is older
than a 1s (use a timer thread if you are concerned) as it being checked
everytime something is put into the cache (and so before the lists grow).
If it is overallocating such that you are running out of memory and not
reusing buffers, the cache is barely functioning. Now that's the
information you want to present, where/when/why/how the cache is not
working. What is being put into the cache that is never reused?
-Chris