[RFC 0/1] ring: add callback infrastructire to ring library

[RFC 0/1] ring: add callback infrastructire to ring library

[Rory Sexton]

This is an RFC proposing the addition of a callback infrastructure to the ring
library, particularly in the ring dequeue functions, but they could also be
added to the enqueue functions if desired.

Callbacks in the ring dequeue functions would be beneficial for a number of
reasons including but not limited to the following:
- would allow users to register specific functions to be called on dequeue of a
  ring avoiding the need to call the function within application code on several
  threads reading from said ring.
- would mirror the callback functionality offered by the ethdev library for
  threads that read exclusively from a ring and process packets based off that,
  thus allowing for the same threads to read from either a NIC i/f or directly
  from a ring without needing a different codepath.

The addition of callbacks wouldn't impact the reading of rings by more than 1
cycle when no callbacks are registered. They could also additionally be compiled
in/out as desired to give more confidence in maintaining performance when
callbacks are not required.

This RFC is to give a feel for what the additional APIs would be and how they
would be integrated within the ring dequeue functions. As such only function
declarations are present. If there is a willingness within the community to add
callback infrastructure to the ring library I will implement further code.

Rory Sexton (1):
  ring: add infrastructure to allow callbacks within the ring library

 lib/ring/rte_ring.h      | 133 ++++++++++++++++++++++++++++++++++++++-
 lib/ring/rte_ring_core.h |   3 +
 2 files changed, 135 insertions(+), 1 deletion(-)

-- 
2.34.1

[RFC 1/1] ring: add infrastructure to allow callbacks within the ring library

[Rory Sexton]

Adding initial code to give a feel for what it would look like if
callbacks were supported by the ring dequeue functions within the
ring library. They could be optionally compiled in/out as required.

Signed-off-by: Rory Sexton <rory.sexton@intel.com>
---
 lib/ring/rte_ring.h      | 133 ++++++++++++++++++++++++++++++++++++++-
 lib/ring/rte_ring_core.h |   3 +
 2 files changed, 135 insertions(+), 1 deletion(-)

diff --git a/lib/ring/rte_ring.h b/lib/ring/rte_ring.h
index 7e4cd60650..9cda49ee51 100644
--- a/lib/ring/rte_ring.h
+++ b/lib/ring/rte_ring.h
@@ -789,6 +789,126 @@ rte_ring_sc_dequeue_burst(struct rte_ring *r, void **obj_table,
 			n, available);
 }
 
+#ifdef RTE_RING_DEQUEUE_CALLBACKS
+/**
+ * Function type used for ring dequeue callbacks.
+ *
+ * The callback function is called on dequeue with a burst of objects that have
+ * been dequeued on the given ring.
+ *
+ * @param r
+ *   A pointer to the ring structure.
+ * @param obj_table
+ *   A pointer to a table of void * pointers (objects) filled by the dequeue.
+ * @param n
+ *   The number of objects to dequeued from the ring to the obj_table.
+ * @param available
+ *   If non-NULL, returns the number of remaining ring entries after the
+ *   dequeue has finished.
+ * @param user_param
+ *   The arbitrary user parameter passed in by the application when the callback
+ *   was originally configured.
+ * @return
+ *   The number of packets returned to the user.
+ */
+typedef uint16_t (*rte_ring_dequeue_callback_fn)(struct rte_ring *r,
+		void **obj_table, unsigned int n, unsigned int *available,
+		void *user_param);
+
+/**
+ * @internal
+ * Structure used to hold information about the callbacks to be called for a
+ * ring on dequeue.
+ */
+struct rte_ring_dequeue_callback {
+	struct rte_ring_dequeue_callback *next;
+	rte_ring_dequeue_callback_fn fn;
+	void *param;
+};
+
+/**
+ * Add a callback to be called on dequeue of a given ring.
+ *
+ * This API configures a function to be called for each burst of
+ * objects dequeued from a given RING. The return value is a pointer
+ * that can be used to later remove the callback using
+ * rte_ring_remove_dequeue_callback().
+ *
+ * Multiple functions are called in the order that they are added.
+ *
+ * @param r
+ *   A pointer to the ring structure.
+ * @param fn
+ *   The callback function.
+ * @param user_param
+ *   A generic pointer parameter which will be passed to each invocation of the
+ *   callback function on this ring.
+ *
+ * @return
+ *   NULL on error.
+ *   On success, a pointer value which can later be used to remove the callback.
+ */
+const struct rte_ring_dequeue_callback *
+rte_ring_add_dequeue_callback(struct rte_ring *r,
+		rte_ring_dequeue_callback_fn fn, void *user_param);
+
+/**
+ * Remove a callback from a given ring.
+ *
+ * This function is used to removed callbacks that were added to a ring
+ * dequeue using rte_ring_add_dequeue_callback().
+ *
+ * Note: the callback is removed from the callback list but it isn't freed
+ * since the it may still be in use. The memory for the callback can be
+ * subsequently freed back by the application by calling rte_free():
+ *
+ * - Immediately - if the user knows that no callbacks are in flight
+ *   e.g. if called from the thread doing the dequeue on that ring.
+ *
+ * - After a short delay - where the delay is sufficient to allow any
+ *   in-flight callbacks to complete. Alternately, the RCU mechanism can be
+ *   used to detect when data plane threads have ceased referencing the
+ *   callback memory.
+ *
+ * @param r
+ *   A pointer to the ring structure.
+ * @param user_cb
+ *   User supplied callback created via rte_ring_add_dequeue_callback().
+ *
+ * @return
+ *   - 0: Success. Callback was removed.
+ *   - -ENODEV:  If *ring* is invalid.
+ *   - -ENOTSUP: Callback support is not available.
+ *   - -EINVAL:  The callback is NULL or not found for the ring.
+ */
+int rte_ring_remove_dequeue_callback(struct rte_ring *r,
+		const struct rte_ring_dequeue_callback *user_cb);
+
+/**
+ * @internal
+ * Helper routine for rte_ring_dequeue_burst().
+ * Invokes dequeue callbacks if any, etc.
+ *
+ * @param r
+ *   A pointer to the ring structure.
+ * @param obj_table
+ *   A pointer to a table of void * pointers (objects) filled by the dequeue.
+ * @param n
+ *   The number of objects to dequeued from the ring to the obj_table.
+ * @param available
+ *   If non-NULL, returns the number of remaining ring entries after the
+ *   dequeue has finished.
+ * @param opaque
+ *   Opaque pointer of ring dequeue callback related data.
+ *
+ * @return
+ *  The number of objects effectively supplied to the @p obj_table array.
+ */
+uint16_t rte_ring_call_dequeue_callbacks(struct rte_ring *r,
+		void **obj_table, unsigned int n, unsigned int *available,
+		void *opaque);
+#endif
+
 /**
  * Dequeue multiple objects from a ring up to a maximum number.
  *
@@ -812,8 +932,19 @@ static __rte_always_inline unsigned int
 rte_ring_dequeue_burst(struct rte_ring *r, void **obj_table,
 		unsigned int n, unsigned int *available)
 {
-	return rte_ring_dequeue_burst_elem(r, obj_table, sizeof(void *),
+	unsigned int nb_rx;
+	nb_rx = rte_ring_dequeue_burst_elem(r, obj_table, sizeof(void *),
 			n, available);
+
+#ifdef RTE_RING_DEQUEUE_CALLBACKS
+	{
+		if (unlikely(r->cb != NULL))
+			nb_rx = rte_ring_call_dequeue_callbacks(r, obj_table,
+					nb_rx, available, r->cb);
+	}
+#endif
+
+	return nb_rx;
 }
 
 #ifdef __cplusplus
diff --git a/lib/ring/rte_ring_core.h b/lib/ring/rte_ring_core.h
index 82b237091b..603e1b45f0 100644
--- a/lib/ring/rte_ring_core.h
+++ b/lib/ring/rte_ring_core.h
@@ -147,6 +147,9 @@ struct rte_ring {
 		struct rte_ring_rts_headtail rts_cons;
 	}  __rte_cache_aligned;
 
+	/** points to array of ring callback data pointers */
+	void **cb;
+
 	char pad2 __rte_cache_aligned; /**< empty cache line */
 };
 
