[Intel-gfx] [PATCH 1/2] intel: Defer setting madv on the bo cache

[Chris Wilson]

On Tue, Apr 14, 2015 at 12:38:56PM -0700, Ben Widawsky wrote:
> >  /** Frees all cached buffers significantly older than @time. */
> >  static void
> >  drm_intel_gem_cleanup_bo_cache(drm_intel_bufmgr_gem *bufmgr_gem, time_t time)
> > @@ -1162,19 +1182,29 @@ drm_intel_gem_cleanup_bo_cache(drm_intel_bufmgr_gem *bufmgr_gem, time_t time)
> >  	for (i = 0; i < bufmgr_gem->num_buckets; i++) {
> >  		struct drm_intel_gem_bo_bucket *bucket =
> >  		    &bufmgr_gem->cache_bucket[i];
> > +		drmMMListHead madv;
> >  
> > +		DRMINITLISTHEAD(&madv);
> >  		while (!DRMLISTEMPTY(&bucket->head)) {
> >  			drm_intel_bo_gem *bo_gem;
> >  
> >  			bo_gem = DRMLISTENTRY(drm_intel_bo_gem,
> >  					      bucket->head.next, head);
> > -			if (time - bo_gem->free_time <= 1)
> > +			if (time - bo_gem->free_time < 2*(1+bo_gem->dontneed))
> 
> Something somewhere will complain about mixing bools and ints, I'd guess.
> 
> Also perhaps for perf bisection maybe do a patch before this changing the
> expiration time to 2 (or 4), then add the extra dontneed state?

It is already 2, since it uses <= and the patch just converts it to <
to be consistent after the multiplication.
 
> >  				break;
> >  
> >  			DRMLISTDEL(&bo_gem->head);
> > -
> > -			drm_intel_gem_bo_free(&bo_gem->bo);
> > +			if (bo_gem->dontneed) {
> > +				drm_intel_gem_bo_free(&bo_gem->bo);
> > +			} else {
> > +				drm_intel_gem_bo_madvise_internal(bufmgr_gem, bo_gem,
> > +								  I915_MADV_DONTNEED);
> > +				bo_gem->dontneed = 1;
> > +				DRMLISTADDTAIL(&bo_gem->head, &madv);
> > +			}
> >  		}
> 
> Maybe add a comment here?
> /* Objects which have elapsed 2s of disuse should go to the top of the bucket. */
> 
> > +		if (!DRMLISTEMPTY(&madv))
> > +			__splice(&madv, &bucket->head, bucket->head.next);
> 
> 
> >  	}
> >  
> >  	bufmgr_gem->time = time;
> > @@ -1285,9 +1315,7 @@ drm_intel_gem_bo_unreference_final(drm_intel_bo *bo, time_t time)
> >  
> >  	bucket = drm_intel_gem_bo_bucket_for_size(bufmgr_gem, bo->size);
> >  	/* Put the buffer into our internal cache for reuse if we can. */
> > -	if (bufmgr_gem->bo_reuse && bo_gem->reusable && bucket != NULL &&
> > -	    drm_intel_gem_bo_madvise_internal(bufmgr_gem, bo_gem,
> > -					      I915_MADV_DONTNEED)) {
> > +	if (bufmgr_gem->bo_reuse && bo_gem->reusable && bucket != NULL) {
> >  		bo_gem->free_time = time;
> >  
> >  		bo_gem->name = NULL;
> 
> In general, it looks fine to me. Like I said above wrt adding the patch before
> this, I am curious how much difference you see just messing with the expiration
> times versus the two state model.

The issue I was looking at was simply struct_mutex contention in madvise
(then busy). For busy we can tweak the ordering to reduce the contention
slightly, but madvise is trickier (though madvise is a candidate for one
of the first per-object locks). This just moves the contention elsewhere,
but madvise/busy calls tend to dominate overall and trimming them reduces
the intersection.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre