Subject: Re: [PATCH for-next v2 2/4] IB/hfi1: Move rvt_cq_wc struct into uapi directory
On Wed, Dec 05, 2018 at 07:10:52AM -0800, Dennis Dalessandro wrote:
> +#include <linux/types.h>
> +#include <rdma/ib_user_verbs.h>
> +#ifndef RDMA_ATOMIC_UAPI
> +#define RDMA_ATOMIC_UAPI(_type, _name) _type _name #endif
> +/*
> + * This structure is used to contain the head pointer, tail pointer,
> + * and completion queue entries as a single memory allocation so
> + * it can be mmap'ed into user space.
> + */
> +struct rvt_cq_wc {
> + /* index of next entry to fill */
> + RDMA_ATOMIC_UAPI(u32, head);
> + /* index of next ib_poll_cq() entry */
> + RDMA_ATOMIC_UAPI(u32, tail);
> I went and looked at all the readers of these variables, and they are all missing a READ_ONCE (ie the kernel version of
> atomic_load(relaxed))
> I think you should change the definition to be:
> #define RDMA_ATOMIC_UAPI(_type, _name) struct {_type atomic_val;} _name
> And then add a kernel function
> #define RDMA_READ_UAPI_ATOMIC(_member) READ_ONCE((_memb)->atomic_val)
> So that the compiler protects against this mistake.
There is already a producer and consumer lock for the circular buffer and only reading the opposition index needs the READ_ONCE control. Example whenever a producer fills the buffer, producer lock is used and READ_ONCE used to for tail variable. We doesn't need to use READ_ONCE for head variable as it is already in context of locking mechanism and additional memory barrier and volatile casts might only make it slower in execution and prevent the advantage of compiler optimization here. We took design model from below :
https://www.kernel.org/doc/Documentation/circular-buffers.txt
Therefore, adding this kernel function is not needed if the index that is being read is protected by the spin lock. Only the peer index requires the READ_ONCE.
We going to make sure only those code path that should have READ_ONCE is in place correctly.
