[dpdk-dev] [PATCH] ethdev: additional parameter in RX callback

[dpdk-dev] [PATCH] ethdev: additional parameter in RX callback

[John McNamara]

This patch is a minor extension to the recent patchset for RX/TX callbacks
based on feedback from users implementing solutions based on it.

The patch adds a new parameter to the RX callback to pass in the number of
available RX packets in addition to the number of dequeued packets.
This provides the RX callback functions with additional information
that can be used to decide how packets from a burst are handled.

The TX callback doesn't require this additional parameter so the RX
and TX callbacks no longer have the same function parameters. As such
the single RX/TX callback has been refactored into two separate callbacks.

Since this is an API change we hope it can be included in 2.0.0 to avoid
changing the API in a subsequent release.    


John McNamara (1):
  ethdev: added additional packet count parameter to RX callbacks

 examples/rxtx_callbacks/main.c |    3 +-
 lib/librte_ether/rte_ethdev.c  |    8 ++--
 lib/librte_ether/rte_ethdev.h  |   74 ++++++++++++++++++++++++++--------------
 3 files changed, 54 insertions(+), 31 deletions(-)

-- 
1.7.4.1

[dpdk-dev] [PATCH] ethdev: added additional packet count parameter to RX callbacks

[John McNamara]

Added a parameter to the RX callback to pass in the number of
available RX packets in addition to the number of dequeued packets.
This provides the RX callback functions with additional information
that can be used to decide how packets from a burst are handled.

The TX callback doesn't require this additional parameter so the RX
and TX callbacks no longer have the same function parameters. As such
the single RX/TX callback has been refactored into two separate callbacks.

Signed-off-by: John McNamara <john.mcnamara@intel.com>
---
 examples/rxtx_callbacks/main.c |    3 +-
 lib/librte_ether/rte_ethdev.c  |    8 ++--
 lib/librte_ether/rte_ethdev.h  |   74 ++++++++++++++++++++++++++--------------
 3 files changed, 54 insertions(+), 31 deletions(-)

diff --git a/examples/rxtx_callbacks/main.c b/examples/rxtx_callbacks/main.c
index 9e5e68e..21117ce 100644
--- a/examples/rxtx_callbacks/main.c
+++ b/examples/rxtx_callbacks/main.c
@@ -61,7 +61,8 @@ static struct {
 
 static uint16_t
 add_timestamps(uint8_t port __rte_unused, uint16_t qidx __rte_unused,
-		struct rte_mbuf **pkts, uint16_t nb_pkts, void *_ __rte_unused)
+		struct rte_mbuf **pkts, uint16_t nb_pkts,
+		uint16_t max_pkts __rte_unused, void *_ __rte_unused)
 {
 	unsigned i;
 	uint64_t now = rte_rdtsc();
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 03fce08..e142920 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -3533,7 +3533,7 @@ rte_eth_dev_filter_ctrl(uint8_t port_id, enum rte_filter_type filter_type,
 
 void *
 rte_eth_add_rx_callback(uint8_t port_id, uint16_t queue_id,
-		rte_rxtx_callback_fn fn, void *user_param)
+		rte_rx_callback_fn fn, void *user_param)
 {
 #ifndef RTE_ETHDEV_RXTX_CALLBACKS
 	rte_errno = ENOTSUP;
@@ -3553,7 +3553,7 @@ rte_eth_add_rx_callback(uint8_t port_id, uint16_t queue_id,
 		return NULL;
 	}
 
-	cb->fn = fn;
+	cb->fn.rx = fn;
 	cb->param = user_param;
 	cb->next = rte_eth_devices[port_id].post_rx_burst_cbs[queue_id];
 	rte_eth_devices[port_id].post_rx_burst_cbs[queue_id] = cb;
@@ -3562,7 +3562,7 @@ rte_eth_add_rx_callback(uint8_t port_id, uint16_t queue_id,
 
 void *
 rte_eth_add_tx_callback(uint8_t port_id, uint16_t queue_id,
-		rte_rxtx_callback_fn fn, void *user_param)
+		rte_tx_callback_fn fn, void *user_param)
 {
 #ifndef RTE_ETHDEV_RXTX_CALLBACKS
 	rte_errno = ENOTSUP;
@@ -3582,7 +3582,7 @@ rte_eth_add_tx_callback(uint8_t port_id, uint16_t queue_id,
 		return NULL;
 	}
 
-	cb->fn = fn;
+	cb->fn.tx = fn;
 	cb->param = user_param;
 	cb->next = rte_eth_devices[port_id].pre_tx_burst_cbs[queue_id];
 	rte_eth_devices[port_id].pre_tx_burst_cbs[queue_id] = cb;
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 21aa359..e0b0b8d 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1384,33 +1384,52 @@ struct eth_dev_ops {
 };
 
 /**
- * Function type used for callbacks for processing packets on RX and TX
+ * Function type used for RX packet processing packet callbacks.
  *
- * If configured for RX, it is called with a burst of packets that have just
- * been received on the given port and queue. On TX, it is called with a burst
- * of packets immediately before those packets are put onto the hardware queue
- * for transmission.
+ * The callback function is called on RX with a burst of packets that have
+ * been received on the given port and queue.
  *
  * @param port
- *   The ethernet port on which rx or tx is being performed
+ *   The Ethernet port on which RX is being performed.
  * @param queue
- *   The queue on the ethernet port which is being used to receive or transmit
- *   the packets.
+ *   The queue on the Ethernet port which is being used to receive the packets.
  * @param pkts
- *   The burst of packets on which processing is to be done. On RX, these
- *   packets have just been received. On TX, they are about to be transmitted.
+ *   The burst of packets that have just been received.
  * @param nb_pkts
- *   The number of packets in the burst pointed to by "pkts"
+ *   The number of packets in the burst pointed to by "pkts".
+ * @param max_pkts
+ *   The max number of packets that can be stored in the "pkts" array.
  * @param user_param
  *   The arbitrary user parameter passed in by the application when the callback
  *   was originally configured.
  * @return
- *   The number of packets remaining in pkts are processing.
- *	* On RX, this will be returned to the user as the return value from
- *	  rte_eth_rx_burst.
- *	* On TX, this will be the number of packets actually written to the NIC.
+ *   The number of packets returned to the user.
  */
-typedef uint16_t (*rte_rxtx_callback_fn)(uint8_t port, uint16_t queue,
+typedef uint16_t (*rte_rx_callback_fn)(uint8_t port, uint16_t queue,
+	struct rte_mbuf *pkts[], uint16_t nb_pkts, uint16_t max_pkts,
+	void *user_param);
+
+/**
+ * Function type used for TX packet processing packet callbacks.
+ *
+ * The callback function is called on TX with a burst of packets immediately
+ * before the packets are put onto the hardware queue for transmission.
+ *
+ * @param port
+ *   The Ethernet port on which TX is being performed.
+ * @param queue
+ *   The queue on the Ethernet port which is being used to transmit the packets.
+ * @param pkts
+ *   The burst of packets that are about to be transmitted.
+ * @param nb_pkts
+ *   The number of packets in the burst pointed to by "pkts".
+ * @param user_param
+ *   The arbitrary user parameter passed in by the application when the callback
+ *   was originally configured.
+ * @return
+ *   The number of packets to be written to the NIC.
+ */
+typedef uint16_t (*rte_tx_callback_fn)(uint8_t port, uint16_t queue,
 	struct rte_mbuf *pkts[], uint16_t nb_pkts, void *user_param);
 
 /**
@@ -1420,7 +1439,10 @@ typedef uint16_t (*rte_rxtx_callback_fn)(uint8_t port, uint16_t queue,
  */
 struct rte_eth_rxtx_callback {
 	struct rte_eth_rxtx_callback *next;
-	rte_rxtx_callback_fn fn;
+	union{
+		rte_rx_callback_fn rx;
+		rte_tx_callback_fn tx;
+	} fn;
 	void *param;
 };
 
@@ -2386,28 +2408,28 @@ extern uint16_t rte_eth_rx_burst(uint8_t port_id, uint16_t queue_id,
 #else
 static inline uint16_t
 rte_eth_rx_burst(uint8_t port_id, uint16_t queue_id,
-		 struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
+		 struct rte_mbuf **rx_pkts, const uint16_t nb_pkts)
 {
 	struct rte_eth_dev *dev;
 
 	dev = &rte_eth_devices[port_id];
 
-	nb_pkts = (*dev->rx_pkt_burst)(dev->data->rx_queues[queue_id], rx_pkts,
-			nb_pkts);
+	int16_t nb_rx = (*dev->rx_pkt_burst)(dev->data->rx_queues[queue_id],
+			rx_pkts, nb_pkts);
 
 #ifdef RTE_ETHDEV_RXTX_CALLBACKS
 	struct rte_eth_rxtx_callback *cb = dev->post_rx_burst_cbs[queue_id];
 
 	if (unlikely(cb != NULL)) {
 		do {
-			nb_pkts = cb->fn(port_id, queue_id, rx_pkts, nb_pkts,
-					cb->param);
+			nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
+						nb_pkts, cb->param);
 			cb = cb->next;
 		} while (cb != NULL);
 	}
 #endif
 
-	return nb_pkts;
+	return nb_rx;
 }
 #endif
 
@@ -2540,7 +2562,7 @@ rte_eth_tx_burst(uint8_t port_id, uint16_t queue_id,
 
 	if (unlikely(cb != NULL)) {
 		do {
-			nb_pkts = cb->fn(port_id, queue_id, tx_pkts, nb_pkts,
+			nb_pkts = cb->fn.tx(port_id, queue_id, tx_pkts, nb_pkts,
 					cb->param);
 			cb = cb->next;
 		} while (cb != NULL);
@@ -3490,7 +3512,7 @@ int rte_eth_dev_filter_ctrl(uint8_t port_id, enum rte_filter_type filter_type,
  *   On success, a pointer value which can later be used to remove the callback.
  */
 void *rte_eth_add_rx_callback(uint8_t port_id, uint16_t queue_id,
-		rte_rxtx_callback_fn fn, void *user_param);
+		rte_rx_callback_fn fn, void *user_param);
 
 /**
  * Add a callback to be called on packet TX on a given port and queue.
@@ -3515,7 +3537,7 @@ void *rte_eth_add_rx_callback(uint8_t port_id, uint16_t queue_id,
  *   On success, a pointer value which can later be used to remove the callback.
  */
 void *rte_eth_add_tx_callback(uint8_t port_id, uint16_t queue_id,
-		rte_rxtx_callback_fn fn, void *user_param);
+		rte_tx_callback_fn fn, void *user_param);
 
 /**
  * Remove an RX packet callback from a given port and queue.
-- 
1.7.4.1

Re: [dpdk-dev] [PATCH] ethdev: additional parameter in RX callback

[Neil Horman]

On Thu, Mar 12, 2015 at 04:54:27PM +0000, John McNamara wrote:
> 
> This patch is a minor extension to the recent patchset for RX/TX callbacks
> based on feedback from users implementing solutions based on it.
> 
> The patch adds a new parameter to the RX callback to pass in the number of
> available RX packets in addition to the number of dequeued packets.
> This provides the RX callback functions with additional information
> that can be used to decide how packets from a burst are handled.
> 
> The TX callback doesn't require this additional parameter so the RX
> and TX callbacks no longer have the same function parameters. As such
> the single RX/TX callback has been refactored into two separate callbacks.
> 
> Since this is an API change we hope it can be included in 2.0.0 to avoid
> changing the API in a subsequent release.    
> 
> 
> John McNamara (1):
>   ethdev: added additional packet count parameter to RX callbacks
> 
>  examples/rxtx_callbacks/main.c |    3 +-
>  lib/librte_ether/rte_ethdev.c  |    8 ++--
>  lib/librte_ether/rte_ethdev.h  |   74 ++++++++++++++++++++++++++--------------
>  3 files changed, 54 insertions(+), 31 deletions(-)
> 
> -- 
> 1.7.4.1
> 
> 


Well, we're well past the new feature phase of this cycle, so I would say NACK.
I would also suggest that you don't need to modify ABI to accomodate this
feature.  Instead just document the pkts array to be terminated by a reserved
value, so that the callback can determine its size dynamically.  You could
alternatively create a new api call that allows you to retrieve that information
from the context of the callback.

Neil

Re: [dpdk-dev] [PATCH] ethdev: additional parameter in RX callback

[Mcnamara, John]

> -----Original Message-----
> From: Neil Horman [mailto:nhorman@tuxdriver.com]
> Sent: Thursday, March 12, 2015 7:16 PM
> To: Mcnamara, John
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] ethdev: additional parameter in RX
> callback
> 
> 
> Well, we're well past the new feature phase of this cycle, so I would say
> NACK.

Hi Neil,

This is more of a bug fix than a feature request.

In retrospect it was a mistake not to have the same function declaration in the callbacks as in the rte_eth_XX_burst functions since it was likely that the callbacks would need access to the same information.

This patch is trying to fix that error before it is set in stone.

John

Re: [dpdk-dev] [PATCH] ethdev: additional parameter in RX callback

[Bruce Richardson]

On Thu, Mar 12, 2015 at 03:15:40PM -0400, Neil Horman wrote:
> On Thu, Mar 12, 2015 at 04:54:27PM +0000, John McNamara wrote:
> > 
> > This patch is a minor extension to the recent patchset for RX/TX callbacks
> > based on feedback from users implementing solutions based on it.
> > 
> > The patch adds a new parameter to the RX callback to pass in the number of
> > available RX packets in addition to the number of dequeued packets.
> > This provides the RX callback functions with additional information
> > that can be used to decide how packets from a burst are handled.
> > 
> > The TX callback doesn't require this additional parameter so the RX
> > and TX callbacks no longer have the same function parameters. As such
> > the single RX/TX callback has been refactored into two separate callbacks.
> > 
> > Since this is an API change we hope it can be included in 2.0.0 to avoid
> > changing the API in a subsequent release.    
> > 
> > 
> > John McNamara (1):
> >   ethdev: added additional packet count parameter to RX callbacks
> > 
> >  examples/rxtx_callbacks/main.c |    3 +-
> >  lib/librte_ether/rte_ethdev.c  |    8 ++--
> >  lib/librte_ether/rte_ethdev.h  |   74 ++++++++++++++++++++++++++--------------
> >  3 files changed, 54 insertions(+), 31 deletions(-)
> > 
> > -- 
> > 1.7.4.1
> > 
> > 
> 
> 
> Well, we're well past the new feature phase of this cycle, so I would say NACK.
> I would also suggest that you don't need to modify ABI to accomodate this
> feature.  Instead just document the pkts array to be terminated by a reserved
> value, so that the callback can determine its size dynamically.  You could
> alternatively create a new api call that allows you to retrieve that information
> from the context of the callback.
> 
> Neil
> 

Yes, I would agree we are past the new feature phase. However, given that we
are making a change to the API, and a fairly small change too - adding one extra
parameter - we think that the benefit of including this now outweighs any risk
of merging the patch. It seems a bit crazy to ship a release with a new API and
then immediately change the API straight after release. Is it not better to
take the received feedback on the API and fix/improve it pre-release before it
gets set-in-stone?

/Bruce

Re: [dpdk-dev] [PATCH] ethdev: additional parameter in RX callback

[Neil Horman]

On Fri, Mar 13, 2015 at 09:41:33AM +0000, Bruce Richardson wrote:
> On Thu, Mar 12, 2015 at 03:15:40PM -0400, Neil Horman wrote:
> > On Thu, Mar 12, 2015 at 04:54:27PM +0000, John McNamara wrote:
> > > 
> > > This patch is a minor extension to the recent patchset for RX/TX callbacks
> > > based on feedback from users implementing solutions based on it.
> > > 
> > > The patch adds a new parameter to the RX callback to pass in the number of
> > > available RX packets in addition to the number of dequeued packets.
> > > This provides the RX callback functions with additional information
> > > that can be used to decide how packets from a burst are handled.
> > > 
> > > The TX callback doesn't require this additional parameter so the RX
> > > and TX callbacks no longer have the same function parameters. As such
> > > the single RX/TX callback has been refactored into two separate callbacks.
> > > 
> > > Since this is an API change we hope it can be included in 2.0.0 to avoid
> > > changing the API in a subsequent release.    
> > > 
> > > 
> > > John McNamara (1):
> > >   ethdev: added additional packet count parameter to RX callbacks
> > > 
> > >  examples/rxtx_callbacks/main.c |    3 +-
> > >  lib/librte_ether/rte_ethdev.c  |    8 ++--
> > >  lib/librte_ether/rte_ethdev.h  |   74 ++++++++++++++++++++++++++--------------
> > >  3 files changed, 54 insertions(+), 31 deletions(-)
> > > 
> > > -- 
> > > 1.7.4.1
> > > 
> > > 
> > 
> > 
> > Well, we're well past the new feature phase of this cycle, so I would say NACK.
> > I would also suggest that you don't need to modify ABI to accomodate this
> > feature.  Instead just document the pkts array to be terminated by a reserved
> > value, so that the callback can determine its size dynamically.  You could
> > alternatively create a new api call that allows you to retrieve that information
> > from the context of the callback.
> > 
> > Neil
> > 
> 
> Yes, I would agree we are past the new feature phase. However, given that we
> are making a change to the API, and a fairly small change too - adding one extra
> parameter - we think that the benefit of including this now outweighs any risk
> of merging the patch. It seems a bit crazy to ship a release with a new API and
> then immediately change the API straight after release. Is it not better to
> take the received feedback on the API and fix/improve it pre-release before it
> gets set-in-stone?
> 
> /Bruce
> 
> 

See above, the API doesn't need to change at all to accomodate this as far as I
can see.

Neil

Re: [dpdk-dev] [PATCH] ethdev: additional parameter in RX callback

[Bruce Richardson]

On Fri, Mar 13, 2015 at 09:45:14AM -0400, Neil Horman wrote:
> On Fri, Mar 13, 2015 at 09:41:33AM +0000, Bruce Richardson wrote:
> > On Thu, Mar 12, 2015 at 03:15:40PM -0400, Neil Horman wrote:
> > > On Thu, Mar 12, 2015 at 04:54:27PM +0000, John McNamara wrote:
> > > > 
> > > > This patch is a minor extension to the recent patchset for RX/TX callbacks
> > > > based on feedback from users implementing solutions based on it.
> > > > 
> > > > The patch adds a new parameter to the RX callback to pass in the number of
> > > > available RX packets in addition to the number of dequeued packets.
> > > > This provides the RX callback functions with additional information
> > > > that can be used to decide how packets from a burst are handled.
> > > > 
> > > > The TX callback doesn't require this additional parameter so the RX
> > > > and TX callbacks no longer have the same function parameters. As such
> > > > the single RX/TX callback has been refactored into two separate callbacks.
> > > > 
> > > > Since this is an API change we hope it can be included in 2.0.0 to avoid
> > > > changing the API in a subsequent release.    
> > > > 
> > > > 
> > > > John McNamara (1):
> > > >   ethdev: added additional packet count parameter to RX callbacks
> > > > 
> > > >  examples/rxtx_callbacks/main.c |    3 +-
> > > >  lib/librte_ether/rte_ethdev.c  |    8 ++--
> > > >  lib/librte_ether/rte_ethdev.h  |   74 ++++++++++++++++++++++++++--------------
> > > >  3 files changed, 54 insertions(+), 31 deletions(-)
> > > > 
> > > > -- 
> > > > 1.7.4.1
> > > > 
> > > > 
> > > 
> > > 
> > > Well, we're well past the new feature phase of this cycle, so I would say NACK.
> > > I would also suggest that you don't need to modify ABI to accomodate this
> > > feature.  Instead just document the pkts array to be terminated by a reserved
> > > value, so that the callback can determine its size dynamically.  You could
> > > alternatively create a new api call that allows you to retrieve that information
> > > from the context of the callback.
> > > 
> > > Neil
> > > 
> > 
> > Yes, I would agree we are past the new feature phase. However, given that we
> > are making a change to the API, and a fairly small change too - adding one extra
> > parameter - we think that the benefit of including this now outweighs any risk
> > of merging the patch. It seems a bit crazy to ship a release with a new API and
> > then immediately change the API straight after release. Is it not better to
> > take the received feedback on the API and fix/improve it pre-release before it
> > gets set-in-stone?
> > 
> > /Bruce
> > 
> > 
> 
> See above, the API doesn't need to change at all to accomodate this as far as I
> can see.
> 
> Neil
Yes, there are alternative ways to see about accomplishing the same thing, but
they are not nearly as clear as adding in the extra param. That's why we'd like
to see this change go in before release, if possible. If it doesn't make 2.0
I'd like to see it in 2.1, but at the cost of having an API change and the
additional versionning and deprecation that ensues.

Regards,
/Bruce

Re: [dpdk-dev] [PATCH] ethdev: additional parameter in RX callback

[Neil Horman]

On Fri, Mar 13, 2015 at 02:50:03PM +0000, Bruce Richardson wrote:
> On Fri, Mar 13, 2015 at 09:45:14AM -0400, Neil Horman wrote:
> > On Fri, Mar 13, 2015 at 09:41:33AM +0000, Bruce Richardson wrote:
> > > On Thu, Mar 12, 2015 at 03:15:40PM -0400, Neil Horman wrote:
> > > > On Thu, Mar 12, 2015 at 04:54:27PM +0000, John McNamara wrote:
> > > > > 
> > > > > This patch is a minor extension to the recent patchset for RX/TX callbacks
> > > > > based on feedback from users implementing solutions based on it.
> > > > > 
> > > > > The patch adds a new parameter to the RX callback to pass in the number of
> > > > > available RX packets in addition to the number of dequeued packets.
> > > > > This provides the RX callback functions with additional information
> > > > > that can be used to decide how packets from a burst are handled.
> > > > > 
> > > > > The TX callback doesn't require this additional parameter so the RX
> > > > > and TX callbacks no longer have the same function parameters. As such
> > > > > the single RX/TX callback has been refactored into two separate callbacks.
> > > > > 
> > > > > Since this is an API change we hope it can be included in 2.0.0 to avoid
> > > > > changing the API in a subsequent release.    
> > > > > 
> > > > > 
> > > > > John McNamara (1):
> > > > >   ethdev: added additional packet count parameter to RX callbacks
> > > > > 
> > > > >  examples/rxtx_callbacks/main.c |    3 +-
> > > > >  lib/librte_ether/rte_ethdev.c  |    8 ++--
> > > > >  lib/librte_ether/rte_ethdev.h  |   74 ++++++++++++++++++++++++++--------------
> > > > >  3 files changed, 54 insertions(+), 31 deletions(-)
> > > > > 
> > > > > -- 
> > > > > 1.7.4.1
> > > > > 
> > > > > 
> > > > 
> > > > 
> > > > Well, we're well past the new feature phase of this cycle, so I would say NACK.
> > > > I would also suggest that you don't need to modify ABI to accomodate this
> > > > feature.  Instead just document the pkts array to be terminated by a reserved
> > > > value, so that the callback can determine its size dynamically.  You could
> > > > alternatively create a new api call that allows you to retrieve that information
> > > > from the context of the callback.
> > > > 
> > > > Neil
> > > > 
> > > 
> > > Yes, I would agree we are past the new feature phase. However, given that we
> > > are making a change to the API, and a fairly small change too - adding one extra
> > > parameter - we think that the benefit of including this now outweighs any risk
> > > of merging the patch. It seems a bit crazy to ship a release with a new API and
> > > then immediately change the API straight after release. Is it not better to
> > > take the received feedback on the API and fix/improve it pre-release before it
> > > gets set-in-stone?
> > > 
> > > /Bruce
> > > 
> > > 
> > 
> > See above, the API doesn't need to change at all to accomodate this as far as I
> > can see.
> > 
> > Neil
> Yes, there are alternative ways to see about accomplishing the same thing, but
> they are not nearly as clear as adding in the extra param. That's why we'd like
> to see this change go in before release, if possible. If it doesn't make 2.0
> I'd like to see it in 2.1, but at the cost of having an API change and the
> additional versionning and deprecation that ensues.
> 
Plese set asside the ABI issue for a moment.  I get that you're trying to get it
in prior to needing to version it.  Thats not the argument.  The argument is how
best to codify the new information you want to express in the callback.  For
this specific case, I think there are better ways to do this than to just
blindly add a new parameter.  Encoding the array size implicitly with a
terminating marker lets you use this equally well with the tx and rx callbacks
(should you ever need it on the latter), and isn't uncommon to do at all.  It
also lets you avoid the odd bugs that arise should the caller ever mis-encode
the array length such that it doesn't match the actual array size.

Using additional context sensitive functions are also nice, because they are
additive without being ABI breaking.  That is to say, in the future, if you want
to export more information to a callback you can do so by adding an API call
that simply didnt' exist before.  Thats a nice feature to be able to support.

Just adding more parameters isn't the only (nor in my view) the more flexible
way to do this

Neil

> Regards,
> /Bruce
> 
> 

Re: [dpdk-dev] [PATCH] ethdev: additional parameter in RX callback

[Mcnamara, John]

> -----Original Message-----
> From: Neil Horman [mailto:nhorman@tuxdriver.com]
> Sent: Friday, March 13, 2015 3:09 PM
> To: Richardson, Bruce
> Cc: Mcnamara, John; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] ethdev: additional parameter in RX
> callback
> 
> Plese set asside the ABI issue for a moment.  I get that you're trying to
> get it in prior to needing to version it.  Thats not the argument.  The
> argument is how best to codify the new information you want to express in
> the callback.  For this specific case, I think there are better ways to do
> this than to just blindly add a new parameter.

Hi Neil,

I think that is good advice is the general case but this is a very specific case. The modified callback is only used in rte_eth_rx_burst(). For context here is the function in its entirety (without #defs). The substantive change (the addition of nb_pkts) is on the line with an asterisk:


    static inline uint16_t
    rte_eth_rx_burst(uint8_t port_id, uint16_t queue_id,
             struct rte_mbuf **rx_pkts, const uint16_t nb_pkts)
    {
        struct rte_eth_dev *dev;

        dev = &rte_eth_devices[port_id];

        int16_t nb_rx = (*dev->rx_pkt_burst)(dev->data->rx_queues[queue_id],
                rx_pkts, nb_pkts);

        struct rte_eth_rxtx_callback *cb = dev->post_rx_burst_cbs[queue_id];

        if (unlikely(cb != NULL)) {
            do {
                nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
    *						nb_pkts, cb->param);
                cb = cb->next;
            } while (cb != NULL);
        }

        return nb_rx;
    }

> Encoding the array size
> implicitly with a terminating marker lets you use this equally well with
> the tx and rx callbacks (should you ever need it on the latter)

Is encoding the information in the array really a better solution here? The cb->param already exists for passing in user defined information to the callback. The proposed patch merely transmits the parent function arguments to the enclosed callback.

John

Re: [dpdk-dev] [PATCH] ethdev: additional parameter in RX callback

[Neil Horman]

On Fri, Mar 13, 2015 at 04:26:52PM +0000, Mcnamara, John wrote:
> 
> 
> > -----Original Message-----
> > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > Sent: Friday, March 13, 2015 3:09 PM
> > To: Richardson, Bruce
> > Cc: Mcnamara, John; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] ethdev: additional parameter in RX
> > callback
> > 
> > Plese set asside the ABI issue for a moment.  I get that you're trying to
> > get it in prior to needing to version it.  Thats not the argument.  The
> > argument is how best to codify the new information you want to express in
> > the callback.  For this specific case, I think there are better ways to do
> > this than to just blindly add a new parameter.
> 
> Hi Neil,
> 
> I think that is good advice is the general case but this is a very specific case. The modified callback is only used in rte_eth_rx_burst(). For context here is the function in its entirety (without #defs). The substantive change (the addition of nb_pkts) is on the line with an asterisk:
> 
> 
>     static inline uint16_t
>     rte_eth_rx_burst(uint8_t port_id, uint16_t queue_id,
>              struct rte_mbuf **rx_pkts, const uint16_t nb_pkts)
>     {
>         struct rte_eth_dev *dev;
> 
>         dev = &rte_eth_devices[port_id];
> 
>         int16_t nb_rx = (*dev->rx_pkt_burst)(dev->data->rx_queues[queue_id],
>                 rx_pkts, nb_pkts);
> 
>         struct rte_eth_rxtx_callback *cb = dev->post_rx_burst_cbs[queue_id];
> 
>         if (unlikely(cb != NULL)) {
>             do {
>                 nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
>     *						nb_pkts, cb->param);
>                 cb = cb->next;
>             } while (cb != NULL);
>         }
> 
>         return nb_rx;
>     }
> 

Not sure I grok your point here.  Why impact does the number of internal
callouts for the callback api have on how we structure the API to external
consumers?


> > Encoding the array size
> > implicitly with a terminating marker lets you use this equally well with
> > the tx and rx callbacks (should you ever need it on the latter)
> 
> Is encoding the information in the array really a better solution here? The cb->param already exists for passing in user defined information to the callback. The proposed patch merely transmits the parent function arguments to the enclosed callback.
> 
The cb->param can't be used here, because its opaque to the internals of the
DPDK.  rte_eth_rx_burst doesn't (and can't) know where in the cb->params pointer
to store that information.  Thats why you added an additional parameter in the
first place, isn't it?  My point is that using an array terminator keeps us out
of this habbit of just adding parameters to communicate more information (as
thats an ABI breaking method, and not particularly scalable if there is more
information to be transmitted in the future).  Using a context sensitive API set
goes beyond even that, and allows to retrieve arbitrary information form
callbacks as needed in an ABI safe manner

Neil

> John
> 
> 
> 
> 
> 
> 
> 

Re: [dpdk-dev] [PATCH] ethdev: additional parameter in RX callback

[Mcnamara, John]

> -----Original Message-----
> From: Neil Horman [mailto:nhorman@tuxdriver.com]
> Sent: Friday, March 13, 2015 5:32 PM
> To: Mcnamara, John
> Cc: Richardson, Bruce; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] ethdev: additional parameter in RX
> callback
> 
> > Is encoding the information in the array really a better solution here?
> The cb->param already exists for passing in user defined information to
> the callback. The proposed patch merely transmits the parent function
> arguments to the enclosed callback.
> >
> The cb->param can't be used here, because its opaque to the internals of
> the DPDK.  rte_eth_rx_burst doesn't (and can't) know where in the cb-
> >params pointer to store that information.  Thats why you added an
> additional parameter in the first place, isn't it?

Yes. That is correct.

> My point is that using
> an array terminator keeps us out of this habbit of just adding parameters
> to communicate more information (as thats an ABI breaking method, and not
> particularly scalable if there is more information to be transmitted in
> the future).  Using a context sensitive API set goes beyond even that, and
> allows to retrieve arbitrary information form callbacks as needed in an
> ABI safe manner

Again I can agree with this in the general case, but it isn't necessary, in this case, to encode the information in the array since it is already local to and available in the function. It seems artificial, at this point, to implement an array terminator solution to protect an API that, effectively, hasn't been published yet.

John

 

Re: [dpdk-dev] [PATCH] ethdev: additional parameter in RX callback

[Neil Horman]

On Fri, Mar 13, 2015 at 06:28:31PM +0000, Mcnamara, John wrote:
> 
> 
> > -----Original Message-----
> > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > Sent: Friday, March 13, 2015 5:32 PM
> > To: Mcnamara, John
> > Cc: Richardson, Bruce; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] ethdev: additional parameter in RX
> > callback
> > 
> > > Is encoding the information in the array really a better solution here?
> > The cb->param already exists for passing in user defined information to
> > the callback. The proposed patch merely transmits the parent function
> > arguments to the enclosed callback.
> > >
> > The cb->param can't be used here, because its opaque to the internals of
> > the DPDK.  rte_eth_rx_burst doesn't (and can't) know where in the cb-
> > >params pointer to store that information.  Thats why you added an
> > additional parameter in the first place, isn't it?
> 
> Yes. That is correct.
> 
Then why did you suggest doing so?

> > My point is that using
> > an array terminator keeps us out of this habbit of just adding parameters
> > to communicate more information (as thats an ABI breaking method, and not
> > particularly scalable if there is more information to be transmitted in
> > the future).  Using a context sensitive API set goes beyond even that, and
> > allows to retrieve arbitrary information form callbacks as needed in an
> > ABI safe manner
> 
> Again I can agree with this in the general case, but it isn't necessary, in this case, to encode the information in the array since it is already local to and available in the function. It seems artificial, at this point, to implement an array terminator solution to protect an API that, effectively, hasn't been published yet.
> 
You indicate that you agree an alternate solution is preferable in the general
case, so as to provide an API that is extensible in a way that isn't subject to
ABI breakage, correct?  If so, why do assert that its not necessecary in this
specific case?  If you feel you need to add information so that callbacks can be
more flexible (in this case specifying the size of a passed in array), why
immediately shoehorn another parmeter in place, and break the consistency
between rx and tx callbacks, when you don't have to?  I don't care if you break
ABI today (although to call it unpublished I think is disingenuous, as lots of
testing and development has already taken place with the ABI as it currently
stands).  I care, as I noted above about not getting into the habbit of just
assuming a change like this requires that you invaliate ABI somehow.  You don't
have to, you can create an API that is fairly invariant to it here if you like.
The question in my mind is, why don't you?

Neil


> John
> 
>  
> 
> 
> 

Re: [dpdk-dev] [PATCH] ethdev: additional parameter in RX callback

[Thomas Monjalon]

2015-03-13 19:15, Neil Horman:
> On Fri, Mar 13, 2015 at 06:28:31PM +0000, Mcnamara, John wrote:
> > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > > 
> > > > Is encoding the information in the array really a better solution here?
> > > The cb->param already exists for passing in user defined information to
> > > the callback. The proposed patch merely transmits the parent function
> > > arguments to the enclosed callback.
> > > >
> > > The cb->param can't be used here, because its opaque to the internals of
> > > the DPDK.  rte_eth_rx_burst doesn't (and can't) know where in the cb-
> > > >params pointer to store that information.  Thats why you added an
> > > additional parameter in the first place, isn't it?
> > 
> > Yes. That is correct.
> > 
> Then why did you suggest doing so?
> 
> > > My point is that using
> > > an array terminator keeps us out of this habbit of just adding parameters
> > > to communicate more information (as thats an ABI breaking method, and not
> > > particularly scalable if there is more information to be transmitted in
> > > the future).  Using a context sensitive API set goes beyond even that, and
> > > allows to retrieve arbitrary information form callbacks as needed in an
> > > ABI safe manner
> > 
> > Again I can agree with this in the general case, but it isn't necessary,
> > in this case, to encode the information in the array since it is already
> > local to and available in the function. It seems artificial, at this point,
> > to implement an array terminator solution to protect an API that,
> > effectively, hasn't been published yet.
> > 
> You indicate that you agree an alternate solution is preferable in the general
> case, so as to provide an API that is extensible in a way that isn't subject to
> ABI breakage, correct?  If so, why do assert that its not necessecary in this
> specific case?  If you feel you need to add information so that callbacks can be
> more flexible (in this case specifying the size of a passed in array), why
> immediately shoehorn another parmeter in place, and break the consistency
> between rx and tx callbacks, when you don't have to?  I don't care if you break
> ABI today (although to call it unpublished I think is disingenuous, as lots of
> testing and development has already taken place with the ABI as it currently
> stands).  I care, as I noted above about not getting into the habbit of just
> assuming a change like this requires that you invaliate ABI somehow.  You don't
> have to, you can create an API that is fairly invariant to it here if you like.
> The question in my mind is, why don't you?

I think John is saying that the API of rte_eth_rx_burst() already includes
the nb_pkts parameter. So it's natural to push it to the callback.
I also think Neil is saying that this parameter is useless in the callback
and in rte_eth_rx_burst() if the array was null terminated.
In any case, having a mix (null termination + parameter in rte_eth_rx_burst)
is not acceptable.
Moreover, I wonder how efficient are the compiler optimizations in each loop
case (index and null termination).

As the API was using an integer count, my opinion is to keep it and push it to
the callback for 2.0.
If null termination is validated to be better, it could be a later rework.

Is there something I'm missing?
Thoughts?

Re: [dpdk-dev] [PATCH] ethdev: additional parameter in RX callback

[Bruce Richardson]

On Mon, Mar 23, 2015 at 04:16:36PM +0100, Thomas Monjalon wrote:
> 2015-03-13 19:15, Neil Horman:
> > On Fri, Mar 13, 2015 at 06:28:31PM +0000, Mcnamara, John wrote:
> > > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > > > 
> > > > > Is encoding the information in the array really a better solution here?
> > > > The cb->param already exists for passing in user defined information to
> > > > the callback. The proposed patch merely transmits the parent function
> > > > arguments to the enclosed callback.
> > > > >
> > > > The cb->param can't be used here, because its opaque to the internals of
> > > > the DPDK.  rte_eth_rx_burst doesn't (and can't) know where in the cb-
> > > > >params pointer to store that information.  Thats why you added an
> > > > additional parameter in the first place, isn't it?
> > > 
> > > Yes. That is correct.
> > > 
> > Then why did you suggest doing so?
> > 
> > > > My point is that using
> > > > an array terminator keeps us out of this habbit of just adding parameters
> > > > to communicate more information (as thats an ABI breaking method, and not
> > > > particularly scalable if there is more information to be transmitted in
> > > > the future).  Using a context sensitive API set goes beyond even that, and
> > > > allows to retrieve arbitrary information form callbacks as needed in an
> > > > ABI safe manner
> > > 
> > > Again I can agree with this in the general case, but it isn't necessary,
> > > in this case, to encode the information in the array since it is already
> > > local to and available in the function. It seems artificial, at this point,
> > > to implement an array terminator solution to protect an API that,
> > > effectively, hasn't been published yet.
> > > 
> > You indicate that you agree an alternate solution is preferable in the general
> > case, so as to provide an API that is extensible in a way that isn't subject to
> > ABI breakage, correct?  If so, why do assert that its not necessecary in this
> > specific case?  If you feel you need to add information so that callbacks can be
> > more flexible (in this case specifying the size of a passed in array), why
> > immediately shoehorn another parmeter in place, and break the consistency
> > between rx and tx callbacks, when you don't have to?  I don't care if you break
> > ABI today (although to call it unpublished I think is disingenuous, as lots of
> > testing and development has already taken place with the ABI as it currently
> > stands).  I care, as I noted above about not getting into the habbit of just
> > assuming a change like this requires that you invaliate ABI somehow.  You don't
> > have to, you can create an API that is fairly invariant to it here if you like.
> > The question in my mind is, why don't you?
> 
> I think John is saying that the API of rte_eth_rx_burst() already includes
> the nb_pkts parameter. So it's natural to push it to the callback.
> I also think Neil is saying that this parameter is useless in the callback
> and in rte_eth_rx_burst() if the array was null terminated.
> In any case, having a mix (null termination + parameter in rte_eth_rx_burst)
> is not acceptable.
> Moreover, I wonder how efficient are the compiler optimizations in each loop
> case (index and null termination).

Compiler can't optimize/unroll the loop in the null termination case. For the
passing-the-size through option, in any app where the RX buffer size is constant,
i.e. probably a lot of them - like in our examples, the compiler can do loop
unrolling, and possibly other optimizations on the known value. [Whether it choses
too or not, is not something we have tested :-)]

/Bruce

> 
> As the API was using an integer count, my opinion is to keep it and push it to
> the callback for 2.0.
> If null termination is validated to be better, it could be a later rework.
> 
> Is there something I'm missing?
> Thoughts?

Re: [dpdk-dev] [PATCH] ethdev: additional parameter in RX callback

[Neil Horman]

On Mon, Mar 23, 2015 at 04:16:36PM +0100, Thomas Monjalon wrote:
> 2015-03-13 19:15, Neil Horman:
> > On Fri, Mar 13, 2015 at 06:28:31PM +0000, Mcnamara, John wrote:
> > > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > > > 
> > > > > Is encoding the information in the array really a better solution here?
> > > > The cb->param already exists for passing in user defined information to
> > > > the callback. The proposed patch merely transmits the parent function
> > > > arguments to the enclosed callback.
> > > > >
> > > > The cb->param can't be used here, because its opaque to the internals of
> > > > the DPDK.  rte_eth_rx_burst doesn't (and can't) know where in the cb-
> > > > >params pointer to store that information.  Thats why you added an
> > > > additional parameter in the first place, isn't it?
> > > 
> > > Yes. That is correct.
> > > 
> > Then why did you suggest doing so?
> > 
> > > > My point is that using
> > > > an array terminator keeps us out of this habbit of just adding parameters
> > > > to communicate more information (as thats an ABI breaking method, and not
> > > > particularly scalable if there is more information to be transmitted in
> > > > the future).  Using a context sensitive API set goes beyond even that, and
> > > > allows to retrieve arbitrary information form callbacks as needed in an
> > > > ABI safe manner
> > > 
> > > Again I can agree with this in the general case, but it isn't necessary,
> > > in this case, to encode the information in the array since it is already
> > > local to and available in the function. It seems artificial, at this point,
> > > to implement an array terminator solution to protect an API that,
> > > effectively, hasn't been published yet.
> > > 
> > You indicate that you agree an alternate solution is preferable in the general
> > case, so as to provide an API that is extensible in a way that isn't subject to
> > ABI breakage, correct?  If so, why do assert that its not necessecary in this
> > specific case?  If you feel you need to add information so that callbacks can be
> > more flexible (in this case specifying the size of a passed in array), why
> > immediately shoehorn another parmeter in place, and break the consistency
> > between rx and tx callbacks, when you don't have to?  I don't care if you break
> > ABI today (although to call it unpublished I think is disingenuous, as lots of
> > testing and development has already taken place with the ABI as it currently
> > stands).  I care, as I noted above about not getting into the habbit of just
> > assuming a change like this requires that you invaliate ABI somehow.  You don't
> > have to, you can create an API that is fairly invariant to it here if you like.
> > The question in my mind is, why don't you?
> 
> I think John is saying that the API of rte_eth_rx_burst() already includes
> the nb_pkts parameter. So it's natural to push it to the callback.
> I also think Neil is saying that this parameter is useless in the callback
> and in rte_eth_rx_burst() if the array was null terminated.
> In any case, having a mix (null termination + parameter in rte_eth_rx_burst)
> is not acceptable.
> Moreover, I wonder how efficient are the compiler optimizations in each loop
> case (index and null termination).
> 
> As the API was using an integer count, my opinion is to keep it and push it to
> the callback for 2.0.
> If null termination is validated to be better, it could be a later rework.
> 

I'm fine with this if thats the consensus, I'm more interested in making sure we
think about these problems in such a way that we're not just running from ABI
issues, because we're eventually going to have to deal with them
Neil

> Is there something I'm missing?
> Thoughts?
> 

Re: [dpdk-dev] [PATCH] ethdev: additional parameter in RX callback

[Thomas Monjalon]

2015-03-23 12:00, Neil Horman:
> On Mon, Mar 23, 2015 at 04:16:36PM +0100, Thomas Monjalon wrote:
> > I think John is saying that the API of rte_eth_rx_burst() already includes
> > the nb_pkts parameter. So it's natural to push it to the callback.
> > I also think Neil is saying that this parameter is useless in the callback
> > and in rte_eth_rx_burst() if the array was null terminated.
> > In any case, having a mix (null termination + parameter in rte_eth_rx_burst)
> > is not acceptable.
> > Moreover, I wonder how efficient are the compiler optimizations in each loop
> > case (index and null termination).
> > 
> > As the API was using an integer count, my opinion is to keep it and push it to
> > the callback for 2.0.
> > If null termination is validated to be better, it could be a later rework.
> > 
> 
> I'm fine with this if thats the consensus, I'm more interested in making sure we
> think about these problems in such a way that we're not just running from ABI
> issues, because we're eventually going to have to deal with them
> Neil

Acked-by: Thomas Monjalon <thomas.monjalon@6wind.com>

Applied, thanks