[PATCH v3 1/6] drm/i915: Fix request locking during error capture & debugfs dump

[Andy Shevchenko]

On Fri, Jan 20, 2023 at 03:06:02PM -0800, John Harrison wrote:
> On 1/19/2023 07:16, Andy Shevchenko wrote:
> > On Wed, Jan 18, 2023 at 10:49:55PM -0800, John.C.Harrison at Intel.com wrote:

...

> > > +		found = false;
> > > +		spin_lock(&ce->guc_state.lock);
> > >   		list_for_each_entry(rq, &ce->guc_state.requests, sched.link) {
> > >   			if (i915_test_request_state(rq) != I915_REQUEST_ACTIVE)
> > >   				continue;
> > > +			found = true;
> > > +			break;
> > > +		}
> > This can be combined to (see also below)
> > 
> > 		list_for_each_entry(rq, &ce->guc_state.requests, sched.link) {
> > 			if (i915_test_request_state(rq) == I915_REQUEST_ACTIVE)
> > 				break;
> > 		}
> > 
> > > +		spin_unlock(&ce->guc_state.lock);
> > Instead of 'found' you can check the current entry pointer
> > 
> > 		if (!list_entry_is_head(...))
> > 
> > And because requests can only be messed up with the guc_state itself, I think
> > you don't need to perform the above check under spinlock, so it's safe.
> I'm not following the argument as to why it is safe to test a guc_state
> owned list outside of holding the guc_state spinlock.

The very same reasons why found is not checked inside the lock.
If something bad to the list head pointer happens, it would mean
that we have much bigger issues. And list_entry_is_head() is specifically
to test the loop exit condition.

> I also think that having an explicit 'found' flag makes the code more
> readable and immediately obvious as to what is going on.

It depends on the perception. With boolean I have to go somewhere to be sure
that found has false when loop is fully revolved. (Sometimes it may be the
inverted loops like

    found = true;
    for (...loop...) {
        if (...cond...) {
	   found = false;
	   break;
        }
    }

while with the helper it's obvious)

> For the sake of one
> bool (which the compiler would optimise out anyway),

Is it really optimized away?

> I don't think it is worth the obfuscation of behaviour and the risk of "I
> think this will work".

Whatever, not big deal :)

> > > +		if (found) {
> > >   			intel_engine_set_hung_context(engine, ce);

-- 
With Best Regards,
Andy Shevchenko