| Message ID | 1349445188-16253-2-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/5/2012 6:53 AM, Chris Wilson wrote: > By using round_jiffies() we can align the wakeup of our worker to the > nearest second in order to batch wakeups and reduce system load, which > is useful for unimportant coarse tasks like our retire_requests. > > Suggested-by: Arjan van de Ven <arjan@linux.intel.com> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Arjan van de Ven <arjan@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 8e05d53..706f481 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2084,6 +2084,11 @@ i915_gem_next_request_seqno(struct intel_ring_buffer *ring) > return ring->outstanding_lazy_request; > } > > +static unsigned long round_jiffies_delay(unsigned long delay) > +{ > + return round_jiffies_relative(delay) - jiffies; > +} this is buggy > + > int > i915_add_request(struct intel_ring_buffer *ring, > struct drm_file *file, > @@ -2155,7 +2160,8 @@ i915_add_request(struct intel_ring_buffer *ring, > } > if (was_empty) { > queue_delayed_work(dev_priv->wq, > - &dev_priv->mm.retire_work, HZ); > + &dev_priv->mm.retire_work, > + round_jiffies_delay(HZ)); when used like this round_jiffies() rounds absolute jiffies towards the next second round_jiffies_relative() already subtracts jiffies from the result, like the helper that you're trying to invent here does ;=) doing that double up is a bad idea.
On Fri, 05 Oct 2012 08:18:17 -0700, Arjan van de Ven <arjan@linux.intel.com> wrote: > On 10/5/2012 6:53 AM, Chris Wilson wrote: > > By using round_jiffies() we can align the wakeup of our worker to the > > nearest second in order to batch wakeups and reduce system load, which > > is useful for unimportant coarse tasks like our retire_requests. > > > > Suggested-by: Arjan van de Ven <arjan@linux.intel.com> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Arjan van de Ven <arjan@linux.intel.com> > > --- > > drivers/gpu/drm/i915/i915_gem.c | 14 +++++++++++--- > > 1 file changed, 11 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index 8e05d53..706f481 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -2084,6 +2084,11 @@ i915_gem_next_request_seqno(struct intel_ring_buffer *ring) > > return ring->outstanding_lazy_request; > > } > > > > +static unsigned long round_jiffies_delay(unsigned long delay) > > +{ > > + return round_jiffies_relative(delay) - jiffies; > > +} > > this is buggy > > > > + > > int > > i915_add_request(struct intel_ring_buffer *ring, > > struct drm_file *file, > > @@ -2155,7 +2160,8 @@ i915_add_request(struct intel_ring_buffer *ring, > > } > > if (was_empty) { > > queue_delayed_work(dev_priv->wq, > > - &dev_priv->mm.retire_work, HZ); > > + &dev_priv->mm.retire_work, > > + round_jiffies_delay(HZ)); > > when used like this > > > round_jiffies() rounds absolute jiffies towards the next second > > round_jiffies_relative() already subtracts jiffies from the result, like > the helper that you're trying to invent here does ;=) > > doing that double up is a bad idea. For some reason the example I read convinced me that round_jiffies_relative() returned the absolute jiffie for the relative delay so that we could put it straight into mod_timer(). Again we can use round up here as well. -Chris
On Fri, 05 Oct 2012, Chris Wilson <chris@chris-wilson.co.uk> wrote: > By using round_jiffies() we can align the wakeup of our worker to the > nearest second in order to batch wakeups and reduce system load, which > is useful for unimportant coarse tasks like our retire_requests. Is there a reason not to just use INIT_DELAYED_WORK_DEFERRABLE()? Come to think of it, same with deferrable timer in patch 1/2. > Suggested-by: Arjan van de Ven <arjan@linux.intel.com> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Arjan van de Ven <arjan@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 8e05d53..706f481 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2084,6 +2084,11 @@ i915_gem_next_request_seqno(struct intel_ring_buffer *ring) > return ring->outstanding_lazy_request; > } > > +static unsigned long round_jiffies_delay(unsigned long delay) > +{ > + return round_jiffies_relative(delay) - jiffies; > +} Hmm, is it possible that would end up negative if someone reuses that with a small delay? An observation: there's a bunch of calls elsewhere in kernel to queue_delayed_work() with the delay wrapped in round_jiffies() or round_jiffies_relative(). The former at least gets queued within expected tolerance (though likely not on full second), but how could the code using the latter ever work?! I guess a function like yours could be useful in generic code. BR, Jani. > + > int > i915_add_request(struct intel_ring_buffer *ring, > struct drm_file *file, > @@ -2155,7 +2160,8 @@ i915_add_request(struct intel_ring_buffer *ring, > } > if (was_empty) { > queue_delayed_work(dev_priv->wq, > - &dev_priv->mm.retire_work, HZ); > + &dev_priv->mm.retire_work, > + round_jiffies_delay(HZ)); > intel_mark_busy(dev_priv->dev); > } > } > @@ -2346,7 +2352,8 @@ i915_gem_retire_work_handler(struct work_struct *work) > > /* Come back later if the device is busy... */ > if (!mutex_trylock(&dev->struct_mutex)) { > - queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work, HZ); > + queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work, > + round_jiffies_delay(HZ)); > return; > } > > @@ -2364,7 +2371,8 @@ i915_gem_retire_work_handler(struct work_struct *work) > } > > if (!dev_priv->mm.suspended && !idle) > - queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work, HZ); > + queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work, > + round_jiffies_delay(HZ)); > if (idle) > intel_mark_idle(dev); > > -- > 1.7.10.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 8e05d53..706f481 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2084,6 +2084,11 @@ i915_gem_next_request_seqno(struct intel_ring_buffer *ring) return ring->outstanding_lazy_request; } +static unsigned long round_jiffies_delay(unsigned long delay) +{ + return round_jiffies_relative(delay) - jiffies; +} + int i915_add_request(struct intel_ring_buffer *ring, struct drm_file *file, @@ -2155,7 +2160,8 @@ i915_add_request(struct intel_ring_buffer *ring, } if (was_empty) { queue_delayed_work(dev_priv->wq, - &dev_priv->mm.retire_work, HZ); + &dev_priv->mm.retire_work, + round_jiffies_delay(HZ)); intel_mark_busy(dev_priv->dev); } } @@ -2346,7 +2352,8 @@ i915_gem_retire_work_handler(struct work_struct *work) /* Come back later if the device is busy... */ if (!mutex_trylock(&dev->struct_mutex)) { - queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work, HZ); + queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work, + round_jiffies_delay(HZ)); return; } @@ -2364,7 +2371,8 @@ i915_gem_retire_work_handler(struct work_struct *work) } if (!dev_priv->mm.suspended && !idle) - queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work, HZ); + queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work, + round_jiffies_delay(HZ)); if (idle) intel_mark_idle(dev);
By using round_jiffies() we can align the wakeup of our worker to the nearest second in order to batch wakeups and reduce system load, which is useful for unimportant coarse tasks like our retire_requests. Suggested-by: Arjan van de Ven <arjan@linux.intel.com> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Arjan van de Ven <arjan@linux.intel.com> --- drivers/gpu/drm/i915/i915_gem.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)