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

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

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?

Make a comment of such situation.

> +		/* 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?

The code itself is quite OK, but the comments mislead my understanding
of the code again.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx