On 7/21/2016 11:13 AM, Chris Wilson wrote:
On Thu, Jul 21, 2016 at 09:11:42AM +0530, Goel, Akash wrote:
On 7/21/2016 1:04 AM, Chris Wilson wrote:
In the end, just the silly locking and placement of complete_all() is
dangerous. reinit_completion() lacks the barrier to be used like this
really, at any rate, racy with the irq handler, so use sparingly or when
you control the irq handler.
Sorry I forgot to add a comment that
guc_cancel_log_flush_work_sync() should be invoked only after
ensuring that there will be no more flush interrupts, which will
happen either by explicitly disabling the interrupt or disabling the
logging and that's what is done at the 2 call sites.
Since had covered reinit_completion() under the irq_lock, thought an
explicit barrier is not needed.
You hadn't controlled everything via the irq_lock, and nor should you.
spin_lock_irq(&dev_priv->irq_lock);
if (guc->log.flush_signal) {
guc->log.flush_signal = false;
reinit_completion(&guc->log.flush_completion);
spin_unlock_irq(&dev_priv->irq_lock);
i915_guc_capture_logs(&dev_priv->drm);
complete_all(&guc->log.flush_completion);
The placement of complete_all isn't right for the case, where
a guc_cancel_log_flush_work_sync() is called but there was no prior
flush interrupt received.
Exactly.
(Also not sure if log.signal = 0 is sane,
Did log.signal = 0 for fast cancellation. Will remove that.
A smp_wmb() after reinit_completion(&flush_completion) would be fine ?
Don't worry, the race can only be controlled by controlling the irq.
In the end, I think something more like
while (signal) ...
complete_all();
schedule();
reinit_completion();
is the simplest.
Thanks much, so will have the task body like this.
do {
set_current_state(TASK_INT);
while (cmpxchg(&log.signal, 1, 0)) {
i915_guc_capture_logs();
};
complete_all(log.complete);
if (kthread_should_stop())
break;
schedule();
reinit_completion();
} while(1);
or the current callsites really require the flush.)
Sync against a ongoing/pending flush is being done for the 2
forceful flush cases, which will be effective only if the pending
flush is completed, so forceful flush should be serialized with a
pending flush.
Or you just signal=true, wakeup task, wait_timeout. Otherwise you
haven't really serialized anything without disabling the interrupt.
Agree without disabling the interrupt, serialization cannot be provided,
For the sync can use,
{
WARN_ON(guc->interrupts_enabled);
wait_for_completion_interruptible_timeout(
guc->log.complete, 5 /* in jiffies*/);
}
Best regards
Akash
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
