<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, May 11, 2017 at 12:41 AM, Chris Wilson <span dir="ltr"><<a href="mailto:chris@chris-wilson.co.uk" target="_blank">chris@chris-wilson.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Wed, May 10, 2017 at 04:08:53PM -0700, Jason Ekstrand wrote:<br>
> One of the key invariants of the relocation system is the<br>
> presumed_offset field.  The assumption is made that the value currently<br>
> in the address to be relocated agrees with the presumed_offset field.<br>
> If presumed_offset is equal to the offset of the BO, the kernel will<br>
> skip the relocation assuming that the value is already correct.<br>
><br>
> Our initial implementation of relocation handling had a race where we<br>
> would read bo->offset once when we wrote the relocation entry and again<br>
> when we filled out actual address.<br>
><br>
> Found with helgrind<br>
><br>
> Cc: "17.0 17.1" <<a href="mailto:mesa-stable@lists.freedesktop.org">mesa-stable@lists.<wbr>freedesktop.org</a>><br>
> ---<br>
>  src/intel/vulkan/anv_batch_<wbr>chain.c | 21 +++++++++++++++++----<br>
>  src/intel/vulkan/anv_private.h     |  2 +-<br>
>  src/intel/vulkan/genX_blorp_<wbr>exec.c |  5 ++++-<br>
>  src/intel/vulkan/genX_cmd_<wbr>buffer.c |  7 +++++--<br>
>  4 files changed, 27 insertions(+), 8 deletions(-)<br>
><br>
> diff --git a/src/intel/vulkan/anv_batch_<wbr>chain.c b/src/intel/vulkan/anv_batch_<wbr>chain.c<br>
> index 9def174..13303b1 100644<br>
> --- a/src/intel/vulkan/anv_batch_<wbr>chain.c<br>
> +++ b/src/intel/vulkan/anv_batch_<wbr>chain.c<br>
> @@ -143,7 +143,8 @@ anv_reloc_list_grow(struct anv_reloc_list *list,<br>
>  VkResult<br>
>  anv_reloc_list_add(struct anv_reloc_list *list,<br>
>                     const VkAllocationCallbacks *alloc,<br>
> -                   uint32_t offset, struct anv_bo *target_bo, uint32_t delta)<br>
> +                   uint32_t offset, struct anv_bo *target_bo, uint32_t delta,<br>
> +                   uint64_t *bo_offset_out)<br>
>  {<br>
>     struct drm_i915_gem_relocation_entry *entry;<br>
>     int index;<br>
> @@ -155,6 +156,14 @@ anv_reloc_list_add(struct anv_reloc_list *list,<br>
>     if (result != VK_SUCCESS)<br>
>        return result;<br>
><br>
> +   /* Read the BO offset once.  This same value will be used in the relocation<br>
> +    * entry and passed back to the caller for it to use when it writes the<br>
> +    * actual value.  This guarantees that the two values match even if there<br>
> +    * is a data race between now and when the caller gets around to writing<br>
> +    * the address into the BO.<br>
> +    */<br>
> +   uint64_t presumed_offset = target_bo->offset;<br>
<br>
</div></div>If it really can be written in parallel and if helgrind says it is, it<br>
is, then you also need a compiler barrier to prevent a reload.<br>
<br>
#define READ_ONCE(x) (*(volatile typeof(x) *)&(x))<br>
uint64_t presumed_offset = READ_ONCE(target_bo->offset);<span class="HOEnZb"><font color="#888888"><br></font></span></blockquote><div><br></div><div>I thought about that but wasn't sure if it was actually an issue.  Happy to add it.<br></div></div></div></div>