[dpdk-dev] [PATCH v1 1/2] lib/ring: add enqueue-dequeue callabck

[Ananyev, Konstantin]

> -----Original Message-----
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Stephen Hemminger
> Sent: Thursday, June 6, 2019 8:49 PM
> To: Varghese, Vipin <vipin.varghese at intel.com>
> Cc: olivier.matz at 6wind.com; Pattan, Reshma <reshma.pattan at intel.com>; Wiles, Keith <keith.wiles at intel.com>; dev at dpdk.org;
> Padubidri, Sanjay A <sanjay.padubidri at intel.com>
> Subject: Re: [dpdk-dev] [PATCH v1 1/2] lib/ring: add enqueue-dequeue callabck
> 
> On Fri,  7 Jun 2019 00:03:54 +0530
> Vipin Varghese <vipin.varghese at intel.com> wrote:
> 
> > Add callback event handler for enqueue dequeue operation on ring.
> > The pre-enqueue and post-dequeue operation on ring is selected to
> > invoke user callback handler.
> >
> > Signed-off-by: Vipin Varghese <vipin.varghese at intel.com>
> > ---
> 
> NAK
> What is the use case for this? Rings are one of the most used elements
> of DPDK and anything like this will have performance impact.
> 
> And as DPDK goes to more of distribution model, all features have to be
> enabled.

+1 here.
Approach in general looks quite expensive, most users wouldn't need it.
For those who want such functionality, it is not big deal to create a wrapper
on top of current rte_ring (drivers/net/ring could be one example).
Konstantin

> 
> 
> Also, this looks like a problem:
> +struct rte_ring_callback {
> +	TAILQ_ENTRY(rte_ring_callback) next; /* Callbacks list */
> +	rte_ring_cb_fn cb_fn; /* Callback address */
> +	void *cb_arg; /* Parameter for callback */
> +	uint32_t active; /* Callback is executing */
> 
> The active element is likely updated in one thread and queried in another
> it needs to be volatile, or better yet an atomic to ensure memory ordering.