From: Kameron Carr <[email protected]> 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 <[email protected]>
My quibble with the Subject notwithstanding,
Reviewed-by: Michael Kelley <[email protected]>
> ---
> 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
