RE: [PATCH 2/4] Drivers: hv: vmbus: Annotate accesses to the shared ring buffer indices

[Michael Kelley]

From: Kameron Carr <kameroncarr@xxxxxxxxxxxxxxxxxxx> Sent: Thursday, October 1, 2026 3:11 PM
> 

I have minor quibble with the patch "Subject". To me, "Annotate"
refers to adding metadata to the code, usually for use by some analysis
tool. READ/WRITE_ONCE() aren't annotations in that sense in that they
may affect the generated code and may be needed for correctness of
the code. As an alternative Subject, how about:

    Drivers: hv: vmbus: Use *_ONCE() to access shared ring buffer indices

But maybe my interpretation of "Annotate" is skewed ....

> The ring buffer read/write indices live in a page shared with the host,
> so the compiler must not split, merge or refetch accesses to them. Add
> READ_ONCE()/WRITE_ONCE() in hv_set_next_write_location(),
> hv_pkt_iter_close(), hv_get_bytes_to_read() and hv_get_bytes_to_write().
> The accesses in hv_ringbuffer_get_debuginfo() are left to the next
> patch.
> 
> Drop hv_get_ring_bufferindices(). It has one caller and is a one-line
> expression on the write index. Inlining it moves the access to the call
> site, so hv_ringbuffer_write() can reuse its validated snapshot of that
> index, old_write, rather than reading the shared memory a second time.
> 
> No functional change intended for a well-behaved host.
> 
> Signed-off-by: Kameron Carr <kameroncarr@xxxxxxxxxxxxxxxxxxx>

My quibble with the Subject notwithstanding,

Reviewed-by: Michael Kelley <mhklinux@xxxxxxxxxxx>

> ---
>  drivers/hv/ring_buffer.c | 15 ++++-----------
>  include/linux/hyperv.h   |  4 ++--
>  2 files changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
> index a18b309..7f466c5 100644
> --- a/drivers/hv/ring_buffer.c
> +++ b/drivers/hv/ring_buffer.c
> @@ -75,7 +75,7 @@ static inline void
>  hv_set_next_write_location(struct hv_ring_buffer_info *ring_info,
>  		     u32 next_write_location)
>  {
> -	ring_info->ring_buffer->write_index = next_write_location;
> +	WRITE_ONCE(ring_info->ring_buffer->write_index, next_write_location);
>  }
> 
>  /* Get the size of the ring buffer. */
> @@ -85,13 +85,6 @@ hv_get_ring_buffersize(const struct hv_ring_buffer_info *ring_info)
>  	return ring_info->ring_datasize;
>  }
> 
> -/* Get the read and write indices as u64 of the specified ring buffer. */
> -static inline u64
> -hv_get_ring_bufferindices(struct hv_ring_buffer_info *ring_info)
> -{
> -	return (u64)ring_info->ring_buffer->write_index << 32;
> -}
> -
>  /*
>   * Helper routine to copy from source to ring buffer.
>   * Assume there is enough room. Handles wrap-around in dest case only!!
> @@ -358,7 +351,7 @@ int hv_ringbuffer_write(struct vmbus_channel *channel,
>  		*trans_id = __trans_id;
> 
>  	/* Set previous packet start */
> -	prev_indices = hv_get_ring_bufferindices(outring_info);
> +	prev_indices = (u64)old_write << 32;
> 
>  	next_write_location = hv_copyto_ringbuffer(outring_info,
>  					     next_write_location,
> @@ -582,8 +575,8 @@ void hv_pkt_iter_close(struct vmbus_channel *channel)
>  	 * is updated.
>  	 */
>  	virt_rmb();
> -	start_read_index = rbi->ring_buffer->read_index;
> -	rbi->ring_buffer->read_index = rbi->priv_read_index;
> +	start_read_index = READ_ONCE(rbi->ring_buffer->read_index);
> +	WRITE_ONCE(rbi->ring_buffer->read_index, rbi->priv_read_index);
> 
>  	/*
>  	 * Older versions of Hyper-V (before WS2102 and Win8) do not
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 9e109d9..5c65820 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -214,7 +214,7 @@ static inline u32 hv_get_bytes_to_read(const struct
> hv_ring_buffer_info *rbi)
>  	u32 read_loc, write_loc, dsize, read;
> 
>  	dsize = rbi->ring_datasize;
> -	read_loc = rbi->ring_buffer->read_index;
> +	read_loc = READ_ONCE(rbi->ring_buffer->read_index);
>  	write_loc = READ_ONCE(rbi->ring_buffer->write_index);
> 
>  	read = write_loc >= read_loc ? (write_loc - read_loc) :
> @@ -229,7 +229,7 @@ static inline u32 hv_get_bytes_to_write(const struct
> hv_ring_buffer_info *rbi)
> 
>  	dsize = rbi->ring_datasize;
>  	read_loc = READ_ONCE(rbi->ring_buffer->read_index);
> -	write_loc = rbi->ring_buffer->write_index;
> +	write_loc = READ_ONCE(rbi->ring_buffer->write_index);
> 
>  	write = write_loc >= read_loc ? dsize - (write_loc - read_loc) :
>  		read_loc - write_loc;
> 
> --
> 2.45.4