Re: [PATCH 11/16] drm/i915: Remove (struct_mutex) locking for busy-ioctl

Chris Wilson
2016-08-04T10:31:07Z

On Thu, Aug 04, 2016 at 01:25:08PM +0300, Joonas Lahtinen wrote:
> On ma, 2016-08-01 at 19:22 +0100, Chris Wilson wrote:
> > +static __always_inline unsigned
> > +__busy_read_flag(const struct drm_i915_gem_request *request)
> > +{
> > + return 0x10000 << request->engine->exec_id;
>
> Duh, this really is our ABI? No BIT(NUM_ENGINES) or something sane?

BIT(NUM_ENGINES) as uabi, you call that sane :)

> Make a comment of such situation.

Like BUILD_BUG_ON(NUM_ENGINES > 16).

> > + /* Yes, the lookups are intentionally racy.
> > +  *
> > +  * Even though we guard the pointer lookup by RCU, that only
> > +  * guarantees that the pointer and its contents remain
> > +  * dereferencable and does *not* mean that the request we
> > +  * have is the same as the one being tracked by the object.
> > +  *
> > +  * Consider that we lookup the request just as it is being
> > +  * retired and free. We take a local copy of the pointer,
>
> s/free/freed/
>
> > +  * but before we add its engine into the busy set, the other
> > +  * thread reallocates it and assigns it to a task on another
> > +  * engine with a fresh and incomplete seqno.
> > +  *
> > +  * So after we lookup the engine's id, we double check that
>
> This double check is nowhere to be seen, time to update this comment
> too?

Actualy snuck it back in. I took it out thinking that there wasn't
sufficent read-dependency to guarrantee the ordering (but now reversed
my decision there) and had re-read my comment about why the check is
required.
-Chris

--
Chris Wilson, Intel Open Source Technology Centre