Re: [dpdk-dev] [PATCH v2 1/2] virtio: one way barrier for split vring used idx

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>]

<snip>
> > Subject: Re: [dpdk-dev] [PATCH v2 1/2] virtio: one way barrier for
> > split vring used idx
> >
> > On Thu,  2 Apr 2020 10:57:52 +0800
> > Joyce Kong <joyce.kong@arm.com> wrote:
> >
> > > -(vq)->vq_used_cons_idx))
> > > +static inline uint16_t
> > > +virtqueue_nused(struct virtqueue *vq)
> > vq is unmodified and should be const
> >
> > > +{
> > > +uint16_t idx;
> > > +if (vq->hw->weak_barriers) {
> > Put blank line between declaration and if statement
> Will fix in v3.
> >
> > > +/* x86 prefers to using rte_smp_rmb over __atomic_load_n as it
> > > +reports
> > > + * a slightly better perf, which comes from the saved branch by the
> > compiler.
> > > + * The if and else branches are identical with the smp and cio
> > > + barriers
> > both
> > > + * defined as compiler barriers on x86.
> > > + */
> >
> > Do not put comments on left margin (except in function prolog).
> Will fix in v3.
> >
> > > +#ifdef RTE_ARCH_X86_64
> > > +idx = vq->vq_split.ring.used->idx;
> > > +rte_smp_rmb();
> > > +#else
> > > +idx = __atomic_load_n(&(vq)->vq_split.ring.used->idx,
> > > +__ATOMIC_ACQUIRE);
> > > +#endif
> > > +} else {
> > > +idx = vq->vq_split.ring.used->idx;
> > > +rte_cio_rmb();
> > > +}
> > > +return (idx - vq->vq_used_cons_idx);
> >
> > Parenthesis around arguments to return are unnecessary.
> > BSD code likes it, Linux style does not.
> Will fix in v3.
> >
> > > +}
> >
> > This kind of arch specific code is hard to maintain.
> > Does it really make that much difference.
> Yes, a stronger than required barrier is a performance killer, especially in the
> fast path.
> The test was conducted on the ThunderX2+Intel XL710 testbed, the PVP test
> case.
I think if the performance deference is not much we should stay with C11 built-ins for x86. How much is the performance difference on x86?

> /Gavin