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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
