| Message ID | 1537463314-7807-3-git-send-email-devesh.sharma@broadcom.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | libnxt_re bug fixes | expand |
On Thu, Sep 20, 2018 at 01:08:32PM -0400, Devesh Sharma wrote: > Remove the lock around the doorbell. It isn't necessary > and was a bottleneck for multithreaded clients. Performance of > spdk improves by 40% with it removed. > > Signed-off-by: JD Zheng <jiandong.zheng@broadcom.com> > Signed-off-by: Jonathan Richardson <jonathan.richardson@broadcom.com> > Signed-off-by: Devesh Sharma <devesh.sharma@broadcom.com> > --- > providers/bnxt_re/db.c | 2 -- > providers/bnxt_re/main.c | 1 - > providers/bnxt_re/main.h | 1 - > providers/bnxt_re/verbs.c | 2 -- > 4 files changed, 6 deletions(-) > > diff --git a/providers/bnxt_re/db.c b/providers/bnxt_re/db.c > index a79f871..2896ceb 100644 > --- a/providers/bnxt_re/db.c > +++ b/providers/bnxt_re/db.c > @@ -44,11 +44,9 @@ static void bnxt_re_ring_db(struct bnxt_re_dpi *dpi, > { > __le64 *dbval; > > - pthread_spin_lock(&dpi->db_lock); > dbval = (__le64 *)&hdr->indx; > udma_to_device_barrier(); > iowrite64(dpi->dbpage, dbval); > - pthread_spin_unlock(&dpi->db_lock); > } Nope, you have to convert this provider to use the mmio accessor helpers if you want to do this. Ie get rid of iowrite64/iowrite32 and use the macros in util/mmio.h They provide the necessary guarentee for atomicity to allow the lock to be removed. Jason
On Thu, Sep 20, 2018 at 10:43 PM Jason Gunthorpe <jgg@mellanox.com> wrote: > > On Thu, Sep 20, 2018 at 01:08:32PM -0400, Devesh Sharma wrote: > > Remove the lock around the doorbell. It isn't necessary > > and was a bottleneck for multithreaded clients. Performance of > > spdk improves by 40% with it removed. > > > > Signed-off-by: JD Zheng <jiandong.zheng@broadcom.com> > > Signed-off-by: Jonathan Richardson <jonathan.richardson@broadcom.com> > > Signed-off-by: Devesh Sharma <devesh.sharma@broadcom.com> > > --- > > providers/bnxt_re/db.c | 2 -- > > providers/bnxt_re/main.c | 1 - > > providers/bnxt_re/main.h | 1 - > > providers/bnxt_re/verbs.c | 2 -- > > 4 files changed, 6 deletions(-) > > > > diff --git a/providers/bnxt_re/db.c b/providers/bnxt_re/db.c > > index a79f871..2896ceb 100644 > > --- a/providers/bnxt_re/db.c > > +++ b/providers/bnxt_re/db.c > > @@ -44,11 +44,9 @@ static void bnxt_re_ring_db(struct bnxt_re_dpi *dpi, > > { > > __le64 *dbval; > > > > - pthread_spin_lock(&dpi->db_lock); > > dbval = (__le64 *)&hdr->indx; > > udma_to_device_barrier(); > > iowrite64(dpi->dbpage, dbval); > > - pthread_spin_unlock(&dpi->db_lock); > > } > > Nope, you have to convert this provider to use the mmio accessor > helpers if you want to do this. > > Ie get rid of iowrite64/iowrite32 and use the macros in util/mmio.h > > They provide the necessary guarentee for atomicity to allow the lock > to be removed. > > Jason Okay, this can be done. While removing the lock I was thinking what will happen on the 32 bit platform. would those new apis handle 32bit platforms too? -Regards Devesh
On Thu, Sep 20, 2018 at 11:16:46PM +0530, Devesh Sharma wrote: > On Thu, Sep 20, 2018 at 10:43 PM Jason Gunthorpe <jgg@mellanox.com> wrote: > > > > On Thu, Sep 20, 2018 at 01:08:32PM -0400, Devesh Sharma wrote: > > > Remove the lock around the doorbell. It isn't necessary > > > and was a bottleneck for multithreaded clients. Performance of > > > spdk improves by 40% with it removed. > > > > > > Signed-off-by: JD Zheng <jiandong.zheng@broadcom.com> > > > Signed-off-by: Jonathan Richardson <jonathan.richardson@broadcom.com> > > > Signed-off-by: Devesh Sharma <devesh.sharma@broadcom.com> > > > providers/bnxt_re/db.c | 2 -- > > > providers/bnxt_re/main.c | 1 - > > > providers/bnxt_re/main.h | 1 - > > > providers/bnxt_re/verbs.c | 2 -- > > > 4 files changed, 6 deletions(-) > > > > > > diff --git a/providers/bnxt_re/db.c b/providers/bnxt_re/db.c > > > index a79f871..2896ceb 100644 > > > +++ b/providers/bnxt_re/db.c > > > @@ -44,11 +44,9 @@ static void bnxt_re_ring_db(struct bnxt_re_dpi *dpi, > > > { > > > __le64 *dbval; > > > > > > - pthread_spin_lock(&dpi->db_lock); > > > dbval = (__le64 *)&hdr->indx; > > > udma_to_device_barrier(); > > > iowrite64(dpi->dbpage, dbval); > > > - pthread_spin_unlock(&dpi->db_lock); > > > } > > > > Nope, you have to convert this provider to use the mmio accessor > > helpers if you want to do this. > > > > Ie get rid of iowrite64/iowrite32 and use the macros in util/mmio.h > > > > They provide the necessary guarentee for atomicity to allow the lock > > to be removed. > > > > Jason > > Okay, this can be done. While removing the lock I was thinking what > will happen on the 32 bit platform. would those new apis handle 32bit > platforms too? Yes Jason
On Thu, Sep 20, 2018 at 11:39 PM Jason Gunthorpe <jgg@mellanox.com> wrote: > > On Thu, Sep 20, 2018 at 11:16:46PM +0530, Devesh Sharma wrote: > > On Thu, Sep 20, 2018 at 10:43 PM Jason Gunthorpe <jgg@mellanox.com> wrote: > > > > > > On Thu, Sep 20, 2018 at 01:08:32PM -0400, Devesh Sharma wrote: > > > > Remove the lock around the doorbell. It isn't necessary > > > > and was a bottleneck for multithreaded clients. Performance of > > > > spdk improves by 40% with it removed. > > > > > > > > Signed-off-by: JD Zheng <jiandong.zheng@broadcom.com> > > > > Signed-off-by: Jonathan Richardson <jonathan.richardson@broadcom.com> > > > > Signed-off-by: Devesh Sharma <devesh.sharma@broadcom.com> > > > > providers/bnxt_re/db.c | 2 -- > > > > providers/bnxt_re/main.c | 1 - > > > > providers/bnxt_re/main.h | 1 - > > > > providers/bnxt_re/verbs.c | 2 -- > > > > 4 files changed, 6 deletions(-) > > > > > > > > diff --git a/providers/bnxt_re/db.c b/providers/bnxt_re/db.c > > > > index a79f871..2896ceb 100644 > > > > +++ b/providers/bnxt_re/db.c > > > > @@ -44,11 +44,9 @@ static void bnxt_re_ring_db(struct bnxt_re_dpi *dpi, > > > > { > > > > __le64 *dbval; > > > > > > > > - pthread_spin_lock(&dpi->db_lock); > > > > dbval = (__le64 *)&hdr->indx; > > > > udma_to_device_barrier(); > > > > iowrite64(dpi->dbpage, dbval); > > > > - pthread_spin_unlock(&dpi->db_lock); > > > > } > > > > > > Nope, you have to convert this provider to use the mmio accessor > > > helpers if you want to do this. > > > > > > Ie get rid of iowrite64/iowrite32 and use the macros in util/mmio.h > > > > > > They provide the necessary guarentee for atomicity to allow the lock > > > to be removed. > > > > > > Jason > > > > Okay, this can be done. While removing the lock I was thinking what > > will happen on the 32 bit platform. would those new apis handle 32bit > > platforms too? > > Yes Thanks, I will post a rev. > > Jason
diff --git a/providers/bnxt_re/db.c b/providers/bnxt_re/db.c index a79f871..2896ceb 100644 --- a/providers/bnxt_re/db.c +++ b/providers/bnxt_re/db.c @@ -44,11 +44,9 @@ static void bnxt_re_ring_db(struct bnxt_re_dpi *dpi, { __le64 *dbval; - pthread_spin_lock(&dpi->db_lock); dbval = (__le64 *)&hdr->indx; udma_to_device_barrier(); iowrite64(dpi->dbpage, dbval); - pthread_spin_unlock(&dpi->db_lock); } static void bnxt_re_init_db_hdr(struct bnxt_re_db_hdr *hdr, uint32_t indx, diff --git a/providers/bnxt_re/main.c b/providers/bnxt_re/main.c index 54e3cc3..a1ba06a 100644 --- a/providers/bnxt_re/main.c +++ b/providers/bnxt_re/main.c @@ -167,7 +167,6 @@ static void bnxt_re_free_context(struct ibv_context *ibvctx) * allocated in this context. */ if (cntx->udpi.dbpage && cntx->udpi.dbpage != MAP_FAILED) { - pthread_spin_destroy(&cntx->udpi.db_lock); munmap(cntx->udpi.dbpage, dev->pg_size); cntx->udpi.dbpage = NULL; } diff --git a/providers/bnxt_re/main.h b/providers/bnxt_re/main.h index 16f55f9..0b5c749 100644 --- a/providers/bnxt_re/main.h +++ b/providers/bnxt_re/main.h @@ -59,7 +59,6 @@ struct bnxt_re_dpi { __u32 dpindx; __u64 *dbpage; - pthread_spinlock_t db_lock; }; struct bnxt_re_pd { diff --git a/providers/bnxt_re/verbs.c b/providers/bnxt_re/verbs.c index 9ce1454..8e9fd8c 100644 --- a/providers/bnxt_re/verbs.c +++ b/providers/bnxt_re/verbs.c @@ -110,8 +110,6 @@ struct ibv_pd *bnxt_re_alloc_pd(struct ibv_context *ibvctx) (void)ibv_cmd_dealloc_pd(&pd->ibvpd); goto out; } - pthread_spin_init(&cntx->udpi.db_lock, - PTHREAD_PROCESS_PRIVATE); } return &pd->ibvpd;