Re: [dpdk-dev] Ring PMD: why are stats counters atomic?

[Bruce Richardson <bruce.richardson@intel.com>]

On Mon, May 16, 2016 at 03:12:10PM +0200, Mauricio Vásquez wrote:
> Hello Bruce,
> 
> Although having this support does not harm anyone, I am not convinced that
> it is useful, mainly because there exists the single-thread limitation in
> other PMDs. Then, if an application has to use different kind of NICs (i.e,
> different PMDs) it has to implement the locking strategies. On the other
> hand, if an application  only uses rte_rings, it could just use the
> rte_ring library.
> 
> Thanks, Mauricio V
> 
I agree. 
If you want, please submit a patch to remove this behaviour and see 
if anyone objects to it. If there are no objections, I have no problem accepting
the patch.

However, since this is a behaviour change to existing functionality, we may
need to implement function versionning for this for ABI compatibility. Please
take that into account when drafting any patch.

Regards,
/Bruce

> On Tue, May 10, 2016 at 11:36 AM, Bruce Richardson <
> bruce.richardson@intel.com> wrote:
> 
> > On Tue, May 10, 2016 at 11:13:08AM +0200, Mauricio Vásquez wrote:
> > > Hello,
> > >
> > > Per-queue stats counters are defined as rte_atomic64_t, in the tx/rx
> > > functions, they are atomically increased if the rings have the multiple
> > > consumers/producer flag enabled.
> > >
> > > According to the design principles, the application should not invoke
> > those
> > > functions on the same queue on different cores, then I think that atomic
> > > increasing is not necessary.
> > >
> > > Is there something wrong with my reasoning?, If not, I am willing to
> > send a
> > > patch.
> > >
> > > Thank you very much,
> > >
> > Since the rte_rings, on which the ring pmd is obviously based, have
> > multi-producer
> > and multi-consumer support built-in, I thought it might be useful in the
> > ring
> > PMD itself to allow multiple threads to access the ring queues at the same
> > time,
> > if the underlying rings are marked as MP/MC safe. When doing enqueues and
> > dequeue
> > from the ring, the stats are either incremented atomically, or
> > non-atomically,
> > depending on the underlying queue type.
> >
> >         const uint16_t nb_rx = (uint16_t)rte_ring_dequeue_burst(r->rng,
> >                         ptrs, nb_bufs);
> >         if (r->rng->flags & RING_F_SC_DEQ)
> >                 r->rx_pkts.cnt += nb_rx;
> >         else
> >                 rte_atomic64_add(&(r->rx_pkts), nb_rx);
> >
> > If people don't think this behaviour is worthwhile keeping, I'm ok with
> > removing
> > it, since all other PMDs have the restriction that the queues are
> > single-thread
> > only.
> >
> > Regards,
> > /Bruce
> >