-- 
2.34.1

RE: [RFC 0/1] ring: add callback infrastructire to ring library

[Morten Brørup]

> From: Rory Sexton [mailto:rory.sexton@intel.com]
> Sent: Thursday, 23 March 2023 12.38
> 
> This is an RFC proposing the addition of a callback infrastructure to the ring
> library, particularly in the ring dequeue functions, but they could also be
> added to the enqueue functions if desired.
> 
> Callbacks in the ring dequeue functions would be beneficial for a number of
> reasons including but not limited to the following:
> - would allow users to register specific functions to be called on dequeue of
> a
>   ring avoiding the need to call the function within application code on
> several
>   threads reading from said ring.
> - would mirror the callback functionality offered by the ethdev library for
>   threads that read exclusively from a ring and process packets based off
> that,
>   thus allowing for the same threads to read from either a NIC i/f or directly
>   from a ring without needing a different codepath.
> 
> The addition of callbacks wouldn't impact the reading of rings by more than 1
> cycle when no callbacks are registered. They could also additionally be
> compiled
> in/out as desired to give more confidence in maintaining performance when
> callbacks are not required.

On the condition that they can be omitted at build time, as you mention here, I don't mind having more callbacks.

> 
> This RFC is to give a feel for what the additional APIs would be and how they
> would be integrated within the ring dequeue functions. As such only function
> declarations are present. If there is a willingness within the community to
> add
> callback infrastructure to the ring library I will implement further code.

I like the idea that these callbacks mimic the ethdev callbacks, so if proceeding with this, I think that both enqueue and dequeue callbacks should be added. The ethdev callbacks share one RTE_ETHDEV_RXTX_CALLBACKS define, so the ring callbacks can probably do the same.

> 
> Rory Sexton (1):
>   ring: add infrastructure to allow callbacks within the ring library
> 
>  lib/ring/rte_ring.h      | 133 ++++++++++++++++++++++++++++++++++++++-
>  lib/ring/rte_ring_core.h |   3 +
>  2 files changed, 135 insertions(+), 1 deletion(-)
> 
> --
> 2.34.1
> 

RE: [RFC 0/1] ring: add callback infrastructire to ring library

[Honnappa Nagarahalli]

Hi Roxy,
	Thanks for the work, few questions inline.

> -----Original Message-----
> From: Rory Sexton <rory.sexton@intel.com>
> Sent: Thursday, March 23, 2023 6:38 AM
> To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>;
> konstantin.v.ananyev@yandex.ru
> Cc: dev@dpdk.org; Rory Sexton <rory.sexton@intel.com>
> Subject: [RFC 0/1] ring: add callback infrastructire to ring library
> 
> This is an RFC proposing the addition of a callback infrastructure to the ring
> library, particularly in the ring dequeue functions, but they could also be added
> to the enqueue functions if desired.
> 
> Callbacks in the ring dequeue functions would be beneficial for a number of
> reasons including but not limited to the following:
> - would allow users to register specific functions to be called on dequeue of a
>   ring avoiding the need to call the function within application code on several
>   threads reading from said ring.
I do not completely understand the 'avoiding the need to call the function within application code on several threads reading from said ring'. Irrespective of where the feature is implemented (either in ring library or the application), the call back function will be called on all the threads that receive from this queue. 

> - would mirror the callback functionality offered by the ethdev library for
>   threads that read exclusively from a ring and process packets based off that,
>   thus allowing for the same threads to read from either a NIC i/f or directly
>   from a ring without needing a different codepath.
Do you plan to support a chain of callbacks?

> 
> The addition of callbacks wouldn't impact the reading of rings by more than 1
> cycle when no callbacks are registered. They could also additionally be compiled
> in/out as desired to give more confidence in maintaining performance when
> callbacks are not required.
I would prefer to keep this feature on always as maintenance is easier. But, I think we should make that decision only after we have performance data. Is it possible to provide some performance data with this feature on but no callbacks registered?

