diff mbox

[dpdk-dev,v5] ethdev: return named opaque type instead of void pointer

Message ID 20180320163404.7780-1-ferruh.yigit@intel.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail apply issues

Commit Message

Ferruh Yigit March 20, 2018, 4:34 p.m. UTC 
  "struct rte_eth_rxtx_callback" is defined as internal data structure and
used as named opaque type.

So the functions that are adding callbacks can return objects in this
type instead of void pointer.

Also const qualifier added to "struct rte_eth_rxtx_callback *" to
protect it better from application modification.

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
v2:
* keep using struct * in parameters, instead add callback functions
return struct rte_eth_rxtx_callback pointer.

v4:
* Remove deprecation notice. LIBABIVER already increased in this release

v5:
* add const qualifier to rte_eth_rxtx_callback
---
 doc/guides/rel_notes/deprecation.rst       |  7 -------
 lib/librte_ether/rte_ethdev.c              | 10 +++++-----
 lib/librte_ether/rte_ethdev.h              | 17 ++++++++++-------
 lib/librte_latencystats/rte_latencystats.c |  2 +-
 lib/librte_pdump/rte_pdump.c               |  2 +-
 5 files changed, 17 insertions(+), 21 deletions(-)

Comments

Neil Horman March 21, 2018, 1:04 p.m. UTC | #1 
On Tue, Mar 20, 2018 at 04:34:04PM +0000, Ferruh Yigit wrote:
> "struct rte_eth_rxtx_callback" is defined as internal data structure and
> used as named opaque type.
> 
> So the functions that are adding callbacks can return objects in this
> type instead of void pointer.
> 
> Also const qualifier added to "struct rte_eth_rxtx_callback *" to
> protect it better from application modification.
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
> v2:
> * keep using struct * in parameters, instead add callback functions
> return struct rte_eth_rxtx_callback pointer.
> 
> v4:
> * Remove deprecation notice. LIBABIVER already increased in this release
> 
> v5:
> * add const qualifier to rte_eth_rxtx_callback
I still wish we could find a way to remove the inline functions and truly
protect that struct, but a const is definately better than nothing

Acked-by: Neil Horman <nhorman@tuxdriver.com>
Bruce Richardson March 23, 2018, 5 p.m. UTC | #2 
On Wed, Mar 21, 2018 at 09:04:01AM -0400, Neil Horman wrote:
> On Tue, Mar 20, 2018 at 04:34:04PM +0000, Ferruh Yigit wrote:
> > "struct rte_eth_rxtx_callback" is defined as internal data structure and
> > used as named opaque type.
> > 
> > So the functions that are adding callbacks can return objects in this
> > type instead of void pointer.
> > 
> > Also const qualifier added to "struct rte_eth_rxtx_callback *" to
> > protect it better from application modification.
> > 
> > Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> > ---
> > v2:
> > * keep using struct * in parameters, instead add callback functions
> > return struct rte_eth_rxtx_callback pointer.
> > 
> > v4:
> > * Remove deprecation notice. LIBABIVER already increased in this release
> > 
> > v5:
> > * add const qualifier to rte_eth_rxtx_callback
> I still wish we could find a way to remove the inline functions and truly
> protect that struct, but a const is definately better than nothing
> 
> Acked-by: Neil Horman <nhorman@tuxdriver.com>
> 
Actually, I think we should do exactly that - convert the rx and tx burst
calls into actual function calls (and consider any other inlined functions
too). The cost would be the overhead of making an additional function call
per-burst, which is likely to be pretty minimal for most common burst sizes
e.g. 32.

We did some quick tests here with the i40e driver, and for a burst size of
32 saw less than 1% perf drop, and for even a small burst of 8 saw less
than 5% drop. Note that this is testing with testpmd, which has nothing but
I/O in the datapath. A real-world app is likely to do far more with the
packets and therefore see proportionally far less perf hit.

Thoughts?

/Bruce

