Bug in non-power-of-2 rings?

Bug in non-power-of-2 rings?

[Morten Brørup]

Bruce, Honnappa, Konstantin,

Back in 2017, Bruce added support for non-power-of-2 rings with this patch [1].

[1]: https://git.dpdk.org/dpdk/commit/lib/librte_ring/rte_ring.h?id=b74461155543430f5253e96ad6d413ebcad36693

I think that the calculation of "entries" in __rte_ring_move_cons_head() [2][3] is incorrect when the ring capacity is not power-of-2, because it is missing the capacity comparison you added to rte_ring_count() [4]. Please review if I'm mistaken.

[2]: https://elixir.bootlin.com/dpdk/v23.07/source/lib/ring/rte_ring_c11_pvt.h#L159
[3]: https://elixir.bootlin.com/dpdk/v23.07/source/lib/ring/rte_ring_generic_pvt.h#L150
[4]: https://elixir.bootlin.com/dpdk/v23.07/source/lib/ring/rte_ring.h#L502

-Morten

Re: Bug in non-power-of-2 rings?

[Bruce Richardson]

On Sun, Aug 20, 2023 at 11:07:33AM +0200, Morten Brørup wrote:
> Bruce, Honnappa, Konstantin,
> 
> Back in 2017, Bruce added support for non-power-of-2 rings with this patch [1].
> 
> [1]: https://git.dpdk.org/dpdk/commit/lib/librte_ring/rte_ring.h?id=b74461155543430f5253e96ad6d413ebcad36693
> 
> I think that the calculation of "entries" in __rte_ring_move_cons_head() [2][3] is incorrect when the ring capacity is not power-of-2, because it is missing the capacity comparison you added to rte_ring_count() [4]. Please review if I'm mistaken.
> 
> [2]: https://elixir.bootlin.com/dpdk/v23.07/source/lib/ring/rte_ring_c11_pvt.h#L159
> [3]: https://elixir.bootlin.com/dpdk/v23.07/source/lib/ring/rte_ring_generic_pvt.h#L150
> [4]: https://elixir.bootlin.com/dpdk/v23.07/source/lib/ring/rte_ring.h#L502
> 
thanks for flagging this inconsistency, but I think we are ok.

For consumer, I think this is correct, because we are only ever reducing
the number of entries in the ring, and the calculation of the number of
entries is made in the usual way using modulo arithmetic. We should never
have more than capacity entries in the ring so the check in ring count I
believe is superflous. [The exception would be if someone bypassed the
inline functions and accessed the ring directly themselves - at which point
"all bets are off", to use the English phrase]

The producer code (__rte_ring_move_prod_head) does do a capacity check,
which is where one is required to ensure we never exceed capacity.

/Bruce

RE: Bug in non-power-of-2 rings?

[Konstantin Ananyev]

Hi everyone,

> On Sun, Aug 20, 2023 at 11:07:33AM +0200, Morten Brørup wrote:
> > Bruce, Honnappa, Konstantin,
> >
> > Back in 2017, Bruce added support for non-power-of-2 rings with this patch [1].
> >
> > [1]: https://git.dpdk.org/dpdk/commit/lib/librte_ring/rte_ring.h?id=b74461155543430f5253e96ad6d413ebcad36693
> >
> > I think that the calculation of "entries" in __rte_ring_move_cons_head() [2][3] is incorrect when the ring capacity is not power-of-2,
> because it is missing the capacity comparison you added to rte_ring_count() [4]. Please review if I'm mistaken.
> >
> > [2]: https://elixir.bootlin.com/dpdk/v23.07/source/lib/ring/rte_ring_c11_pvt.h#L159
> > [3]: https://elixir.bootlin.com/dpdk/v23.07/source/lib/ring/rte_ring_generic_pvt.h#L150
> > [4]: https://elixir.bootlin.com/dpdk/v23.07/source/lib/ring/rte_ring.h#L502

Just to confirm you suggest something like that:
-	*entries = (r->prod.tail - *old_head);
+	count = (r->prod.tail - *old_head);
+ 	entries = (count > r->capacity) ? r->capacity : count;
 right?

