| Message ID | 20200519152725.63486-3-mb@smartsharesystems.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Marchand |
| Headers | show |
| Series | ring: empty optimization | expand |
| Context | Check | Description |
|---|---|---|
| ci/checkpatch | success | coding style OK |
| ci/Intel-compilation | fail | Compilation issues |
On Tue, 19 May 2020 15:27:25 +0000 Morten Brørup <mb@smartsharesystems.com> wrote: > diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h > index 9078e7c24..f67141482 100644 > --- a/lib/librte_ring/rte_ring.h > +++ b/lib/librte_ring/rte_ring.h > @@ -733,7 +733,9 @@ rte_ring_full(const struct rte_ring *r) > static inline int > rte_ring_empty(const struct rte_ring *r) > { > - return rte_ring_count(r) == 0; > + uint32_t prod_tail = r->prod.tail; > + uint32_t cons_tail = r->cons.tail; > + return cons_tail == prod_tail; > } Blank line after declarations? Are the temporary variable even needed?
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger > Sent: Tuesday, May 19, 2020 5:52 PM > > On Tue, 19 May 2020 15:27:25 +0000 > Morten Brørup <mb@smartsharesystems.com> wrote: > > > diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h > > index 9078e7c24..f67141482 100644 > > --- a/lib/librte_ring/rte_ring.h > > +++ b/lib/librte_ring/rte_ring.h > > @@ -733,7 +733,9 @@ rte_ring_full(const struct rte_ring *r) > > static inline int > > rte_ring_empty(const struct rte_ring *r) > > { > > - return rte_ring_count(r) == 0; > > + uint32_t prod_tail = r->prod.tail; > > + uint32_t cons_tail = r->cons.tail; > > + return cons_tail == prod_tail; > > } > > Blank line after declarations? > > Are the temporary variable even needed? Personally, I agree with you, but I was trying to match the existing coding style of the closely related rte_ring_count() function - only to avoid this kind of feedback. Damn if you do, damn if you don't. :-)
> > Testing if the ring is empty is as simple as comparing the producer and > consumer pointers. > > In theory, this optimization reduces the number of potential cache misses > from 3 to 2 by not having to read r->mask in rte_ring_count(). > > The modification of this function were also discussed in the RFC here: > https://mails.dpdk.org/archives/dev/2020-April/165752.html > > Signed-off-by: Morten Brørup <mb@smartsharesystems.com> > --- > lib/librte_ring/rte_ring.h | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h > index 9078e7c24..f67141482 100644 > --- a/lib/librte_ring/rte_ring.h > +++ b/lib/librte_ring/rte_ring.h > @@ -733,7 +733,9 @@ rte_ring_full(const struct rte_ring *r) > static inline int > rte_ring_empty(const struct rte_ring *r) > { > - return rte_ring_count(r) == 0; > + uint32_t prod_tail = r->prod.tail; > + uint32_t cons_tail = r->cons.tail; > + return cons_tail == prod_tail; > } > > /** > -- Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com> > 2.17.1
On Tue, May 19, 2020 at 6:02 PM Morten Brørup <mb@smartsharesystems.com> wrote: > > Blank line after declarations? > > > > Are the temporary variable even needed? > > Personally, I agree with you, but I was trying to match the existing coding style of the closely related rte_ring_count() function - only to avoid this kind of feedback. > > Damn if you do, damn if you don't. :-) Yes, looking at the code, it seems fair taking this patch as is.
diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h index 9078e7c24..f67141482 100644 --- a/lib/librte_ring/rte_ring.h +++ b/lib/librte_ring/rte_ring.h @@ -733,7 +733,9 @@ rte_ring_full(const struct rte_ring *r) static inline int rte_ring_empty(const struct rte_ring *r) { - return rte_ring_count(r) == 0; + uint32_t prod_tail = r->prod.tail; + uint32_t cons_tail = r->cons.tail; + return cons_tail == prod_tail; } /**
Testing if the ring is empty is as simple as comparing the producer and consumer pointers. In theory, this optimization reduces the number of potential cache misses from 3 to 2 by not having to read r->mask in rte_ring_count(). The modification of this function were also discussed in the RFC here: https://mails.dpdk.org/archives/dev/2020-April/165752.html Signed-off-by: Morten Brørup <mb@smartsharesystems.com> --- lib/librte_ring/rte_ring.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)