[RFC,01/15] eventdev: make driver interface as internal

Message ID 20210823194020.1229-1-pbhagavatula@marvell.com (mailing list archive)
State Superseded, archived
Delegated to: Jerin Jacob
Headers
Series [RFC,01/15] eventdev: make driver interface as internal |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Pavan Nikhilesh Bhagavatula Aug. 23, 2021, 7:40 p.m. UTC
  From: Pavan Nikhilesh <pbhagavatula@marvell.com>

Mark all the driver specific functions as internal, remove
`rte` prefix from `struct rte_eventdev_ops`.
Remove experimental tag from internal functions.
Remove `eventdev_pmd.h` from non-internal header files.

Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
---
 drivers/event/cnxk/cn10k_eventdev.c        |  2 +-
 drivers/event/cnxk/cn9k_eventdev.c         |  2 +-
 drivers/event/dlb2/dlb2.c                  |  2 +-
 drivers/event/dpaa/dpaa_eventdev.c         |  2 +-
 drivers/event/dpaa2/dpaa2_eventdev.c       |  2 +-
 drivers/event/dsw/dsw_evdev.c              |  2 +-
 drivers/event/octeontx/ssovf_evdev.c       |  2 +-
 drivers/event/octeontx2/otx2_evdev.c       |  2 +-
 drivers/event/opdl/opdl_evdev.c            |  2 +-
 drivers/event/skeleton/skeleton_eventdev.c |  2 +-
 drivers/event/sw/sw_evdev.c                |  2 +-
 lib/eventdev/eventdev_pmd.h                |  6 +++++-
 lib/eventdev/eventdev_pmd_pci.h            |  4 +++-
 lib/eventdev/eventdev_pmd_vdev.h           |  2 ++
 lib/eventdev/meson.build                   |  6 ++++++
 lib/eventdev/rte_event_crypto_adapter.h    |  1 -
 lib/eventdev/rte_eventdev.h                |  4 ++--
 lib/eventdev/version.map                   | 17 +++++++++--------
 18 files changed, 38 insertions(+), 24 deletions(-)
  

Comments

Mattias Rönnblom Aug. 24, 2021, 7:43 a.m. UTC | #1
On 2021-08-23 21:40, pbhagavatula@marvell.com wrote:
> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>
> Mark all the driver specific functions as internal, remove
> `rte` prefix from `struct rte_eventdev_ops`.
> Remove experimental tag from internal functions.
> Remove `eventdev_pmd.h` from non-internal header files.
>
Is the enqueue/dequeue shortcut still worth the trouble? Considering the 
size of this patch set, it seems to be a lot of trouble to handle this 
special case.


Is the same kind of reorganization planned for the ethdev API?


<snip>
  
Pavan Nikhilesh Bhagavatula Aug. 24, 2021, 7:47 a.m. UTC | #2
>On 2021-08-23 21:40, pbhagavatula@marvell.com wrote:
>> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>>
>> Mark all the driver specific functions as internal, remove
>> `rte` prefix from `struct rte_eventdev_ops`.
>> Remove experimental tag from internal functions.
>> Remove `eventdev_pmd.h` from non-internal header files.
>>
>Is the enqueue/dequeue shortcut still worth the trouble? Considering
>the
>size of this patch set, it seems to be a lot of trouble to handle this
>special case.
>
>
>Is the same kind of reorganization planned for the ethdev API?

There is already a series by Konstantin 
http://patches.dpdk.org/project/dpdk/list/?series=18422

>
>
><snip>
>
  
Pavan Nikhilesh Bhagavatula Aug. 24, 2021, 8:05 a.m. UTC | #3
>>On 2021-08-23 21:40, pbhagavatula@marvell.com wrote:
>>> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>>>
>>> Mark all the driver specific functions as internal, remove
>>> `rte` prefix from `struct rte_eventdev_ops`.
>>> Remove experimental tag from internal functions.
>>> Remove `eventdev_pmd.h` from non-internal header files.
>>>
>>Is the enqueue/dequeue shortcut still worth the trouble? Considering
>>the
>>size of this patch set, it seems to be a lot of trouble to handle this
>>special case.
>>
>>
>>Is the same kind of reorganization planned for the ethdev API?
>
>There is already a series by Konstantin
>http://patches.dpdk.org/project/dpdk/list/?series=18422

http://patches.dpdk.org/project/dpdk/list/?series=18382

>>
>>
>><snip>
>>
  
Mattias Rönnblom Aug. 30, 2021, 10:25 a.m. UTC | #4
On 2021-08-24 09:43, Mattias Rönnblom wrote:
> On 2021-08-23 21:40, pbhagavatula@marvell.com wrote:
>> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>>
>> Mark all the driver specific functions as internal, remove
>> `rte` prefix from `struct rte_eventdev_ops`.
>> Remove experimental tag from internal functions.
>> Remove `eventdev_pmd.h` from non-internal header files.
>>
> Is the enqueue/dequeue shortcut still worth the trouble? Considering the
> size of this patch set, it seems to be a lot of trouble to handle this
> special case.
>
>

I had a quick look at this, using an overhead measurement benchmark for 
DSW. Depending on compiler version and details of the test program's 
structure, the gains ranged from modest to non-existent. In some 
scenarios, the inline versions even performed more poorly than a 
function call proper. This was on a Intel Skylake and static DPDK linking.


The dev and port lookup are essentially a very short pointer chase, and 
in case the dev table and the dev struct itself is not in a close cache, 
significant stalls may occur. For most applications they will be in L1 
though, I imagine. The inline version should give the compiler some 
freedom to generate the appropriate loads earlier. If you insert a 
compiler barrier before the rte_event_*() call, the inline version seem 
to have no gains at all.


Did anyone else attempt to quantify the performance gains with keeping 
these functions as inline?


/M
  
Jerin Jacob Sept. 28, 2021, 9:56 a.m. UTC | #5
On Tue, Aug 24, 2021 at 1:10 AM <pbhagavatula@marvell.com> wrote:
>
> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>
> Mark all the driver specific functions as internal, remove
> `rte` prefix from `struct rte_eventdev_ops`.
> Remove experimental tag from internal functions.
> Remove `eventdev_pmd.h` from non-internal header files.
>
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>


Seems like ethdev side there is conscious with
https://patches.dpdk.org/project/dpdk/list/?series=19084
Could you respin the version similar to
https://patches.dpdk.org/project/dpdk/list/?series=19084 or the next
version v3 from Konstantin.
Since eventdev does not have a callback, largely this series aligns
with expected output. But please align function and structure name
etc with ethdev for next series. Marking as "Changes Requested".
Thanks for the rework.
  

Patch

diff --git a/drivers/event/cnxk/cn10k_eventdev.c b/drivers/event/cnxk/cn10k_eventdev.c
index 6f37c5bd23..697b134041 100644
--- a/drivers/event/cnxk/cn10k_eventdev.c
+++ b/drivers/event/cnxk/cn10k_eventdev.c
@@ -821,7 +821,7 @@  cn10k_sso_tx_adapter_queue_del(uint8_t id, const struct rte_eventdev *event_dev,
 	return cn10k_sso_updt_tx_adptr_data(event_dev);
 }
 
