On 21/07/16 07:18, Goel, Akash wrote:
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*/);
}
We don't even know for sure if the thread (especially rt priority) is
absolutely required. Worker solution would be so much simpler if good
enough.
Because in the meantime it was discovered that relayfs has a silly
polling behaviour where it delays waking up readers by a jiffy and that
copies can be done much faster with SSE instructions. And that we are
increasing the GuC buffer anyway to improve space utilization.
So I say again, I think you need to evaluate all that and determine if a
complicated thread solution is really needed.
In parallel question is also if relayfs needs to be improved wrt this
latency issue, or if it can't, maybe it is the wrong facility for this
use case.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
