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 <coh...@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