PS: This un-inlining could probably be applied to other device types too,
e.g. cryptodev (though probably not eventdev as it tends to have smaller
bursts in some use-cases).
Neil Horman March 24, 2018, 2:08 a.m. UTC | #3 
On Fri, Mar 23, 2018 at 05:00:34PM +0000, Bruce Richardson wrote:
> On Wed, Mar 21, 2018 at 09:04:01AM -0400, Neil Horman wrote:
> > On Tue, Mar 20, 2018 at 04:34:04PM +0000, Ferruh Yigit wrote:
> > > "struct rte_eth_rxtx_callback" is defined as internal data structure and
> > > used as named opaque type.
> > > 
> > > So the functions that are adding callbacks can return objects in this
> > > type instead of void pointer.
> > > 
> > > Also const qualifier added to "struct rte_eth_rxtx_callback *" to
> > > protect it better from application modification.
> > > 
> > > Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> > > ---
> > > v2:
> > > * keep using struct * in parameters, instead add callback functions
> > > return struct rte_eth_rxtx_callback pointer.
> > > 
> > > v4:
> > > * Remove deprecation notice. LIBABIVER already increased in this release
> > > 
> > > v5:
> > > * add const qualifier to rte_eth_rxtx_callback
> > I still wish we could find a way to remove the inline functions and truly
> > protect that struct, but a const is definately better than nothing
> > 
> > Acked-by: Neil Horman <nhorman@tuxdriver.com>
> > 
> Actually, I think we should do exactly that - convert the rx and tx burst
> calls into actual function calls (and consider any other inlined functions
> too). The cost would be the overhead of making an additional function call
> per-burst, which is likely to be pretty minimal for most common burst sizes
> e.g. 32.
> 
> We did some quick tests here with the i40e driver, and for a burst size of
> 32 saw less than 1% perf drop, and for even a small burst of 8 saw less
> than 5% drop. Note that this is testing with testpmd, which has nothing but
> I/O in the datapath. A real-world app is likely to do far more with the
> packets and therefore see proportionally far less perf hit.
> 
> Thoughts?
> 
> /Bruce
> 
> PS: This un-inlining could probably be applied to other device types too,
> e.g. cryptodev (though probably not eventdev as it tends to have smaller
> bursts in some use-cases).
> 
I would be 1000% on board with this conversion.

Best
Neil
Ananyev, Konstantin March 26, 2018, 8:47 a.m. UTC | #4 
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Neil Horman
> Sent: Saturday, March 24, 2018 2:09 AM
> To: Richardson, Bruce <bruce.richardson@intel.com>
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Mcnamara, John <john.mcnamara@intel.com>; Kovacevic, Marko
> <marko.kovacevic@intel.com>; Thomas Monjalon <thomas@monjalon.net>; Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v5] ethdev: return named opaque type instead of void pointer
> 
> On Fri, Mar 23, 2018 at 05:00:34PM +0000, Bruce Richardson wrote:
> > On Wed, Mar 21, 2018 at 09:04:01AM -0400, Neil Horman wrote:
> > > On Tue, Mar 20, 2018 at 04:34:04PM +0000, Ferruh Yigit wrote:
> > > > "struct rte_eth_rxtx_callback" is defined as internal data structure and
> > > > used as named opaque type.
> > > >
> > > > So the functions that are adding callbacks can return objects in this
> > > > type instead of void pointer.
> > > >
> > > > Also const qualifier added to "struct rte_eth_rxtx_callback *" to
> > > > protect it better from application modification.
> > > >
> > > > Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> > > > ---
> > > > v2:
> > > > * keep using struct * in parameters, instead add callback functions
> > > > return struct rte_eth_rxtx_callback pointer.
> > > >
> > > > v4:
> > > > * Remove deprecation notice. LIBABIVER already increased in this release
> > > >
> > > > v5:
> > > > * add const qualifier to rte_eth_rxtx_callback
> > > I still wish we could find a way to remove the inline functions and truly
> > > protect that struct, but a const is definately better than nothing
> > >
> > > Acked-by: Neil Horman <nhorman@tuxdriver.com>
> > >
> > Actually, I think we should do exactly that - convert the rx and tx burst
> > calls into actual function calls (and consider any other inlined functions
> > too). The cost would be the overhead of making an additional function call
> > per-burst, which is likely to be pretty minimal for most common burst sizes
> > e.g. 32.
> >
> > We did some quick tests here with the i40e driver, and for a burst size of
> > 32 saw less than 1% perf drop, and for even a small burst of 8 saw less
> > than 5% drop. Note that this is testing with testpmd, which has nothing but
> > I/O in the datapath. A real-world app is likely to do far more with the
> > packets and therefore see proportionally far less perf hit.
> >
> > Thoughts?
> >
> > /Bruce
> >
> > PS: This un-inlining could probably be applied to other device types too,
> > e.g. cryptodev (though probably not eventdev as it tends to have smaller
> > bursts in some use-cases).
> >
> I would be 1000% on board with this conversion.

+1 from me too.
It would allow to greatly simplify support and further development for ethdev.
Konstantin
Ferruh Yigit March 27, 2018, 7:10 p.m. UTC | #5 
On 3/21/2018 1:04 PM, Neil Horman wrote:
> On Tue, Mar 20, 2018 at 04:34:04PM +0000, Ferruh Yigit wrote:
>> "struct rte_eth_rxtx_callback" is defined as internal data structure and
>> used as named opaque type.
>>
>> So the functions that are adding callbacks can return objects in this
>> type instead of void pointer.
>>
>> Also const qualifier added to "struct rte_eth_rxtx_callback *" to
>> protect it better from application modification.
>>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>> ---
>> v2:
>> * keep using struct * in parameters, instead add callback functions
>> return struct rte_eth_rxtx_callback pointer.
>>
>> v4:
>> * Remove deprecation notice. LIBABIVER already increased in this release
>>
>> v5:
>> * add const qualifier to rte_eth_rxtx_callback
> I still wish we could find a way to remove the inline functions and truly
> protect that struct, but a const is definately better than nothing
> 
> Acked-by: Neil Horman <nhorman@tuxdriver.com>

Applied to dpdk-next-net/master, thanks.
diff mbox

Patch

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index ad54de721..0c696f743 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -142,13 +142,6 @@  Deprecation Notices
   successful. This modification will only impact the PMDs, not the
   applications.
 
-* ethdev: functions add rx/tx callback will return named opaque type
-  ``rte_eth_add_rx_callback()``, ``rte_eth_add_first_rx_callback()`` and
-  ``rte_eth_add_tx_callback()`` functions currently return callback object as
-  ``void \*`` but APIs to delete callbacks get ``struct rte_eth_rxtx_callback \*``
-  as parameter. For consistency functions adding callback will return
-  ``struct rte_eth_rxtx_callback \*`` instead of ``void \*``.
-
 * i40e: The default flexible payload configuration which extracts the first 16
   bytes of the payload for RSS will be deprecated starting from 18.02. If
   required the previous behavior can be configured using existing flow
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index dd9470099..9304d0440 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -3487,7 +3487,7 @@  rte_eth_dev_filter_ctrl(uint16_t port_id, enum rte_filter_type filter_type,
 							     filter_op, arg));
 }
 
-void *
+const struct rte_eth_rxtx_callback *
 rte_eth_add_rx_callback(uint16_t port_id, uint16_t queue_id,
 		rte_rx_callback_fn fn, void *user_param)
 {
@@ -3529,7 +3529,7 @@  rte_eth_add_rx_callback(uint16_t port_id, uint16_t queue_id,
 	return cb;
 }
 
-void *
+const struct rte_eth_rxtx_callback *
 rte_eth_add_first_rx_callback(uint16_t port_id, uint16_t queue_id,
 		rte_rx_callback_fn fn, void *user_param)
 {
@@ -3564,7 +3564,7 @@  rte_eth_add_first_rx_callback(uint16_t port_id, uint16_t queue_id,
 	return cb;
 }
 
-void *
+const struct rte_eth_rxtx_callback *
 rte_eth_add_tx_callback(uint16_t port_id, uint16_t queue_id,
 		rte_tx_callback_fn fn, void *user_param)
 {
@@ -3609,7 +3609,7 @@  rte_eth_add_tx_callback(uint16_t port_id, uint16_t queue_id,
 
 int
 rte_eth_remove_rx_callback(uint16_t port_id, uint16_t queue_id,
-		struct rte_eth_rxtx_callback *user_cb)
+		const struct rte_eth_rxtx_callback *user_cb)
 {
 #ifndef RTE_ETHDEV_RXTX_CALLBACKS
 	return -ENOTSUP;
@@ -3643,7 +3643,7 @@  rte_eth_remove_rx_callback(uint16_t port_id, uint16_t queue_id,
 
 int
 rte_eth_remove_tx_callback(uint16_t port_id, uint16_t queue_id,
-		struct rte_eth_rxtx_callback *user_cb)
+		const struct rte_eth_rxtx_callback *user_cb)
 {
 #ifndef RTE_ETHDEV_RXTX_CALLBACKS
 	return -ENOTSUP;
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 213fe27e1..d890fa7bc 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -3004,6 +3004,8 @@  int rte_eth_dev_filter_ctrl(uint16_t port_id, enum rte_filter_type filter_type,
 int rte_eth_dev_get_dcb_info(uint16_t port_id,
 			     struct rte_eth_dcb_info *dcb_info);
 
+struct rte_eth_rxtx_callback;
+
 /**
  * Add a callback to be called on packet RX on a given port and queue.
  *
@@ -3028,7 +3030,8 @@  int rte_eth_dev_get_dcb_info(uint16_t port_id,
  *   NULL on error.
  *   On success, a pointer value which can later be used to remove the callback.
  */
-void *rte_eth_add_rx_callback(uint16_t port_id, uint16_t queue_id,
+const struct rte_eth_rxtx_callback *
+rte_eth_add_rx_callback(uint16_t port_id, uint16_t queue_id,
 		rte_rx_callback_fn fn, void *user_param);
 
 /**
@@ -3056,7 +3059,8 @@  void *rte_eth_add_rx_callback(uint16_t port_id, uint16_t queue_id,
  *   NULL on error.
  *   On success, a pointer value which can later be used to remove the callback.
  */
-void *rte_eth_add_first_rx_callback(uint16_t port_id, uint16_t queue_id,
+const struct rte_eth_rxtx_callback *
+rte_eth_add_first_rx_callback(uint16_t port_id, uint16_t queue_id,
 		rte_rx_callback_fn fn, void *user_param);
 
 /**
@@ -3083,11 +3087,10 @@  void *rte_eth_add_first_rx_callback(uint16_t port_id, uint16_t queue_id,
  *   NULL on error.
  *   On success, a pointer value which can later be used to remove the callback.
  */
-void *rte_eth_add_tx_callback(uint16_t port_id, uint16_t queue_id,
+const struct rte_eth_rxtx_callback *
+rte_eth_add_tx_callback(uint16_t port_id, uint16_t queue_id,
 		rte_tx_callback_fn fn, void *user_param);
 
-struct rte_eth_rxtx_callback;
-
 /**
  * Remove an RX packet callback from a given port and queue.
  *
@@ -3119,7 +3122,7 @@  struct rte_eth_rxtx_callback;
  *               is NULL or not found for the port/queue.
  */
 int rte_eth_remove_rx_callback(uint16_t port_id, uint16_t queue_id,
-		struct rte_eth_rxtx_callback *user_cb);
+		const struct rte_eth_rxtx_callback *user_cb);
 
 /**
  * Remove a TX packet callback from a given port and queue.
@@ -3152,7 +3155,7 @@  int rte_eth_remove_rx_callback(uint16_t port_id, uint16_t queue_id,
  *               is NULL or not found for the port/queue.
  */
 int rte_eth_remove_tx_callback(uint16_t port_id, uint16_t queue_id,
-		struct rte_eth_rxtx_callback *user_cb);
+		const struct rte_eth_rxtx_callback *user_cb);
 
 /**
  * Retrieve information about given port's RX queue.
diff --git a/lib/librte_latencystats/rte_latencystats.c b/lib/librte_latencystats/rte_latencystats.c
index 66330203c..fc9497659 100644
--- a/lib/librte_latencystats/rte_latencystats.c
+++ b/lib/librte_latencystats/rte_latencystats.c
@@ -46,7 +46,7 @@  struct rte_latency_stats {
 static struct rte_latency_stats *glob_stats;
 
 struct rxtx_cbs {
-	struct rte_eth_rxtx_callback *cb;
+	const struct rte_eth_rxtx_callback *cb;
 };
 
 static struct rxtx_cbs rx_cbs[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
diff --git a/lib/librte_pdump/rte_pdump.c b/lib/librte_pdump/rte_pdump.c
index ec8a5d84c..4fb0b4204 100644
--- a/lib/librte_pdump/rte_pdump.c
+++ b/lib/librte_pdump/rte_pdump.c
@@ -75,7 +75,7 @@  struct pdump_response {
 static struct pdump_rxtx_cbs {
 	struct rte_ring *ring;
 	struct rte_mempool *mp;
-	struct rte_eth_rxtx_callback *cb;
+	const struct rte_eth_rxtx_callback *cb;
 	void *filter;
 } rx_cbs[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT],
 tx_cbs[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];