> -----Original Message-----
> From: Ola Liljedahl [mailto:ola.liljed...@arm.com]
> Sent: Saturday, June 15, 2019 4:00 PM
> To: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>; Richardson,
> Bruce <bruce.richard...@intel.com>; Eads, Gage <gage.e...@intel.com>;
> dev@dpdk.org
> Cc: nd <n...@arm.com>
> Subject: Re: [RFC,v2] lfring: lock-free ring buffer
>
> On Wed, 2019-06-05 at 18:21 +0000, Eads, Gage wrote:
> > Hi Ola,
> >
> > Is it possible to add burst enqueue and dequeue functions as well, so
> > we can plug this into a mempool handler?
> Proper burst enqueue is difficult or at least not very efficient.
>
> > Besides the mempool handler, I think the all-or-nothing semantics
> > would be useful for applications. Besides that, this RFC looks good at a
> > high
> level.
> >
> > For a complete submission, a few more changes are needed:
> > - Builds: Need to add 'lfring' to lib/meson.build and mk/rte.app.mk
> > - Documentation: need to update release notes, add a new section in
> > the programmer's guide, and update the doxygen configuration files
> > - Tests: This patchset should add a set of lfring tests as well
> >
> > Code comments follow.
> Thanks for the review comments, I only had time to look at a few of them. I
> will return with more complete answers and a new version of the patch.
>
Sounds good.
<snip>
> > +/* search a ring from its name */
> > +struct rte_lfring *
> > +rte_lfring_lookup(const char *name)
> > +{
> > + struct rte_tailq_entry *te;
> > + struct rte_lfring *r = NULL;
> > + struct rte_lfring_list *ring_list;
> > +
> > + ring_list = RTE_TAILQ_CAST(rte_lfring_tailq.head, rte_lfring_list);
> > +
> > + rte_rwlock_read_lock(RTE_EAL_TAILQ_RWLOCK);
> > +
> > + TAILQ_FOREACH(te, ring_list, next) {
> > + r = (struct rte_lfring *) te->data;
> > + if (strncmp(name, r->name, RTE_LFRING_NAMESIZE) == 0)
> >
> > Missing a NULL pointer check before dereferencing 'name'
> Why shouldn't the program crash if someone passes a NULL pointer
> parameter?
> Callers will be internal, external users should be able to control whether
> NULL is passed instead of a valid pointer.
> A crash and a core dump is the best way to detect and debug errors.
If you think crashing is the appropriate response, rte_panic() with a
descriptive error string would be better than a segfault alone.
<snip>
> > +/**
> > + * Return the number of elements which can be stored in the lfring.
> > + *
> > + * @param r
> > + * A pointer to the lfring structure.
> > + * @return
> > + * The usable size of the lfring.
> > + */
> > +static inline unsigned int
> > +rte_lfring_get_capacity(const struct rte_lfring *r) {
> > + return r->size;
> >
> > I believe this should return r->mask, to account for the one unusable
> > ring entry.
>
> I think this is a mistake, all ring entries should be usable.
Ok, then do these comments from elsewhere in the header need to be corrected?
"The real usable lfring size is *count-1* instead of *count* to differentiate a
free lfring from an empty lfring."
