Re: [PATCH RFC] pvrdma: wean code off pvrdma_ring.h kernel header

[Michael S. Tsirkin]

On Fri, Jan 22, 2021 at 07:00:29PM +0100, Cornelia Huck wrote:
> The pvrdma code relies on the pvrdma_ring.h kernel header for some
> basic ring buffer handling. The content of that header isn't very
> exciting, but contains some (q)atomic_*() invocations that (a)
> cause manual massaging when doing a headers update, and (b) are
> an indication that we probably should not be importing that header
> at all.
> 
> Let's reimplement the ring buffer handling directly in the pvrdma
> code instead. This arguably also improves readability of the code.
> 
> Importing the header can now be dropped.
> 
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>

Given it's a single struct that we need, this is a good fix.
How about adding a comment explaining where it came from,
just in case rdma guys decide to export this in uapi properly?

> ---
> 
> Compile-tested only, needs both testing and more eyeballs :)
> 
> ---
>  hw/rdma/vmw/pvrdma.h                          |   5 +-
>  hw/rdma/vmw/pvrdma_cmd.c                      |   6 +-
>  hw/rdma/vmw/pvrdma_dev_ring.c                 |  41 ++++---
>  hw/rdma/vmw/pvrdma_dev_ring.h                 |   9 +-
>  hw/rdma/vmw/pvrdma_main.c                     |   4 +-
>  .../infiniband/hw/vmw_pvrdma/pvrdma_ring.h    | 114 ------------------
>  scripts/update-linux-headers.sh               |   3 +-
>  7 files changed, 38 insertions(+), 144 deletions(-)
>  delete mode 100644 
> include/standard-headers/drivers/infiniband/hw/vmw_pvrdma/pvrdma_ring.h
> 
> diff --git a/hw/rdma/vmw/pvrdma.h b/hw/rdma/vmw/pvrdma.h
> index 1d36a76f1e3b..d08965d3e2d5 100644
> --- a/hw/rdma/vmw/pvrdma.h
> +++ b/hw/rdma/vmw/pvrdma.h
> @@ -26,7 +26,6 @@
>  #include "../rdma_backend_defs.h"
>  #include "../rdma_rm_defs.h"
>  
> -#include "standard-headers/drivers/infiniband/hw/vmw_pvrdma/pvrdma_ring.h"
>  #include "standard-headers/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h"
>  #include "pvrdma_dev_ring.h"
>  #include "qom/object.h"
> @@ -64,10 +63,10 @@ typedef struct DSRInfo {
>      union pvrdma_cmd_req *req;
>      union pvrdma_cmd_resp *rsp;
>  
> -    struct pvrdma_ring *async_ring_state;
> +    PvrdmaRingState *async_ring_state;
>      PvrdmaRing async;
>  
> -    struct pvrdma_ring *cq_ring_state;
> +    PvrdmaRingState *cq_ring_state;
>      PvrdmaRing cq;
>  } DSRInfo;
>  
> diff --git a/hw/rdma/vmw/pvrdma_cmd.c b/hw/rdma/vmw/pvrdma_cmd.c
> index 692125ac2681..f59879e2574e 100644
> --- a/hw/rdma/vmw/pvrdma_cmd.c
> +++ b/hw/rdma/vmw/pvrdma_cmd.c
> @@ -262,7 +262,7 @@ static int create_cq_ring(PCIDevice *pci_dev , PvrdmaRing 
> **ring,
>      r = g_malloc(sizeof(*r));
>      *ring = r;
>  
> -    r->ring_state = (struct pvrdma_ring *)
> +    r->ring_state = (PvrdmaRingState *)
>          rdma_pci_dma_map(pci_dev, tbl[0], TARGET_PAGE_SIZE);
>  
>      if (!r->ring_state) {
> @@ -398,7 +398,7 @@ static int create_qp_rings(PCIDevice *pci_dev, uint64_t 
> pdir_dma,
>      *rings = sr;
>  
>      /* Create send ring */
> -    sr->ring_state = (struct pvrdma_ring *)
> +    sr->ring_state = (PvrdmaRingState *)
>          rdma_pci_dma_map(pci_dev, tbl[0], TARGET_PAGE_SIZE);
>      if (!sr->ring_state) {
>          rdma_error_report("Failed to map to QP ring state");
> @@ -639,7 +639,7 @@ static int create_srq_ring(PCIDevice *pci_dev, PvrdmaRing 
> **ring,
>      r = g_malloc(sizeof(*r));
>      *ring = r;
>  
> -    r->ring_state = (struct pvrdma_ring *)
> +    r->ring_state = (PvrdmaRingState *)
>              rdma_pci_dma_map(pci_dev, tbl[0], TARGET_PAGE_SIZE);
>      if (!r->ring_state) {
>          rdma_error_report("Failed to map tp SRQ ring state");
> diff --git a/hw/rdma/vmw/pvrdma_dev_ring.c b/hw/rdma/vmw/pvrdma_dev_ring.c
> index f0bcde74b06a..074ac59b84db 100644
> --- a/hw/rdma/vmw/pvrdma_dev_ring.c
> +++ b/hw/rdma/vmw/pvrdma_dev_ring.c
> @@ -22,11 +22,10 @@
>  #include "trace.h"
>  
>  #include "../rdma_utils.h"
> -#include "standard-headers/drivers/infiniband/hw/vmw_pvrdma/pvrdma_ring.h"
>  #include "pvrdma_dev_ring.h"
>  
>  int pvrdma_ring_init(PvrdmaRing *ring, const char *name, PCIDevice *dev,
> -                     struct pvrdma_ring *ring_state, uint32_t max_elems,
> +                     PvrdmaRingState *ring_state, uint32_t max_elems,
>                       size_t elem_sz, dma_addr_t *tbl, uint32_t npages)
>  {
>      int i;
> @@ -73,48 +72,54 @@ out:
>  
>  void *pvrdma_ring_next_elem_read(PvrdmaRing *ring)
>  {
> -    int e;
> -    unsigned int idx = 0, offset;
> +    unsigned int idx, offset;
> +    const uint32_t tail = qatomic_read(&ring->ring_state->prod_tail);
> +    const uint32_t head = qatomic_read(&ring->ring_state->cons_head);
>  
> -    e = pvrdma_idx_ring_has_data(ring->ring_state, ring->max_elems, &idx);
> -    if (e <= 0) {
> +    if (tail & ~((ring->max_elems << 1) - 1) ||
> +        head & ~((ring->max_elems << 1) - 1) ||
> +        tail == head) {
>          trace_pvrdma_ring_next_elem_read_no_data(ring->name);
>          return NULL;
>      }
>  
> +    idx = head & (ring->max_elems - 1);
>      offset = idx * ring->elem_sz;
>      return ring->pages[offset / TARGET_PAGE_SIZE] + (offset % 
> TARGET_PAGE_SIZE);
>  }
>  
>  void pvrdma_ring_read_inc(PvrdmaRing *ring)
>  {
> -    pvrdma_idx_ring_inc(&ring->ring_state->cons_head, ring->max_elems);
> +    uint32_t idx = qatomic_read(&ring->ring_state->cons_head);
> +
> +    idx = (idx + 1) & ((ring->max_elems << 1) - 1);
> +    qatomic_set(&ring->ring_state->cons_head, idx);
>  }
>  
>  void *pvrdma_ring_next_elem_write(PvrdmaRing *ring)
>  {
> -    int idx;
> -    unsigned int offset, tail;
> +    unsigned int idx, offset;
> +    const uint32_t tail = qatomic_read(&ring->ring_state->prod_tail);
> +    const uint32_t head = qatomic_read(&ring->ring_state->cons_head);
>  
> -    idx = pvrdma_idx_ring_has_space(ring->ring_state, ring->max_elems, 
> &tail);
> -    if (idx <= 0) {
> +    if (tail & ~((ring->max_elems << 1) - 1) ||
> +        head & ~((ring->max_elems << 1) - 1) ||
> +        tail == (head ^ ring->max_elems)) {
>          rdma_error_report("CQ is full");
>          return NULL;
>      }
>  
> -    idx = pvrdma_idx(&ring->ring_state->prod_tail, ring->max_elems);
> -    if (idx < 0 || tail != idx) {
> -        rdma_error_report("Invalid idx %d", idx);
> -        return NULL;
> -    }
> -
> +    idx = tail & (ring->max_elems - 1);
>      offset = idx * ring->elem_sz;
>      return ring->pages[offset / TARGET_PAGE_SIZE] + (offset % 
> TARGET_PAGE_SIZE);
>  }
>  
>  void pvrdma_ring_write_inc(PvrdmaRing *ring)
>  {
> -    pvrdma_idx_ring_inc(&ring->ring_state->prod_tail, ring->max_elems);
> +    uint32_t idx = qatomic_read(&ring->ring_state->prod_tail);
> +
> +    idx = (idx + 1) & ((ring->max_elems << 1) - 1);
> +    qatomic_set(&ring->ring_state->prod_tail, idx);
>  }
>  
>  void pvrdma_ring_free(PvrdmaRing *ring)
> diff --git a/hw/rdma/vmw/pvrdma_dev_ring.h b/hw/rdma/vmw/pvrdma_dev_ring.h
> index 5f2a0cf9b9fa..d231588ce004 100644
> --- a/hw/rdma/vmw/pvrdma_dev_ring.h
> +++ b/hw/rdma/vmw/pvrdma_dev_ring.h
> @@ -19,18 +19,23 @@
>  
>  #define MAX_RING_NAME_SZ 32
>  
> +typedef struct PvrdmaRingState {
> +    int prod_tail; /* producer tail */
> +    int cons_head; /* consumer head */
> +} PvrdmaRingState;
> +
>  typedef struct PvrdmaRing {
>      char name[MAX_RING_NAME_SZ];
>      PCIDevice *dev;
>      uint32_t max_elems;
>      size_t elem_sz;
> -    struct pvrdma_ring *ring_state; /* used only for unmap */
> +    PvrdmaRingState *ring_state; /* used only for unmap */
>      int npages;
>      void **pages;
>  } PvrdmaRing;
>  
>  int pvrdma_ring_init(PvrdmaRing *ring, const char *name, PCIDevice *dev,
> -                     struct pvrdma_ring *ring_state, uint32_t max_elems,
> +                     PvrdmaRingState *ring_state, uint32_t max_elems,
>                       size_t elem_sz, dma_addr_t *tbl, uint32_t npages);
>  void *pvrdma_ring_next_elem_read(PvrdmaRing *ring);
>  void pvrdma_ring_read_inc(PvrdmaRing *ring);
> diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
> index 85935703322f..84ae8024fcfd 100644
> --- a/hw/rdma/vmw/pvrdma_main.c
> +++ b/hw/rdma/vmw/pvrdma_main.c
> @@ -85,7 +85,7 @@ static void free_dev_ring(PCIDevice *pci_dev, PvrdmaRing 
> *ring,
>      rdma_pci_dma_unmap(pci_dev, ring_state, TARGET_PAGE_SIZE);
>  }
>  
> -static int init_dev_ring(PvrdmaRing *ring, struct pvrdma_ring **ring_state,
> +static int init_dev_ring(PvrdmaRing *ring, PvrdmaRingState **ring_state,
>                           const char *name, PCIDevice *pci_dev,
>                           dma_addr_t dir_addr, uint32_t num_pages)
>  {
> @@ -114,7 +114,7 @@ static int init_dev_ring(PvrdmaRing *ring, struct 
> pvrdma_ring **ring_state,
>      /* RX ring is the second */
>      (*ring_state)++;
>      rc = pvrdma_ring_init(ring, name, pci_dev,
> -                          (struct pvrdma_ring *)*ring_state,
> +                          (PvrdmaRingState *)*ring_state,
>                            (num_pages - 1) * TARGET_PAGE_SIZE /
>                            sizeof(struct pvrdma_cqne),
>                            sizeof(struct pvrdma_cqne),
> diff --git 
> a/include/standard-headers/drivers/infiniband/hw/vmw_pvrdma/pvrdma_ring.h 
> b/include/standard-headers/drivers/infiniband/hw/vmw_pvrdma/pvrdma_ring.h
> deleted file mode 100644
> index 7b4062a1a107..000000000000
> --- a/include/standard-headers/drivers/infiniband/hw/vmw_pvrdma/pvrdma_ring.h
> +++ /dev/null
> @@ -1,114 +0,0 @@
> -/*
> - * Copyright (c) 2012-2016 VMware, Inc.  All rights reserved.
> - *
> - * This program is free software; you can redistribute it and/or
> - * modify it under the terms of EITHER the GNU General Public License
> - * version 2 as published by the Free Software Foundation or the BSD
> - * 2-Clause License. This program is distributed in the hope that it
> - * will be useful, but WITHOUT ANY WARRANTY; WITHOUT EVEN THE IMPLIED
> - * WARRANTY OF MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE.
> - * See the GNU General Public License version 2 for more details at
> - * http://www.gnu.org/licenses/old-licenses/gpl-2.0.en.html.
> - *
> - * You should have received a copy of the GNU General Public License
> - * along with this program available in the file COPYING in the main
> - * directory of this source tree.
> - *
> - * The BSD 2-Clause License
> - *
> - *     Redistribution and use in source and binary forms, with or
> - *     without modification, are permitted provided that the following
> - *     conditions are met:
> - *
> - *      - Redistributions of source code must retain the above
> - *        copyright notice, this list of conditions and the following
> - *        disclaimer.
> - *
> - *      - Redistributions in binary form must reproduce the above
> - *        copyright notice, this list of conditions and the following
> - *        disclaimer in the documentation and/or other materials
> - *        provided with the distribution.
> - *
> - * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> - * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> - * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
> - * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
> - * COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
> - * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> - * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
> - * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> - * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
> - * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> - * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED
> - * OF THE POSSIBILITY OF SUCH DAMAGE.
> - */
> -
> -#ifndef __PVRDMA_RING_H__
> -#define __PVRDMA_RING_H__
> -
> -#include "standard-headers/linux/types.h"
> -
> -#define PVRDMA_INVALID_IDX   -1      /* Invalid index. */
> -
> -struct pvrdma_ring {
> -     int prod_tail;  /* Producer tail. */
> -     int cons_head;  /* Consumer head. */
> -};
> -
> -struct pvrdma_ring_state {
> -     struct pvrdma_ring tx;  /* Tx ring. */
> -     struct pvrdma_ring rx;  /* Rx ring. */
> -};
> -
> -static inline int pvrdma_idx_valid(uint32_t idx, uint32_t max_elems)
> -{
> -     /* Generates fewer instructions than a less-than. */
> -     return (idx & ~((max_elems << 1) - 1)) == 0;
> -}
> -
> -static inline int32_t pvrdma_idx(int *var, uint32_t max_elems)
> -{
> -     const unsigned int idx = qatomic_read(var);
> -
> -     if (pvrdma_idx_valid(idx, max_elems))
> -             return idx & (max_elems - 1);
> -     return PVRDMA_INVALID_IDX;
> -}
> -
> -static inline void pvrdma_idx_ring_inc(int *var, uint32_t max_elems)
> -{
> -     uint32_t idx = qatomic_read(var) + 1;   /* Increment. */
> -
> -     idx &= (max_elems << 1) - 1;            /* Modulo size, flip gen. */
> -     qatomic_set(var, idx);
> -}
> -
> -static inline int32_t pvrdma_idx_ring_has_space(const struct pvrdma_ring *r,
> -                                           uint32_t max_elems, uint32_t 
> *out_tail)
> -{
> -     const uint32_t tail = qatomic_read(&r->prod_tail);
> -     const uint32_t head = qatomic_read(&r->cons_head);
> -
> -     if (pvrdma_idx_valid(tail, max_elems) &&
> -         pvrdma_idx_valid(head, max_elems)) {
> -             *out_tail = tail & (max_elems - 1);
> -             return tail != (head ^ max_elems);
> -     }
> -     return PVRDMA_INVALID_IDX;
> -}
> -
> -static inline int32_t pvrdma_idx_ring_has_data(const struct pvrdma_ring *r,
> -                                          uint32_t max_elems, uint32_t 
> *out_head)
> -{
> -     const uint32_t tail = qatomic_read(&r->prod_tail);
> -     const uint32_t head = qatomic_read(&r->cons_head);
> -
> -     if (pvrdma_idx_valid(tail, max_elems) &&
> -         pvrdma_idx_valid(head, max_elems)) {
> -             *out_head = head & (max_elems - 1);
> -             return tail != head;
> -     }
> -     return PVRDMA_INVALID_IDX;
> -}
> -
> -#endif /* __PVRDMA_RING_H__ */
> diff --git a/scripts/update-linux-headers.sh b/scripts/update-linux-headers.sh
> index fa6f2b6272b7..1050e361694f 100755
> --- a/scripts/update-linux-headers.sh
> +++ b/scripts/update-linux-headers.sh
> @@ -215,8 +215,7 @@ sed  -e '1h;2,$H;$!d;g'  -e 's/[^};]*pvrdma[^(| 
> ]*([^)]*);//g' \
>      "$linux/drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.h" > \
>      "$tmp_pvrdma_verbs";
>  
> -for i in "$linux/drivers/infiniband/hw/vmw_pvrdma/pvrdma_ring.h" \
> -         "$linux/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h" \
> +for i in "$linux/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h" \
>           "$tmp_pvrdma_verbs"; do \
>      cp_portable "$i" \
>           "$output/include/standard-headers/drivers/infiniband/hw/vmw_pvrdma/"
> -- 
> 2.26.2