> >
> thanks for flagging this inconsistency, but I think we are ok.
> 
> For consumer, I think this is correct, because we are only ever reducing
> the number of entries in the ring, and the calculation of the number of
> entries is made in the usual way using modulo arithmetic. We should never
> have more than capacity entries in the ring so the check in ring count I
> believe is superflous. [The exception would be if someone bypassed the
> inline functions and accessed the ring directly themselves - at which point
> "all bets are off", to use the English phrase]
> 
> The producer code (__rte_ring_move_prod_head) does do a capacity check,
> which is where one is required to ensure we never exceed capacity.

I also can't come up with the case, when current code will cause an issue..
In properly operating ring, I think we should never  have more then r->capacity 
entries populated, so this extra check can be skipped.
Unless you do have some particular case in mind?
Konstantin  

RE: Bug in non-power-of-2 rings?

[Morten Brørup]

+CC Olivier, referring to your review of the original patch.

> From: Konstantin Ananyev [mailto:konstantin.ananyev@huawei.com]
> Sent: Monday, 21 August 2023 11.29
> 
> Hi everyone,
> 
> > On Sun, Aug 20, 2023 at 11:07:33AM +0200, Morten Brørup wrote:
> > > Bruce, Honnappa, Konstantin,
> > >
> > > Back in 2017, Bruce added support for non-power-of-2 rings with this
> patch [1].
> > >
> > > [1]:
> https://git.dpdk.org/dpdk/commit/lib/librte_ring/rte_ring.h?id=b74461155
> 543430f5253e96ad6d413ebcad36693
> > >
> > > I think that the calculation of "entries" in
> __rte_ring_move_cons_head() [2][3] is incorrect when the ring capacity
> is not power-of-2,
> > because it is missing the capacity comparison you added to
> rte_ring_count() [4]. Please review if I'm mistaken.
> > >
> > > [2]:
> https://elixir.bootlin.com/dpdk/v23.07/source/lib/ring/rte_ring_c11_pvt.
> h#L159
> > > [3]:
> https://elixir.bootlin.com/dpdk/v23.07/source/lib/ring/rte_ring_generic_
> pvt.h#L150
> > > [4]:
> https://elixir.bootlin.com/dpdk/v23.07/source/lib/ring/rte_ring.h#L502
> 
> Just to confirm you suggest something like that:
> -	*entries = (r->prod.tail - *old_head);
> +	count = (r->prod.tail - *old_head);
> + 	entries = (count > r->capacity) ? r->capacity : count;
>  right?

Yes, since rte_ring_count() does it, it might be required here too.

> 
> > >
> > thanks for flagging this inconsistency, but I think we are ok.
> >
> > For consumer, I think this is correct, because we are only ever
> reducing
> > the number of entries in the ring, and the calculation of the number
> of
> > entries is made in the usual way using modulo arithmetic. We should
> never
> > have more than capacity entries in the ring so the check in ring count
> I
> > believe is superflous. [The exception would be if someone bypassed the
> > inline functions and accessed the ring directly themselves - at which
> point
> > "all bets are off", to use the English phrase]

I have now found the comments to the original patch [5]. It seems that Olivier flagged this as a risk for rte_ring_free_count(), which reads r->prod.tail and r->cons.tail without synchronization.

However, since __rte_ring_move_cons_head() uses synchronization, I guess that such a risk is not present here.

[5]: https://patchwork.dpdk.org/project/dpdk/patch/20170607133620.275801-2-bruce.richardson@intel.com/

> >
> > The producer code (__rte_ring_move_prod_head) does do a capacity
> check,
> > which is where one is required to ensure we never exceed capacity.

Agreed. Thanks for double checking.

> 
> I also can't come up with the case, when current code will cause an
> issue..
> In properly operating ring, I think we should never  have more then r-
> >capacity
> entries populated, so this extra check can be skipped.
> Unless you do have some particular case in mind?

No special case in mind. Just stumbled over this code looking different than similar code in rte_ring_free_count().