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

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

[Mauricio Vásquez]

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,

Mauricio V,

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

[Bruce Richardson]

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

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

[Mauricio Vásquez]

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

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
>

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

[Bruce Richardson]

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

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

[Mauricio Vásquez]

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