* Bug in non-power-of-2 rings? @ 2023-08-20 9:07 Morten Brørup 2023-08-21 8:39 ` Bruce Richardson 0 siblings, 1 reply; 4+ messages in thread From: Morten Brørup @ 2023-08-20 9:07 UTC (permalink / raw) To: Bruce Richardson, honnappa.nagarahalli, konstantin.v.ananyev; +Cc: jerinj, dev 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Bug in non-power-of-2 rings? 2023-08-20 9:07 Bug in non-power-of-2 rings? Morten Brørup @ 2023-08-21 8:39 ` Bruce Richardson 2023-08-21 9:29 ` Konstantin Ananyev 0 siblings, 1 reply; 4+ messages in thread From: Bruce Richardson @ 2023-08-21 8:39 UTC (permalink / raw) To: Morten Brørup Cc: honnappa.nagarahalli, konstantin.v.ananyev, jerinj, dev 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: Bug in non-power-of-2 rings? 2023-08-21 8:39 ` Bruce Richardson @ 2023-08-21 9:29 ` Konstantin Ananyev 2023-08-21 9:36 ` Morten Brørup 0 siblings, 1 reply; 4+ messages in thread From: Konstantin Ananyev @ 2023-08-21 9:29 UTC (permalink / raw) To: Bruce Richardson, Morten Brørup Cc: honnappa.nagarahalli, konstantin.v.ananyev, jerinj, dev 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: Bug in non-power-of-2 rings? 2023-08-21 9:29 ` Konstantin Ananyev @ 2023-08-21 9:36 ` Morten Brørup 0 siblings, 0 replies; 4+ messages in thread From: Morten Brørup @ 2023-08-21 9:36 UTC (permalink / raw) To: Konstantin Ananyev, Bruce Richardson Cc: honnappa.nagarahalli, konstantin.v.ananyev, jerinj, dev, olivier.matz +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(). ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-08-21 9:36 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-08-20 9:07 Bug in non-power-of-2 rings? Morten Brørup 2023-08-21 8:39 ` Bruce Richardson 2023-08-21 9:29 ` Konstantin Ananyev 2023-08-21 9:36 ` Morten Brørup
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).