-static struct rte_eventdev_ops cn10k_sso_dev_ops = {
+static struct eventdev_ops cn10k_sso_dev_ops = {
 	.dev_infos_get = cn10k_sso_info_get,
 	.dev_configure = cn10k_sso_dev_configure,
 	.queue_def_conf = cnxk_sso_queue_def_conf,
diff --git a/drivers/event/cnxk/cn9k_eventdev.c b/drivers/event/cnxk/cn9k_eventdev.c
index a69edff195..9b439947e5 100644
--- a/drivers/event/cnxk/cn9k_eventdev.c
+++ b/drivers/event/cnxk/cn9k_eventdev.c
@@ -1069,7 +1069,7 @@  cn9k_sso_tx_adapter_queue_del(uint8_t id, const struct rte_eventdev *event_dev,
 	return cn9k_sso_updt_tx_adptr_data(event_dev);
 }
 
-static struct rte_eventdev_ops cn9k_sso_dev_ops = {
+static struct eventdev_ops cn9k_sso_dev_ops = {
 	.dev_infos_get = cn9k_sso_info_get,
 	.dev_configure = cn9k_sso_dev_configure,
 	.queue_def_conf = cnxk_sso_queue_def_conf,
diff --git a/drivers/event/dlb2/dlb2.c b/drivers/event/dlb2/dlb2.c
index 252bbd8d5e..c8742ddb2c 100644
--- a/drivers/event/dlb2/dlb2.c
+++ b/drivers/event/dlb2/dlb2.c
@@ -4384,7 +4384,7 @@  dlb2_entry_points_init(struct rte_eventdev *dev)
 	struct dlb2_eventdev *dlb2;
 
 	/* Expose PMD's eventdev interface */
-	static struct rte_eventdev_ops dlb2_eventdev_entry_ops = {
+	static struct eventdev_ops dlb2_eventdev_entry_ops = {
 		.dev_infos_get    = dlb2_eventdev_info_get,
 		.dev_configure    = dlb2_eventdev_configure,
 		.dev_start        = dlb2_eventdev_start,
diff --git a/drivers/event/dpaa/dpaa_eventdev.c b/drivers/event/dpaa/dpaa_eventdev.c
index ec74160325..9f14390d28 100644
--- a/drivers/event/dpaa/dpaa_eventdev.c
+++ b/drivers/event/dpaa/dpaa_eventdev.c
@@ -925,7 +925,7 @@  dpaa_eventdev_txa_enqueue(void *port,
 	return nb_events;
 }
 
-static struct rte_eventdev_ops dpaa_eventdev_ops = {
+static struct eventdev_ops dpaa_eventdev_ops = {
 	.dev_infos_get    = dpaa_event_dev_info_get,
 	.dev_configure    = dpaa_event_dev_configure,
 	.dev_start        = dpaa_event_dev_start,
diff --git a/drivers/event/dpaa2/dpaa2_eventdev.c b/drivers/event/dpaa2/dpaa2_eventdev.c
index 5ccf22f77f..d577f64824 100644
--- a/drivers/event/dpaa2/dpaa2_eventdev.c
+++ b/drivers/event/dpaa2/dpaa2_eventdev.c
@@ -1015,7 +1015,7 @@  dpaa2_eventdev_txa_enqueue(void *port,
 	return nb_events;
 }
 
-static struct rte_eventdev_ops dpaa2_eventdev_ops = {
+static struct eventdev_ops dpaa2_eventdev_ops = {
 	.dev_infos_get    = dpaa2_eventdev_info_get,
 	.dev_configure    = dpaa2_eventdev_configure,
 	.dev_start        = dpaa2_eventdev_start,
diff --git a/drivers/event/dsw/dsw_evdev.c b/drivers/event/dsw/dsw_evdev.c
index 2301a4b7a0..01f060fff3 100644
--- a/drivers/event/dsw/dsw_evdev.c
+++ b/drivers/event/dsw/dsw_evdev.c
@@ -398,7 +398,7 @@  dsw_crypto_adapter_caps_get(const struct rte_eventdev *dev  __rte_unused,
 	return 0;
 }
 
-static struct rte_eventdev_ops dsw_evdev_ops = {
+static struct eventdev_ops dsw_evdev_ops = {
 	.port_setup = dsw_port_setup,
 	.port_def_conf = dsw_port_def_conf,
 	.port_release = dsw_port_release,
diff --git a/drivers/event/octeontx/ssovf_evdev.c b/drivers/event/octeontx/ssovf_evdev.c
index b93f6ec8c6..4a8c6a13a5 100644
--- a/drivers/event/octeontx/ssovf_evdev.c
+++ b/drivers/event/octeontx/ssovf_evdev.c
@@ -790,7 +790,7 @@  ssovf_crypto_adapter_qp_del(const struct rte_eventdev *dev,
 }
 
 /* Initialize and register event driver with DPDK Application */
-static struct rte_eventdev_ops ssovf_ops = {
+static struct eventdev_ops ssovf_ops = {
 	.dev_infos_get    = ssovf_info_get,
 	.dev_configure    = ssovf_configure,
 	.queue_def_conf   = ssovf_queue_def_conf,
diff --git a/drivers/event/octeontx2/otx2_evdev.c b/drivers/event/octeontx2/otx2_evdev.c
index 38a6b651d9..00902ebf53 100644
--- a/drivers/event/octeontx2/otx2_evdev.c
+++ b/drivers/event/octeontx2/otx2_evdev.c
@@ -1596,7 +1596,7 @@  otx2_sso_close(struct rte_eventdev *event_dev)
 }
 
 /* Initialize and register event driver with DPDK Application */
-static struct rte_eventdev_ops otx2_sso_ops = {
+static struct eventdev_ops otx2_sso_ops = {
 	.dev_infos_get    = otx2_sso_info_get,
 	.dev_configure    = otx2_sso_configure,
 	.queue_def_conf   = otx2_sso_queue_def_conf,
diff --git a/drivers/event/opdl/opdl_evdev.c b/drivers/event/opdl/opdl_evdev.c
index cfa9733b64..739dc64c82 100644
--- a/drivers/event/opdl/opdl_evdev.c
+++ b/drivers/event/opdl/opdl_evdev.c
@@ -609,7 +609,7 @@  set_do_test(const char *key __rte_unused, const char *value, void *opaque)
 static int
 opdl_probe(struct rte_vdev_device *vdev)
 {
-	static struct rte_eventdev_ops evdev_opdl_ops = {
+	static struct eventdev_ops evdev_opdl_ops = {
 		.dev_configure = opdl_dev_configure,
 		.dev_infos_get = opdl_info_get,
 		.dev_close = opdl_close,
diff --git a/drivers/event/skeleton/skeleton_eventdev.c b/drivers/event/skeleton/skeleton_eventdev.c
index 6fd1102596..c9e17e7cb1 100644
--- a/drivers/event/skeleton/skeleton_eventdev.c
+++ b/drivers/event/skeleton/skeleton_eventdev.c
@@ -320,7 +320,7 @@  skeleton_eventdev_dump(struct rte_eventdev *dev, FILE *f)
 
 
 /* Initialize and register event driver with DPDK Application */
-static struct rte_eventdev_ops skeleton_eventdev_ops = {
+static struct eventdev_ops skeleton_eventdev_ops = {
 	.dev_infos_get    = skeleton_eventdev_info_get,
 	.dev_configure    = skeleton_eventdev_configure,
 	.dev_start        = skeleton_eventdev_start,
diff --git a/drivers/event/sw/sw_evdev.c b/drivers/event/sw/sw_evdev.c
index a5e6ca22e8..9b72073322 100644
--- a/drivers/event/sw/sw_evdev.c
+++ b/drivers/event/sw/sw_evdev.c
@@ -945,7 +945,7 @@  static int32_t sw_sched_service_func(void *args)
 static int
 sw_probe(struct rte_vdev_device *vdev)
 {
-	static struct rte_eventdev_ops evdev_sw_ops = {
+	static struct eventdev_ops evdev_sw_ops = {
 			.dev_configure = sw_dev_configure,
 			.dev_infos_get = sw_info_get,
 			.dev_close = sw_close,
diff --git a/lib/eventdev/eventdev_pmd.h b/lib/eventdev/eventdev_pmd.h
index 0f724ac85d..5dab9e2f70 100644
--- a/lib/eventdev/eventdev_pmd.h
+++ b/lib/eventdev/eventdev_pmd.h
@@ -103,6 +103,7 @@  extern struct rte_eventdev *rte_eventdevs;
  * @return
  *   - The rte_eventdev structure pointer for the given device ID.
  */
+__rte_internal
 static inline struct rte_eventdev *
 rte_event_pmd_get_named_dev(const char *name)
 {
@@ -131,6 +132,7 @@  rte_event_pmd_get_named_dev(const char *name)
  * @return
  *   - If the device index is valid (1) or not (0).
  */
+__rte_internal
 static inline unsigned
 rte_event_pmd_is_valid_dev(uint8_t dev_id)
 {
@@ -1060,7 +1062,7 @@  typedef int (*eventdev_eth_tx_adapter_stats_reset_t)(uint8_t id,
 					const struct rte_eventdev *dev);
 
 /** Event device operations function pointer table */
-struct rte_eventdev_ops {
+struct eventdev_ops {
 	eventdev_info_get_t dev_infos_get;	/**< Get device info. */
 	eventdev_configure_t dev_configure;	/**< Configure device. */
 	eventdev_start_t dev_start;		/**< Start device. */
@@ -1178,6 +1180,7 @@  struct rte_eventdev_ops {
  * @return
  *   - Slot in the rte_dev_devices array for a new device;
  */
+__rte_internal
 struct rte_eventdev *
 rte_event_pmd_allocate(const char *name, int socket_id);
 
@@ -1189,6 +1192,7 @@  rte_event_pmd_allocate(const char *name, int socket_id);
  * @return
  *   - 0 on success, negative on error
  */
+__rte_internal
 int
 rte_event_pmd_release(struct rte_eventdev *eventdev);
 
diff --git a/lib/eventdev/eventdev_pmd_pci.h b/lib/eventdev/eventdev_pmd_pci.h
index d14ea634b8..02c6d42a80 100644
--- a/lib/eventdev/eventdev_pmd_pci.h
+++ b/lib/eventdev/eventdev_pmd_pci.h
@@ -36,7 +36,7 @@  typedef int (*eventdev_pmd_pci_callback_t)(struct rte_eventdev *dev);
  * interface.  Same as rte_event_pmd_pci_probe, except caller can specify
  * the name.
  */
-__rte_experimental
+__rte_internal
 static inline int
 rte_event_pmd_pci_probe_named(struct rte_pci_driver *pci_drv,
 			      struct rte_pci_device *pci_dev,
@@ -90,6 +90,7 @@  rte_event_pmd_pci_probe_named(struct rte_pci_driver *pci_drv,
  * Wrapper for use by pci drivers as a .probe function to attach to a event
  * interface.
  */
+__rte_internal
 static inline int
 rte_event_pmd_pci_probe(struct rte_pci_driver *pci_drv,
 			    struct rte_pci_device *pci_dev,
@@ -113,6 +114,7 @@  rte_event_pmd_pci_probe(struct rte_pci_driver *pci_drv,
  * Wrapper for use by pci drivers as a .remove function to detach a event
  * interface.
  */
+__rte_internal
 static inline int
 rte_event_pmd_pci_remove(struct rte_pci_device *pci_dev,
 			     eventdev_pmd_pci_callback_t devuninit)
diff --git a/lib/eventdev/eventdev_pmd_vdev.h b/lib/eventdev/eventdev_pmd_vdev.h
index bc0cf44c8c..e645a21aad 100644
--- a/lib/eventdev/eventdev_pmd_vdev.h
+++ b/lib/eventdev/eventdev_pmd_vdev.h
@@ -41,6 +41,7 @@  extern "C" {
  *   - Eventdev pointer if device is successfully created.
  *   - NULL if device cannot be created.
  */
+__rte_internal
 static inline struct rte_eventdev *
 rte_event_pmd_vdev_init(const char *name, size_t dev_private_size,
 		int socket_id)
@@ -78,6 +79,7 @@  rte_event_pmd_vdev_init(const char *name, size_t dev_private_size,
  * @return
  *   - 0 on success, negative on error
  */
+__rte_internal
 static inline int
 rte_event_pmd_vdev_uninit(const char *name)
 {
diff --git a/lib/eventdev/meson.build b/lib/eventdev/meson.build
index 32abeba794..523ea9ccae 100644
--- a/lib/eventdev/meson.build
+++ b/lib/eventdev/meson.build
@@ -27,5 +27,11 @@  headers = files(
         'rte_event_crypto_adapter.h',
         'rte_event_eth_tx_adapter.h',
 )
+driver_sdk_headers += files(
+        'eventdev_pmd.h',
+        'eventdev_pmd_pci.h',
+        'eventdev_pmd_vdev.h',
+)
+
 deps += ['ring', 'ethdev', 'hash', 'mempool', 'mbuf', 'timer', 'cryptodev']
 deps += ['telemetry']
diff --git a/lib/eventdev/rte_event_crypto_adapter.h b/lib/eventdev/rte_event_crypto_adapter.h
index f8c6cca87c..431d05b6ed 100644
--- a/lib/eventdev/rte_event_crypto_adapter.h
+++ b/lib/eventdev/rte_event_crypto_adapter.h
@@ -171,7 +171,6 @@  extern "C" {
 #include <stdint.h>
 
 #include "rte_eventdev.h"
-#include "eventdev_pmd.h"
 
 /**
  * Crypto event adapter mode
diff --git a/lib/eventdev/rte_eventdev.h b/lib/eventdev/rte_eventdev.h
index a9c496fb62..6ba116002f 100644
--- a/lib/eventdev/rte_eventdev.h
+++ b/lib/eventdev/rte_eventdev.h
@@ -1324,7 +1324,7 @@  int
 rte_event_eth_tx_adapter_caps_get(uint8_t dev_id, uint16_t eth_port_id,
 				uint32_t *caps);
 
-struct rte_eventdev_ops;
+struct eventdev_ops;
 struct rte_eventdev;
 
 typedef uint16_t (*event_enqueue_t)(void *port, const struct rte_event *ev);
@@ -1429,7 +1429,7 @@  struct rte_eventdev {
 	/**< Pointer to PMD eth Tx adapter enqueue function. */
 	struct rte_eventdev_data *data;
 	/**< Pointer to device data */
-	struct rte_eventdev_ops *dev_ops;
+	struct eventdev_ops *dev_ops;
 	/**< Functions exported by PMD */
 	struct rte_device *dev;
 	/**< Device info. supplied by probing */
diff --git a/lib/eventdev/version.map b/lib/eventdev/version.map
index 88625621ec..5f1fe412a4 100644
--- a/lib/eventdev/version.map
+++ b/lib/eventdev/version.map
@@ -55,12 +55,6 @@  DPDK_22 {
 	rte_event_eth_tx_adapter_stats_get;
 	rte_event_eth_tx_adapter_stats_reset;
 	rte_event_eth_tx_adapter_stop;
-	rte_event_pmd_allocate;
-	rte_event_pmd_pci_probe;
-	rte_event_pmd_pci_remove;
-	rte_event_pmd_release;
-	rte_event_pmd_vdev_init;
-	rte_event_pmd_vdev_uninit;
 	rte_event_port_attr_get;
 	rte_event_port_default_conf_get;
 	rte_event_port_link;
@@ -136,8 +130,6 @@  EXPERIMENTAL {
 
 	# changed in 20.11
 	__rte_eventdev_trace_port_setup;
-	# added in 20.11
-	rte_event_pmd_pci_probe_named;
 
 	#added in 21.05
 	rte_event_vector_pool_create;
@@ -150,4 +142,13 @@  INTERNAL {
 	global:
 
 	rte_event_pmd_selftest_seqn_dynfield_offset;
+	rte_event_pmd_allocate;
+	rte_event_pmd_get_named_dev;
+	rte_event_pmd_is_valid_dev;
+	rte_event_pmd_pci_probe;
+	rte_event_pmd_pci_probe_named;
+	rte_event_pmd_pci_remove;
+	rte_event_pmd_release;
+	rte_event_pmd_vdev_init;
+	rte_event_pmd_vdev_uninit;
 };