Re: [PATCH RFC] pvrdma: wean code off pvrdma

qemu-devel
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH RFC] pvrdma: wean code off pvrdma_ring.h kernel header

From:	Cornelia Huck
Subject:	Re: [PATCH RFC] pvrdma: wean code off pvrdma_ring.h kernel header
Date:	Mon, 8 Feb 2021 18:17:45 +0100
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/"  
>
[Prev in Thread]
Current Thread
[Next in Thread]
Re: [PATCH RFC] pvrdma: wean code off pvrdma_ring.h kernel header, Cornelia Huck <=
- Re: [PATCH RFC] pvrdma: wean code off pvrdma_ring.h kernel header, Paolo Bonzini, 2021/02/08
  - Re: [PATCH RFC] pvrdma: wean code off pvrdma_ring.h kernel header, Jason Wang, 2021/02/08
    - Re: [PATCH RFC] pvrdma: wean code off pvrdma_ring.h kernel header, Marcel Apfelbaum, 2021/02/22
    - Re: [PATCH RFC] pvrdma: wean code off pvrdma_ring.h kernel header, Jason Wang, 2021/02/22
- Re: [PATCH RFC] pvrdma: wean code off pvrdma_ring.h kernel header, Michael S. Tsirkin, 2021/02/08
  - Re: [PATCH RFC] pvrdma: wean code off pvrdma_ring.h kernel header, Cornelia Huck, 2021/02/09
- Re: [PATCH RFC] pvrdma: wean code off pvrdma_ring.h kernel header, Yuval Shaia, 2021/02/10
Prev by Date: Re: [PATCH v2 1/3] hw/net: Add npcm7xx emc model
Next by Date: Re: [PATCH v2 2/3] hw/arm: Add npcm7xx emc model
Previous by thread: Re: [PATCH v2 6/6] hw/i2c: Implement NPCM7XX SMBus Module FIFO Mode
Next by thread: Re: [PATCH RFC] pvrdma: wean code off pvrdma_ring.h kernel header
Index(es):
- Date
- Thread