[Intel-gfx] [PATCH 11/39] drm/i915: Allow userspace to clone contexts on creation

[Tvrtko Ursulin]

On 14/03/2019 16:54, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-03-14 16:18:17)
>>
>> On 13/03/2019 14:43, Chris Wilson wrote:
>>> +     if (local.flags & I915_CONTEXT_CLONE_VM) {
>>> +             struct i915_hw_ppgtt *ppgtt;
>>> +
>>> +             do {
>>> +                     ppgtt = READ_ONCE(src->ppgtt);
>>> +                     if (!ppgtt)
>>> +                             break;
>>> +
>>> +                     if (!kref_get_unless_zero(&ppgtt->ref))
>>> +                             continue;
>>> +
>>> +                     if (ppgtt == READ_ONCE(src->ppgtt))
>>> +                             break;
>>
>> This loop needs a comment. For instance I don't understand what is this
>> line for. Firstly, we have a reference on ppgtt, so what do we care if
>> the source context in the meantime changed it? Secondly, even though it
>> matches now, why would it still match when we exit the loop?
> 
> Because it may have been reallocated between the read and the kref. Once
> we have the kref, and passed through the strong memory barrier, can we
> decide if this is the ppgtt that we wanted. It may indeed be released
> from src->ppgtt after this point, but it can no longer be reused
> elsewhere, so we can not inadvertently share the ppgtt from a third
> party.

I don't get it. As soon as we have obtained a reference we could proceed 
I think. It was a ppgtt which was set in the source context at one 
point. It may not be by the time we decide to replace ours with it, but 
apart from doing all under struct_mutex (to match set_ppgtt) I don't see 
what another check after kref brings.

Regards,

Tvrtko