> 
> This RFC is to give a feel for what the additional APIs would be and how they
> would be integrated within the ring dequeue functions. As such only function
> declarations are present. If there is a willingness within the community to add
> callback infrastructure to the ring library I will implement further code.
> 
> Rory Sexton (1):
>   ring: add infrastructure to allow callbacks within the ring library
> 
>  lib/ring/rte_ring.h      | 133 ++++++++++++++++++++++++++++++++++++++-
>  lib/ring/rte_ring_core.h |   3 +
>  2 files changed, 135 insertions(+), 1 deletion(-)
> 
> --
> 2.34.1

RE: [RFC 0/1] ring: add callback infrastructire to ring library

[Sexton, Rory]

Hi Honnappa,

Responses inline.
I will continue with an initial implementation of this feature and capture performance data as suggested. 

Rgds,
Rory

> -----Original Message-----
> From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com> 
> Sent: Wednesday, April 5, 2023 1:46 AM
> To: Sexton, Rory <rory.sexton@intel.com>; konstantin.v.ananyev@yandex.ru
> Cc: dev@dpdk.org; nd <nd@arm.com>; nd <nd@arm.com>
> Subject: RE: [RFC 0/1] ring: add callback infrastructire to ring library
> 
> Hi Roxy,
> 	Thanks for the work, few questions inline.
> 
> > -----Original Message-----
> > From: Rory Sexton <rory.sexton@intel.com>
> > Sent: Thursday, March 23, 2023 6:38 AM
> > To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; 
> > konstantin.v.ananyev@yandex.ru
> > Cc: dev@dpdk.org; Rory Sexton <rory.sexton@intel.com>
> > Subject: [RFC 0/1] ring: add callback infrastructire to ring library
> > 
> > This is an RFC proposing the addition of a callback infrastructure to 
> > the ring library, particularly in the ring dequeue functions, but they 
> > could also be added to the enqueue functions if desired.
> > 
> > Callbacks in the ring dequeue functions would be beneficial for a 
> > number of reasons including but not limited to the following:
> > - would allow users to register specific functions to be called on dequeue of a
> >   ring avoiding the need to call the function within application code on several
> >   threads reading from said ring.
> I do not completely understand the 'avoiding the need to call the function within application code on several threads reading from said ring'. Irrespective of where the feature is implemented (either in ring library or the application), the call back function will be called on all the threads that receive from this queue. 
I just mean the callback infrastructure would handle the call back function rather than developer needing to call their implemented function explicitely 

> 
> > - would mirror the callback functionality offered by the ethdev library for
> >   threads that read exclusively from a ring and process packets based off that,
> >   thus allowing for the same threads to read from either a NIC i/f or directly
> >   from a ring without needing a different codepath.
> Do you plan to support a chain of callbacks?
Yes I would plan to support a chain of callbacks 

> 
> > 
> > The addition of callbacks wouldn't impact the reading of rings by more 
> > than 1 cycle when no callbacks are registered. They could also 
> > additionally be compiled in/out as desired to give more confidence in 
> > maintaining performance when callbacks are not required.
> I would prefer to keep this feature on always as maintenance is easier. But, I think we should make that decision only after we have performance data. Is it possible to provide some performance data with this feature on but no callbacks registered?
Agreed that keeping the feature always on would be easier long-term.
I can capture performance data with the feature on but no callbacks registered.
We can base the final implementation with that data in mind. 

> 
> > 
> > This RFC is to give a feel for what the additional APIs would be and 
> > how they would be integrated within the ring dequeue functions. As 
> > such only function declarations are present. If there is a willingness 
> > within the community to add callback infrastructure to the ring library I will implement further code.
> > 
> > Rory Sexton (1):
> >   ring: add infrastructure to allow callbacks within the ring library
> > 
> >  lib/ring/rte_ring.h      | 133 ++++++++++++++++++++++++++++++++++++++-
> >  lib/ring/rte_ring_core.h |   3 +
> >  2 files changed, 135 insertions(+), 1 deletion(-)
> > 
> > --
> > 2.34.1

RE: [RFC 0/1] ring: add callback infrastructire to ring library

[Honnappa Nagarahalli]

<snip>

> 
> Hi Honnappa,
> 
> Responses inline.
> I will continue with an initial implementation of this feature and capture
> performance data as suggested.
> 
> Rgds,
> Rory
> 
> > -----Original Message-----
> > From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> > Sent: Wednesday, April 5, 2023 1:46 AM
> > To: Sexton, Rory <rory.sexton@intel.com>;
> > konstantin.v.ananyev@yandex.ru
> > Cc: dev@dpdk.org; nd <nd@arm.com>; nd <nd@arm.com>
> > Subject: RE: [RFC 0/1] ring: add callback infrastructire to ring
> > library
> >
> > Hi Roxy,
Sincere apologies for spelling your name incorrectly.

> > 	Thanks for the work, few questions inline.
> >
> > > -----Original Message-----
> > > From: Rory Sexton <rory.sexton@intel.com>
> > > Sent: Thursday, March 23, 2023 6:38 AM
> > > To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>;
> > > konstantin.v.ananyev@yandex.ru
> > > Cc: dev@dpdk.org; Rory Sexton <rory.sexton@intel.com>
> > > Subject: [RFC 0/1] ring: add callback infrastructire to ring library
> > >
> > > This is an RFC proposing the addition of a callback infrastructure
> > > to the ring library, particularly in the ring dequeue functions, but
> > > they could also be added to the enqueue functions if desired.
> > >
> > > Callbacks in the ring dequeue functions would be beneficial for a
> > > number of reasons including but not limited to the following:
> > > - would allow users to register specific functions to be called on dequeue
> of a
> > >   ring avoiding the need to call the function within application code on
> several
> > >   threads reading from said ring.
> > I do not completely understand the 'avoiding the need to call the function
> within application code on several threads reading from said ring'.
> Irrespective of where the feature is implemented (either in ring library or the
> application), the call back function will be called on all the threads that
> receive from this queue.
> I just mean the callback infrastructure would handle the call back function
> rather than developer needing to call their implemented function explicitly
Thanks for clarifying

> 
> >
<snip>

> 
> >
> > >
> > > The addition of callbacks wouldn't impact the reading of rings by
> > > more than 1 cycle when no callbacks are registered. They could also
> > > additionally be compiled in/out as desired to give more confidence
> > > in maintaining performance when callbacks are not required.
> > I would prefer to keep this feature on always as maintenance is easier. But, I
> think we should make that decision only after we have performance data. Is it
> possible to provide some performance data with this feature on but no
> callbacks registered?
> Agreed that keeping the feature always on would be easier long-term.
> I can capture performance data with the feature on but no callbacks
> registered.
> We can base the final implementation with that data in mind.
Ack

<snip>

Re: [RFC 1/1] ring: add infrastructure to allow callbacks within the ring library

[Stephen Hemminger]

On Thu, 23 Mar 2023 11:37:43 +0000
Rory Sexton <rory.sexton@intel.com> wrote:

> From: Rory Sexton <rory.sexton@intel.com>
> To: honnappa.nagarahalli@arm.com,  konstantin.v.ananyev@yandex.ru
> Cc: dev@dpdk.org,  Rory Sexton <rory.sexton@intel.com>
> Subject: [RFC 1/1] ring: add infrastructure to allow callbacks within the ring  library
> Date: Thu, 23 Mar 2023 11:37:43 +0000
> X-Mailer: git-send-email 2.34.1
> 
> Adding initial code to give a feel for what it would look like if
> callbacks were supported by the ring dequeue functions within the
> ring library. They could be optionally compiled in/out as required.
> 
> Signed-off-by: Rory Sexton <rory.sexton@intel.com>

Interesting but there is no current use for this.
Adding dead code, just creates more test cases and bloat to DPDK.
Could you use tracepoints for this instead.