Re: [Intel-gfx] [PATCH] drm: Inject a cond_resched() into long drm_clflush_sg()

From: David Laight <David.Laight@ACULAB.COM>
To: 'Chris Wilson' <chris@chris-wilson.co.uk>,
	Daniel Vetter <daniel@ffwll.ch>
Cc: "intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH] drm: Inject a cond_resched() into long drm_clflush_sg()
Date: Thu, 16 Jan 2020 12:26:45 +0000	[thread overview]
Message-ID: <8f6b9daa2af342a79137064203255242@AcuMS.aculab.com> (raw)
In-Reply-To: <157916041994.14122.8524532515240369595@skylake-alporthouse-com>

From: Chris Wilson <chris@chris-wilson.co.uk>
> Sent: 16 January 2020 07:40
> Quoting Daniel Vetter (2020-01-16 06:52:42)
> > On Wed, Jan 15, 2020 at 08:52:45PM +0000, Chris Wilson wrote:
> > > Since we may try and flush the cachelines associated with large buffers
> > > (an 8K framebuffer is about 128MiB, even before we try HDR), this leads
> > > to unacceptably long latencies (when using a voluntary CONFIG_PREEMPT).
> > > If we call cond_resched() between each sg chunk, that it about every 128
> > > pages, we have a natural break point in which to check if the process
> > > needs to be rescheduled. Naturally, this means that drm_clflush_sg() can
> > > only be called from process context -- which is true at the moment. The
> > > other clflush routines remain usable from atomic context.
> > >
> > > Even though flushing large objects takes a demonstrable amount to time
> > > to flush all the cachelines, clflush is still preferred over a
> > > system-wide wbinvd as the latter has unpredictable latencies affecting
> > > the whole system not just the local task.
> > >
> > > Reported-by: David Laight <David.Laight@ACULAB.COM>
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: David Laight <David.Laight@ACULAB.COM>
> >
> > The original bug report is complaining about latencies for SCHED_RT
> > threads, on a system that doesn't even use CONFIG_PREEMPT. I'm not sure
> > it's terribly valid to cater to that use-case - all the desktop distros
> > seem a lot more reasonable. So firmly *shrug* from my side ...
> 
> Yeah, I had the same immediate response to the complaint), but otoh we've
> inserted cond_resched() before when it looks like may consume entire
> jiffies inside a loop. At the very minimum, we should have a
> might_sleep() here and a reminder that this can be very slow (remember
> byt?).

I'm using RT to get more deterministic scheduling to look for long
scheduling delays, rather than because we need very tight scheduling.
Delays of several 100us aren't a real problem.

The problem with CONFIG_PREEMPT is that the distros don't
enable it and it isn't a command line option.
So it is really useless unless you are able/willing to build your
own kernel.

I could run the code under the normal scheduler with 'nice -19'.
I stlll wouldn't expect to have all but one cpu idle when I've just
done a cv_broadcast() to wake up a lot of threads.

I've added 'if (!(++i & 31)) cond_resched();' after the drm_clfulsh_page()
call in drm_cflush_sg().
In my case that it 3600/32 reschedules in 3.3ms - plenty.

However there is a call from __i915_gem_objet_set_pages() that
is preceded by a lockdep_assert_held() check - so mustn't sleep.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx