diff mbox series

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

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

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.
diff mbox series

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;
 };