* Re: [PATCH RFC] pvrdma: wean code off pvrdma_ring.h kernel header
2021-01-22 18:00 [PATCH RFC] pvrdma: wean code off pvrdma_ring.h kernel header Cornelia Huck
@ 2021-01-29 15:27 ` Cornelia Huck
2021-02-08 17:17 ` Cornelia Huck
2021-02-08 18:29 ` Michael S. Tsirkin
2021-02-10 8:56 ` Yuval Shaia
2 siblings, 1 reply; 10+ messages in thread
From: Cornelia Huck @ 2021-01-29 15:27 UTC (permalink / raw)
To: Yuval Shaia, Marcel Apfelbaum
Cc: Paolo Bonzini, qemu-devel, Michael S. Tsirkin
On Fri, 22 Jan 2021 19:00:29 +0100
Cornelia Huck <cohuck@redhat.com> 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>
> ---
>
> Compile-tested only, needs both testing and more eyeballs :)
Friendly ping :)
Suggestions for a test setup to do some sanity checks (that does not
require special hardware) also welcome.
>
> ---
> 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/"
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RFC] pvrdma: wean code off pvrdma_ring.h kernel header
2021-01-29 15:27 ` Cornelia Huck
@ 2021-02-08 17:17 ` Cornelia Huck
2021-02-08 17:28 ` Paolo Bonzini
0 siblings, 1 reply; 10+ messages in thread
From: Cornelia Huck @ 2021-02-08 17:17 UTC (permalink / raw)
To: Yuval Shaia, Marcel Apfelbaum
Cc: Paolo Bonzini, qemu-devel, Michael S. Tsirkin
On Fri, 29 Jan 2021 16:27:19 +0100
Cornelia Huck <cohuck@redhat.com> wrote:
> On Fri, 22 Jan 2021 19:00:29 +0100
> Cornelia Huck <cohuck@redhat.com> 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>
> > ---
> >
> > Compile-tested only, needs both testing and more eyeballs :)
>
> Friendly ping :)
>
> Suggestions for a test setup to do some sanity checks (that does not
> require special hardware) also welcome.
Can I interest anyone in this? I'd be happy doing sanity tests myself,
but I have a hard time figuring out even where to start...
>
> >
> > ---
> > 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/"
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RFC] pvrdma: wean code off pvrdma_ring.h kernel header
2021-02-08 17:17 ` Cornelia Huck
@ 2021-02-08 17:28 ` Paolo Bonzini
2021-02-09 3:47 ` Jason Wang
0 siblings, 1 reply; 10+ messages in thread
From: Paolo Bonzini @ 2021-02-08 17:28 UTC (permalink / raw)
To: Cornelia Huck, Yuval Shaia, Marcel Apfelbaum,
Jason Wang (jasowang@redhat.com)
Cc: qemu-devel, Michael S. Tsirkin
On 08/02/21 18:17, Cornelia Huck wrote:
> On Fri, 29 Jan 2021 16:27:19 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
>
>> On Fri, 22 Jan 2021 19:00:29 +0100
>> Cornelia Huck <cohuck@redhat.com> 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>
>>> ---
>>>
>>> Compile-tested only, needs both testing and more eyeballs :)
>>
>> Friendly ping :)
>>
>> Suggestions for a test setup to do some sanity checks (that does not
>> require special hardware) also welcome.
>
> Can I interest anyone in this? I'd be happy doing sanity tests myself,
> but I have a hard time figuring out even where to start...
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Jason, Michael, are you going to pick this up?
Paolo
>>
>>>
>>> ---
>>> 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/"
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RFC] pvrdma: wean code off pvrdma_ring.h kernel header
2021-02-08 17:28 ` Paolo Bonzini
@ 2021-02-09 3:47 ` Jason Wang
2021-02-22 8:33 ` Marcel Apfelbaum
0 siblings, 1 reply; 10+ messages in thread
From: Jason Wang @ 2021-02-09 3:47 UTC (permalink / raw)
To: Paolo Bonzini, Cornelia Huck, Yuval Shaia, Marcel Apfelbaum
Cc: qemu-devel, Michael S. Tsirkin
On 2021/2/9 上午1:28, Paolo Bonzini wrote:
> On 08/02/21 18:17, Cornelia Huck wrote:
>> On Fri, 29 Jan 2021 16:27:19 +0100
>> Cornelia Huck <cohuck@redhat.com> wrote:
>>
>>> On Fri, 22 Jan 2021 19:00:29 +0100
>>> Cornelia Huck <cohuck@redhat.com> 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>
>>>> ---
>>>>
>>>> Compile-tested only, needs both testing and more eyeballs :)
>>>
>>> Friendly ping :)
>>>
>>> Suggestions for a test setup to do some sanity checks (that does not
>>> require special hardware) also welcome.
>>
>> Can I interest anyone in this? I'd be happy doing sanity tests myself,
>> but I have a hard time figuring out even where to start...
>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>
> Jason, Michael, are you going to pick this up?
>
> Paolo
I will queue this.
Thanks
>
>>>
>>>>
>>>> ---
>>>> 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/"
>>>
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RFC] pvrdma: wean code off pvrdma_ring.h kernel header
2021-02-09 3:47 ` Jason Wang
@ 2021-02-22 8:33 ` Marcel Apfelbaum
2021-02-22 8:40 ` Jason Wang
0 siblings, 1 reply; 10+ messages in thread
From: Marcel Apfelbaum @ 2021-02-22 8:33 UTC (permalink / raw)
To: Jason Wang
Cc: qemu devel list, Paolo Bonzini, Cornelia Huck, Yuval Shaia,
Michael S. Tsirkin
[-- Attachment #1: Type: text/plain, Size: 1766 bytes --]
Hi Jason.
On Tue, Feb 9, 2021 at 5:47 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On 2021/2/9 上午1:28, Paolo Bonzini wrote:
> > On 08/02/21 18:17, Cornelia Huck wrote:
> >> On Fri, 29 Jan 2021 16:27:19 +0100
> >> Cornelia Huck <cohuck@redhat.com> wrote:
> >>
> >>> On Fri, 22 Jan 2021 19:00:29 +0100
> >>> Cornelia Huck <cohuck@redhat.com> 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>
> >>>> ---
> >>>>
> >>>> Compile-tested only, needs both testing and more eyeballs :)
> >>>
> >>> Friendly ping :)
> >>>
> >>> Suggestions for a test setup to do some sanity checks (that does not
> >>> require special hardware) also welcome.
> >>
> >> Can I interest anyone in this? I'd be happy doing sanity tests myself,
> >> but I have a hard time figuring out even where to start...
> >
> > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> >
> > Jason, Michael, are you going to pick this up?
> >
> > Paolo
>
>
> I will queue this.
>
Have you picked it up? It will be great so I'll not send a PR with a single
patch...
BTW, Yuval tested and acked the patch.
Thanks,
Marcel
[...]
>
>
[-- Attachment #2: Type: text/html, Size: 2964 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RFC] pvrdma: wean code off pvrdma_ring.h kernel header
2021-02-22 8:33 ` Marcel Apfelbaum
@ 2021-02-22 8:40 ` Jason Wang
0 siblings, 0 replies; 10+ messages in thread
From: Jason Wang @ 2021-02-22 8:40 UTC (permalink / raw)
To: Marcel Apfelbaum
Cc: Paolo Bonzini, Cornelia Huck, Michael S. Tsirkin,
qemu devel list, Yuval Shaia
----- 原始邮件 -----
> Hi Jason.
>
> On Tue, Feb 9, 2021 at 5:47 AM Jason Wang <jasowang@redhat.com> wrote:
>
> >
> > On 2021/2/9 上午1:28, Paolo Bonzini wrote:
> > > On 08/02/21 18:17, Cornelia Huck wrote:
> > >> On Fri, 29 Jan 2021 16:27:19 +0100
> > >> Cornelia Huck <cohuck@redhat.com> wrote:
> > >>
> > >>> On Fri, 22 Jan 2021 19:00:29 +0100
> > >>> Cornelia Huck <cohuck@redhat.com> 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>
> > >>>> ---
> > >>>>
> > >>>> Compile-tested only, needs both testing and more eyeballs :)
> > >>>
> > >>> Friendly ping :)
> > >>>
> > >>> Suggestions for a test setup to do some sanity checks (that does not
> > >>> require special hardware) also welcome.
> > >>
> > >> Can I interest anyone in this? I'd be happy doing sanity tests myself,
> > >> but I have a hard time figuring out even where to start...
> > >
> > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> > >
> > > Jason, Michael, are you going to pick this up?
> > >
> > > Paolo
> >
> >
> > I will queue this.
> >
>
>
> Have you picked it up? It will be great so I'll not send a PR with a single
> patch...
> BTW, Yuval tested and acked the patch.
Plan to send a pull request this Friday.
It should be part of that.
Thanks
>
> Thanks,
> Marcel
>
> [...]
>
>
> >
> >
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RFC] pvrdma: wean code off pvrdma_ring.h kernel header
2021-01-22 18:00 [PATCH RFC] pvrdma: wean code off pvrdma_ring.h kernel header Cornelia Huck
2021-01-29 15:27 ` Cornelia Huck
@ 2021-02-08 18:29 ` Michael S. Tsirkin
2021-02-09 7:40 ` Cornelia Huck
2021-02-10 8:56 ` Yuval Shaia
2 siblings, 1 reply; 10+ messages in thread
From: Michael S. Tsirkin @ 2021-02-08 18:29 UTC (permalink / raw)
To: Cornelia Huck; +Cc: Paolo Bonzini, Yuval Shaia, qemu-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RFC] pvrdma: wean code off pvrdma_ring.h kernel header
2021-02-08 18:29 ` Michael S. Tsirkin
@ 2021-02-09 7:40 ` Cornelia Huck
0 siblings, 0 replies; 10+ messages in thread
From: Cornelia Huck @ 2021-02-09 7:40 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Paolo Bonzini, Yuval Shaia, qemu-devel
On Mon, 8 Feb 2021 13:29:53 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> 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?
> > 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
> >
/* struct pvrdma_ring from drivers/infiniband/hw/vmw_pvrdma/pvrdma_ring.h */
> > +typedef struct PvrdmaRingState {
> > + int prod_tail; /* producer tail */
> > + int cons_head; /* consumer head */
> > +} PvrdmaRingState;
> > +
I guess this can be folded in?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RFC] pvrdma: wean code off pvrdma_ring.h kernel header
2021-01-22 18:00 [PATCH RFC] pvrdma: wean code off pvrdma_ring.h kernel header Cornelia Huck
2021-01-29 15:27 ` Cornelia Huck
2021-02-08 18:29 ` Michael S. Tsirkin
@ 2021-02-10 8:56 ` Yuval Shaia
2 siblings, 0 replies; 10+ messages in thread
From: Yuval Shaia @ 2021-02-10 8:56 UTC (permalink / raw)
To: Cornelia Huck; +Cc: Paolo Bonzini, QEMU Developers, Michael S. Tsirkin
[-- Attachment #1: Type: text/plain, Size: 14931 bytes --]
On Fri, 22 Jan 2021 at 20:00, Cornelia Huck <cohuck@redhat.com> 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>
> ---
>
> 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
>
>
Thanks!
I guess somewhere in the kernel there is such a clean and generic
implementation of a ring and VM folks could utilize that instead of writing
their own.
Tested-by: Yuval Shaia <yuval.shaia.ml@gmail.com>
Reviewed-by: Yuval Shaia <yuval.shaia.ml@gmail.com>
[-- Attachment #2: Type: text/html, Size: 18074 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread