Re: [dpdk-dev] [PATCH 2/2] eal: use c11 atomics for interrupt status

[Phil Yang <Phil.Yang@arm.com>]

> -----Original Message-----
> From: Kinsella, Ray <mdr@ashroe.eu>
> Sent: Wednesday, July 8, 2020 11:05 PM
> To: David Marchand <david.marchand@redhat.com>; Phil Yang
> <Phil.Yang@arm.com>; Aaron Conole <aconole@redhat.com>
> Cc: dev <dev@dpdk.org>; David Christensen <drc@linux.vnet.ibm.com>;
> Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; nd <nd@arm.com>; Dodji Seketeli
> <dodji@redhat.com>; Neil Horman <nhorman@tuxdriver.com>; Harman
> Kalra <hkalra@marvell.com>
> Subject: Re: [dpdk-dev] [PATCH 2/2] eal: use c11 atomics for interrupt status
> 
> 
> 
> On 08/07/2020 13:29, David Marchand wrote:
> > On Thu, Jun 11, 2020 at 12:25 PM Phil Yang <phil.yang@arm.com> wrote:
> >>
> >> The event status is defined as a volatile variable and shared
> >> between threads. Use c11 atomics with explicit ordering instead
> >> of rte_atomic ops which enforce unnecessary barriers on aarch64.
> >>
> >> Signed-off-by: Phil Yang <phil.yang@arm.com>
> >> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> >> ---
> >>  lib/librte_eal/include/rte_eal_interrupts.h |  2 +-
> >>  lib/librte_eal/linux/eal_interrupts.c       | 47 ++++++++++++++++++++----
> -----
> >>  2 files changed, 34 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/lib/librte_eal/include/rte_eal_interrupts.h
> b/lib/librte_eal/include/rte_eal_interrupts.h
> >> index 773a34a..b1e8a29 100644
> >> --- a/lib/librte_eal/include/rte_eal_interrupts.h
> >> +++ b/lib/librte_eal/include/rte_eal_interrupts.h
> >> @@ -59,7 +59,7 @@ enum {
> >>
> >>  /** interrupt epoll event obj, taken by epoll_event.ptr */
> >>  struct rte_epoll_event {
> >> -       volatile uint32_t status;  /**< OUT: event status */
> >> +       uint32_t status;           /**< OUT: event status */
> >>         int fd;                    /**< OUT: event fd */
> >>         int epfd;       /**< OUT: epoll instance the ev associated with */
> >>         struct rte_epoll_data epdata;
> >
> > I got a reject from the ABI check in my env.
> >
> > 1 function with some indirect sub-type change:
> >
> >   [C]'function int rte_pci_ioport_map(rte_pci_device*, int,
> > rte_pci_ioport*)' at pci.c:756:1 has some indirect sub-type changes:
> >     parameter 1 of type 'rte_pci_device*' has sub-type changes:
> >       in pointed to type 'struct rte_pci_device' at rte_bus_pci.h:57:1:
> >         type size hasn't changed
> >         1 data member changes (2 filtered):
> >          type of 'rte_intr_handle rte_pci_device::intr_handle' changed:
> >            type size hasn't changed
> >            1 data member change:
> >             type of 'rte_epoll_event rte_intr_handle::elist[512]' changed:
> >               array element type 'struct rte_epoll_event' changed:
> >                 type size hasn't changed
> >                 1 data member change:
> >                  type of 'volatile uint32_t rte_epoll_event::status' changed:
> >                    entity changed from 'volatile uint32_t' to 'typedef
> > uint32_t' at stdint-uintn.h:26:1
> >                    type size hasn't changed
> >
> >               type size hasn't changed
> >
> >
> > This is probably harmless in our case (going from volatile to non
> > volatile), but it won't pass the check in the CI without an exception
> > rule.
> >
> > Note: checking on the test-report ml, I saw nothing, but ovsrobot did
> > catch the issue with this change too, Aaron?
> >
> >
> Agreed, probably harmless and requires something in libagigail.ignore.

OK. Will update libagigail.ignore in the next version.

Thanks,
Phil