[Mesa-dev] [Mesa-stable] [PATCH 1/3] anv: Stop racing relocation offsets
Andres Gomez
agomez at igalia.com
Mon Jun 26 19:39:57 UTC 2017
Jason, it doesn't seem like this patch has landed in master. Are you in
need of review or is it that this has been superseded?
Thanks!
On Wed, 2017-05-10 at 16:08 -0700, Jason Ekstrand wrote:
> One of the key invariants of the relocation system is the
> presumed_offset field. The assumption is made that the value currently
> in the address to be relocated agrees with the presumed_offset field.
> If presumed_offset is equal to the offset of the BO, the kernel will
> skip the relocation assuming that the value is already correct.
>
> Our initial implementation of relocation handling had a race where we
> would read bo->offset once when we wrote the relocation entry and again
> when we filled out actual address.
>
> Found with helgrind
>
> Cc: "17.0 17.1" <mesa-stable at lists.freedesktop.org>
> ---
> src/intel/vulkan/anv_batch_chain.c | 21 +++++++++++++++++----
> src/intel/vulkan/anv_private.h | 2 +-
> src/intel/vulkan/genX_blorp_exec.c | 5 ++++-
> src/intel/vulkan/genX_cmd_buffer.c | 7 +++++--
> 4 files changed, 27 insertions(+), 8 deletions(-)
>
> diff --git a/src/intel/vulkan/anv_batch_chain.c b/src/intel/vulkan/anv_batch_chain.c
> index 9def174..13303b1 100644
> --- a/src/intel/vulkan/anv_batch_chain.c
> +++ b/src/intel/vulkan/anv_batch_chain.c
> @@ -143,7 +143,8 @@ anv_reloc_list_grow(struct anv_reloc_list *list,
> VkResult
> anv_reloc_list_add(struct anv_reloc_list *list,
> const VkAllocationCallbacks *alloc,
> - uint32_t offset, struct anv_bo *target_bo, uint32_t delta)
> + uint32_t offset, struct anv_bo *target_bo, uint32_t delta,
> + uint64_t *bo_offset_out)
> {
> struct drm_i915_gem_relocation_entry *entry;
> int index;
> @@ -155,6 +156,14 @@ anv_reloc_list_add(struct anv_reloc_list *list,
> if (result != VK_SUCCESS)
> return result;
>
> + /* Read the BO offset once. This same value will be used in the relocation
> + * entry and passed back to the caller for it to use when it writes the
> + * actual value. This guarantees that the two values match even if there
> + * is a data race between now and when the caller gets around to writing
> + * the address into the BO.
> + */
> + uint64_t presumed_offset = target_bo->offset;
> +
> /* XXX: Can we use I915_EXEC_HANDLE_LUT? */
> index = list->num_relocs++;
> list->reloc_bos[index] = target_bo;
> @@ -162,11 +171,13 @@ anv_reloc_list_add(struct anv_reloc_list *list,
> entry->target_handle = target_bo->gem_handle;
> entry->delta = delta;
> entry->offset = offset;
> - entry->presumed_offset = target_bo->offset;
> + entry->presumed_offset = presumed_offset;
> entry->read_domains = domain;
> entry->write_domain = domain;
> VG(VALGRIND_CHECK_MEM_IS_DEFINED(entry, sizeof(*entry)));
>
> + *bo_offset_out = presumed_offset;
> +
> return VK_SUCCESS;
> }
>
> @@ -218,14 +229,16 @@ uint64_t
> anv_batch_emit_reloc(struct anv_batch *batch,
> void *location, struct anv_bo *bo, uint32_t delta)
> {
> + uint64_t bo_offset;
> VkResult result = anv_reloc_list_add(batch->relocs, batch->alloc,
> - location - batch->start, bo, delta);
> + location - batch->start, bo, delta,
> + &bo_offset);
> if (result != VK_SUCCESS) {
> anv_batch_set_error(batch, result);
> return 0;
> }
>
> - return bo->offset + delta;
> + return bo_offset + delta;
> }
>
> void
> diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
> index 9b0dd67..1686da8 100644
> --- a/src/intel/vulkan/anv_private.h
> +++ b/src/intel/vulkan/anv_private.h
> @@ -825,7 +825,7 @@ void anv_reloc_list_finish(struct anv_reloc_list *list,
> VkResult anv_reloc_list_add(struct anv_reloc_list *list,
> const VkAllocationCallbacks *alloc,
> uint32_t offset, struct anv_bo *target_bo,
> - uint32_t delta);
> + uint32_t delta, uint64_t *bo_offset_out);
>
> struct anv_batch_bo {
> /* Link in the anv_cmd_buffer.owned_batch_bos list */
> diff --git a/src/intel/vulkan/genX_blorp_exec.c b/src/intel/vulkan/genX_blorp_exec.c
> index 71ed707..513c269 100644
> --- a/src/intel/vulkan/genX_blorp_exec.c
> +++ b/src/intel/vulkan/genX_blorp_exec.c
> @@ -57,9 +57,12 @@ blorp_surface_reloc(struct blorp_batch *batch, uint32_t ss_offset,
> struct blorp_address address, uint32_t delta)
> {
> struct anv_cmd_buffer *cmd_buffer = batch->driver_batch;
> + MAYBE_UNUSED uint64_t bo_offset;
> +
> VkResult result =
> anv_reloc_list_add(&cmd_buffer->surface_relocs, &cmd_buffer->pool->alloc,
> - ss_offset, address.buffer, address.offset + delta);
> + ss_offset, address.buffer, address.offset + delta,
> + &bo_offset);
> if (result != VK_SUCCESS)
> anv_batch_set_error(&cmd_buffer->batch, result);
> }
> diff --git a/src/intel/vulkan/genX_cmd_buffer.c b/src/intel/vulkan/genX_cmd_buffer.c
> index ef9b7d0..4276f59 100644
> --- a/src/intel/vulkan/genX_cmd_buffer.c
> +++ b/src/intel/vulkan/genX_cmd_buffer.c
> @@ -170,10 +170,12 @@ add_surface_state_reloc(struct anv_cmd_buffer *cmd_buffer,
> struct anv_bo *bo, uint32_t offset)
> {
> const struct isl_device *isl_dev = &cmd_buffer->device->isl_dev;
> + MAYBE_UNUSED uint64_t bo_offset;
>
> VkResult result =
> anv_reloc_list_add(&cmd_buffer->surface_relocs, &cmd_buffer->pool->alloc,
> - state.offset + isl_dev->ss.addr_offset, bo, offset);
> + state.offset + isl_dev->ss.addr_offset, bo, offset,
> + &bo_offset);
> if (result != VK_SUCCESS)
> anv_batch_set_error(&cmd_buffer->batch, result);
> }
> @@ -199,11 +201,12 @@ add_image_view_relocs(struct anv_cmd_buffer *cmd_buffer,
> uint32_t *aux_addr_dw = state.map + isl_dev->ss.aux_addr_offset;
> aux_offset += *aux_addr_dw & 0xfff;
>
> + MAYBE_UNUSED uint64_t bo_offset;
> VkResult result =
> anv_reloc_list_add(&cmd_buffer->surface_relocs,
> &cmd_buffer->pool->alloc,
> state.offset + isl_dev->ss.aux_addr_offset,
> - iview->bo, aux_offset);
> + iview->bo, aux_offset, &bo_offset);
> if (result != VK_SUCCESS)
> anv_batch_set_error(&cmd_buffer->batch, result);
> }
--
Br,
Andres
More information about the mesa-dev
mailing list