Re: [dpdk-dev] [RFC,v2] lfring: lock-free ring buffer

["Eads, Gage" <gage.eads@intel.com>]

> -----Original Message-----
> From: Ola Liljedahl [mailto:Ola.Liljedahl@arm.com]
> Sent: Saturday, June 15, 2019 4:00 PM
> To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Richardson,
> Bruce <bruce.richardson@intel.com>; Eads, Gage <gage.eads@intel.com>;
> dev@dpdk.org
> Cc: nd <nd@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."