[PATCH v3 3/7] dma/idxd: replace rte atomics with GCC builtin atomics

[Bruce Richardson]

On Wed, May 24, 2023 at 10:09:04PM +0200, David Marchand wrote:
> Hello Bruce, Kevin,
> 
> Review please.
> 
> 
> On Thu, Mar 23, 2023 at 11:54 PM Tyler Retzlaff
> <roretzla at linux.microsoft.com> wrote:
> >
> > Replace the use of rte_atomic.h types and functions, instead use GCC
> > supplied C++11 memory model builtins.
> >
> > Signed-off-by: Tyler Retzlaff <roretzla at linux.microsoft.com>

Two small comments inline below.

Acked-by: Bruce Richardson <bruce.richardson at intel.com>

> > ---
> >  drivers/dma/idxd/idxd_internal.h | 3 +--
> >  drivers/dma/idxd/idxd_pci.c      | 8 +++++---
> >  2 files changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/dma/idxd/idxd_internal.h b/drivers/dma/idxd/idxd_internal.h
> > index 180a858..cd41777 100644
> > --- a/drivers/dma/idxd/idxd_internal.h
> > +++ b/drivers/dma/idxd/idxd_internal.h
> > @@ -7,7 +7,6 @@
> >
> >  #include <rte_dmadev_pmd.h>
> >  #include <rte_spinlock.h>
> > -#include <rte_atomic.h>
> >
> >  #include "idxd_hw_defs.h"
> >
> > @@ -34,7 +33,7 @@ struct idxd_pci_common {
> >         rte_spinlock_t lk;
> >
> >         uint8_t wq_cfg_sz;
> > -       rte_atomic16_t ref_count;
> > +       uint16_t ref_count;
> >         volatile struct rte_idxd_bar0 *regs;
> >         volatile uint32_t *wq_regs_base;
> >         volatile struct rte_idxd_grpcfg *grp_regs;
> > diff --git a/drivers/dma/idxd/idxd_pci.c b/drivers/dma/idxd/idxd_pci.c
> > index 781fa02..2de5d15 100644
> > --- a/drivers/dma/idxd/idxd_pci.c
> > +++ b/drivers/dma/idxd/idxd_pci.c
> > @@ -6,7 +6,6 @@
> >  #include <rte_devargs.h>
> >  #include <rte_dmadev_pmd.h>
> >  #include <rte_malloc.h>
> > -#include <rte_atomic.h>
> >
> >  #include "idxd_internal.h"
> >
> > @@ -136,7 +135,9 @@
> >         /* if this is the last WQ on the device, disable the device and free
> >          * the PCI struct
> >          */
> > -       is_last_wq = rte_atomic16_dec_and_test(&idxd->u.pci->ref_count);
> > +       /* NOTE: review for potential ordering optimization */
> > +       is_last_wq = __atomic_fetch_sub(&idxd->u.pci->ref_count, 1,
> > +               __ATOMIC_SEQ_CST) - 1 == 0;

Rather than "__atomic_fetch_sub(...) - 1 == 0", I think just comparing
"== 1" is simpler and better. I would also bracket the comparison for
clarity.

> >         if (is_last_wq) {
> >                 /* disable the device */
> >                 err_code = idxd_pci_dev_command(idxd, idxd_disable_dev);
> > @@ -350,7 +351,8 @@
> >                                 free(idxd.u.pci);
> >                         return ret;
> >                 }
> > -               rte_atomic16_inc(&idxd.u.pci->ref_count);
> > +               /* NOTE: review for potential ordering optimization */

I think we can drop the note. Since this is not datapath code the perf is
not that important.

> > +               __atomic_fetch_add(&idxd.u.pci->ref_count, 1, __ATOMIC_SEQ_CST);
> >         }
> >
> >         return 0;
> > --
> > 1.8.3.1
> >
> 
> -- 
> David Marchand
>