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

["Mauricio Vásquez" <mauricio.vasquezbernal@studenti.polito.it>]

Finally I have some time to have a look to it.

On Mon, May 16, 2016 at 3:16 PM, Bruce Richardson
<bruce.richardson@intel.com> wrote:
> 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.
>

Do you think that versioning is required in this case?
If anyone is using a functionality that is not supposed to work in
that way, should we care about it?

I am not against versioning, I just want to know if it is worthy to do.

> 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
>> >

Regards,

Mauricio V