[PATCH 1/2] eventdev: add device stop flush callback

[PATCH 1/2] eventdev: add device stop flush callback

[Gage Eads]

When an event device is stopped, it drains all event queues. These events
may contain pointers, so to prevent memory leaks eventdev now supports a
user-provided flush callback that is called during the queue drain process.
This callback is stored in process memory, so the callback must be
registered by any process that may call rte_event_dev_stop().

This commit also clarifies the behavior of rte_event_dev_stop().

This follows this mailing list discussion:
http://dpdk.org/ml/archives/dev/2018-January/087484.html

Signed-off-by: Gage Eads <gage.eads@intel.com>
---
 lib/librte_eventdev/rte_eventdev.c           | 20 +++++++++++
 lib/librte_eventdev/rte_eventdev.h           | 53 ++++++++++++++++++++++++++--
 lib/librte_eventdev/rte_eventdev_version.map |  6 ++++
 3 files changed, 77 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eventdev/rte_eventdev.c b/lib/librte_eventdev/rte_eventdev.c
index 851a119..7de44f9 100644
--- a/lib/librte_eventdev/rte_eventdev.c
+++ b/lib/librte_eventdev/rte_eventdev.c
@@ -1123,6 +1123,26 @@ rte_event_dev_start(uint8_t dev_id)
 	return 0;
 }
 
+int
+rte_event_dev_stop_flush_callback_register(uint8_t dev_id,
+		eventdev_stop_flush_t callback, void *userdata)
+{
+	struct rte_eventdev *dev;
+
+	RTE_EDEV_LOG_DEBUG("Stop flush register dev_id=%" PRIu8, dev_id);
+
+	RTE_EVENTDEV_VALID_DEVID_OR_ERR_RET(dev_id, -EINVAL);
+	dev = &rte_eventdevs[dev_id];
+
+	if (callback == NULL)
+		return -EINVAL;
+
+	dev->dev_stop_flush = callback;
+	dev->dev_stop_flush_arg = userdata;
+
+	return 0;
+}
+
 void
 rte_event_dev_stop(uint8_t dev_id)
 {
diff --git a/lib/librte_eventdev/rte_eventdev.h b/lib/librte_eventdev/rte_eventdev.h
index b21c271..ec3497f 100644
--- a/lib/librte_eventdev/rte_eventdev.h
+++ b/lib/librte_eventdev/rte_eventdev.h
@@ -835,11 +835,23 @@ int
 rte_event_dev_start(uint8_t dev_id);
 
 /**
- * Stop an event device. The device can be restarted with a call to
- * rte_event_dev_start()
+ * Stop an event device.
+ *
+ * This function causes all queued events to be drained. While draining events
+ * out of the device, this function calls the user-provided flush callback
+ * (if one was registered) once per event.
+ *
+ * This function does not drain events from event ports; the application is
+ * responsible for flushing events from all ports before stopping the device.
+ *
+ * The device can be restarted with a call to rte_event_dev_start(). Threads
+ * that continue to enqueue/dequeue while the device is stopped, or being
+ * stopped, will result in undefined behavior.
  *
  * @param dev_id
  *   Event device identifier.
+ *
+ * @see rte_event_dev_stop_flush_callback_register()
  */
 void
 rte_event_dev_stop(uint8_t dev_id);
@@ -1115,6 +1127,12 @@ typedef uint16_t (*event_dequeue_burst_t)(void *port, struct rte_event ev[],
 		uint16_t nb_events, uint64_t timeout_ticks);
 /**< @internal Dequeue burst of events from port of a device */
 
+typedef void (*eventdev_stop_flush_t)(uint8_t dev_id, struct rte_event event,
+		void *arg);
+/**< Callback function called during rte_event_dev_stop(), invoked once per
+ * flushed event.
+ */
+
 #define RTE_EVENTDEV_NAME_MAX_LEN	(64)
 /**< @internal Max length of name of event PMD */
 
@@ -1176,6 +1194,11 @@ struct rte_eventdev {
 	event_dequeue_burst_t dequeue_burst;
 	/**< Pointer to PMD dequeue burst function. */
 
+	eventdev_stop_flush_t dev_stop_flush;
+	/**< Optional, user-provided event flush function */
+	void *dev_stop_flush_arg;
+	/**< User-provided argument for event flush function */
+
 	struct rte_eventdev_data *data;
 	/**< Pointer to device data */
 	const struct rte_eventdev_ops *dev_ops;
@@ -1822,6 +1845,32 @@ rte_event_dev_xstats_reset(uint8_t dev_id,
  */
 int rte_event_dev_selftest(uint8_t dev_id);
 
+/**
+ * Registers a callback function to be invoked during rte_event_dev_stop() for
+ * each flushed event. This function can be used to properly dispose of queued
+ * events, for example events containing memory pointers.
+ *
+ * The callback function is only registered for the calling process. The
+ * callback function must be registered in every process that can call
+ * rte_event_dev_stop().
+ *
+ * @param dev_id
+ *   The identifier of the device.
+ * @param callback
+ *   Callback function invoked once per flushed event.
+ * @param userdata
+ *   Argument supplied to callback.
+ *
+ * @return
+ *  - 0 on success.
+ *  - -EINVAL if *dev_id* is invalid or *callback* is NULL
+ *
+ * @see rte_event_dev_stop()
+ */
+int
+rte_event_dev_stop_flush_callback_register(uint8_t dev_id,
+		eventdev_stop_flush_t callback, void *userdata);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_eventdev/rte_eventdev_version.map b/lib/librte_eventdev/rte_eventdev_version.map
index 2aef470..4396536 100644
--- a/lib/librte_eventdev/rte_eventdev_version.map
+++ b/lib/librte_eventdev/rte_eventdev_version.map
@@ -74,3 +74,9 @@ DPDK_18.02 {
 
 	rte_event_dev_selftest;
 } DPDK_17.11;
+
+DPDK_18.05 {
+	global:
+
+	rte_event_dev_stop_flush_callback_register;
+} DPDK_18.02;
-- 
2.7.4

[PATCH 2/2] event/sw: support device stop flush callback

[Gage Eads]

This commit also adds a flush callback test to the sw eventdev's selftest
suite.

Signed-off-by: Gage Eads <gage.eads@intel.com>
---
 drivers/event/sw/sw_evdev.c          | 25 +++++++++++-
 drivers/event/sw/sw_evdev_selftest.c | 75 +++++++++++++++++++++++++++++++++++-
 2 files changed, 97 insertions(+), 3 deletions(-)

diff --git a/drivers/event/sw/sw_evdev.c b/drivers/event/sw/sw_evdev.c
index 6672fd8..4b57e5b 100644
--- a/drivers/event/sw/sw_evdev.c
+++ b/drivers/event/sw/sw_evdev.c
@@ -362,8 +362,25 @@ sw_init_qid_iqs(struct sw_evdev *sw)
 }
 
 static void
-sw_clean_qid_iqs(struct sw_evdev *sw)
+sw_flush_iq(struct rte_eventdev *dev, struct sw_iq *iq)
 {
+	struct sw_evdev *sw = sw_pmd_priv(dev);
+
+	while (iq_count(iq) > 0) {
+		struct rte_event event;
+
+		iq_dequeue_burst(sw, iq, &event, 1);
+
+		dev->dev_stop_flush(dev->data->dev_id,
+				    event,
+				    dev->dev_stop_flush_arg);
+	}
+}
+
+static void
+sw_clean_qid_iqs(struct rte_eventdev *dev)
+{
+	struct sw_evdev *sw = sw_pmd_priv(dev);
 	int i, j;
 
 	/* Release the IQ memory of all configured qids */
@@ -373,7 +390,11 @@ sw_clean_qid_iqs(struct sw_evdev *sw)
 		for (j = 0; j < SW_IQS_MAX; j++) {
 			if (!qid->iq[j].head)
 				continue;
+
+			if (dev->dev_stop_flush)
+				sw_flush_iq(dev, &qid->iq[j]);
 			iq_free_chunk_list(sw, qid->iq[j].head);
+
 			qid->iq[j].head = NULL;
 		}
 	}
@@ -702,7 +723,7 @@ static void
 sw_stop(struct rte_eventdev *dev)
 {
 	struct sw_evdev *sw = sw_pmd_priv(dev);
-	sw_clean_qid_iqs(sw);
+	sw_clean_qid_iqs(dev);
 	sw_xstats_uninit(sw);
 	sw->started = 0;
 	rte_smp_wmb();
diff --git a/drivers/event/sw/sw_evdev_selftest.c b/drivers/event/sw/sw_evdev_selftest.c
index 78d30e0..f59362e 100644
--- a/drivers/event/sw/sw_evdev_selftest.c
+++ b/drivers/event/sw/sw_evdev_selftest.c
@@ -28,6 +28,7 @@
 #define MAX_PORTS 16
 #define MAX_QIDS 16
 #define NUM_PACKETS (1<<18)
+#define DEQUEUE_DEPTH 128
 
 static int evdev;
 
@@ -147,7 +148,7 @@ init(struct test *t, int nb_queues, int nb_ports)
 			.nb_event_ports = nb_ports,
 			.nb_event_queue_flows = 1024,
 			.nb_events_limit = 4096,
-			.nb_event_port_dequeue_depth = 128,
+			.nb_event_port_dequeue_depth = DEQUEUE_DEPTH,
 			.nb_event_port_enqueue_depth = 128,
 	};
 	int ret;
@@ -2807,6 +2808,72 @@ holb(struct test *t) /* test to check we avoid basic head-of-line blocking */
 	return -1;
 }
 
+static void
+flush(uint8_t dev_id __rte_unused, struct rte_event event, void *arg)
+{
+	*((uint8_t *) arg) += (event.u64 == 0xCA11BACC) ? 1 : 0;
+}
+
+static int
+dev_stop_flush(struct test *t) /* test to check we can properly flush events */
+{
+	const struct rte_event new_ev = {
+			.op = RTE_EVENT_OP_NEW,
+			.u64 = 0xCA11BACC
+			/* all other fields zero */
+	};
+	struct rte_event ev = new_ev;
+	uint8_t count = 0;
+	int i;
+
+	if (init(t, 1, 1) < 0 ||
+	    create_ports(t, 1) < 0 ||
+	    create_atomic_qids(t, 1) < 0) {
+		printf("%d: Error initializing device\n", __LINE__);
+		return -1;
+	}
+
+	/* Link the queue so *_start() doesn't error out */
+	if (rte_event_port_link(evdev, t->port[0], NULL, NULL, 0) != 1) {
+		printf("%d: Error linking queue to port\n", __LINE__);
+		goto err;
+	}
+
+	if (rte_event_dev_start(evdev) < 0) {
+		printf("%d: Error with start call\n", __LINE__);
+		goto err;
+	}
+
+	for (i = 0; i < DEQUEUE_DEPTH + 1; i++) {
+		if (rte_event_enqueue_burst(evdev, t->port[0], &ev, 1) != 1) {
+			printf("%d: Error enqueuing events\n", __LINE__);
+			goto err;
+		}
+	}
+
+	/* Schedule the events from the port to the IQ. At least one event
+	 * should be remaining in the queue.
+	 */
+	rte_service_run_iter_on_app_lcore(t->service_id, 1);
+
+	if (rte_event_dev_stop_flush_callback_register(evdev, flush, &count)) {
+		printf("%d: Error installing the flush callback\n", __LINE__);
+		goto err;
+	}
+
+	cleanup(t);
+
+	if (count == 0) {
+		printf("%d: Error executing the flush callback\n", __LINE__);
+		goto err;
+	}
+
+	return 0;
+err:
+	rte_event_dev_dump(evdev, stdout);
+	cleanup(t);
+	return -1;
+}
 static int
 worker_loopback_worker_fn(void *arg)
 {
@@ -3211,6 +3278,12 @@ test_sw_eventdev(void)
 		printf("ERROR - Head-of-line-blocking test FAILED.\n");
 		goto test_fail;
 	}
+	printf("*** Running Stop Flush test...\n");
+	ret = dev_stop_flush(t);
+	if (ret != 0) {
+		printf("ERROR - Stop Flush test FAILED.\n");
+		goto test_fail;
+	}
 	if (rte_lcore_count() >= 3) {
 		printf("*** Running Worker loopback test...\n");
 		ret = worker_loopback(t, 0);
-- 
2.7.4

[PATCH v2 1/2] eventdev: add device stop flush callback

[Gage Eads]

When an event device is stopped, it drains all event queues. These events
may contain pointers, so to prevent memory leaks eventdev now supports a
user-provided flush callback that is called during the queue drain process.
This callback is stored in process memory, so the callback must be
registered by any process that may call rte_event_dev_stop().

This commit also clarifies the behavior of rte_event_dev_stop().

This follows this mailing list discussion:
http://dpdk.org/ml/archives/dev/2018-January/087484.html

Signed-off-by: Gage Eads <gage.eads@intel.com>
---
v2: allow a NULL callback pointer to unregister the callback

 lib/librte_eventdev/rte_eventdev.c           | 17 +++++++++
 lib/librte_eventdev/rte_eventdev.h           | 55 +++++++++++++++++++++++++++-
 lib/librte_eventdev/rte_eventdev_version.map |  6 +++
 3 files changed, 76 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eventdev/rte_eventdev.c b/lib/librte_eventdev/rte_eventdev.c
index 851a119..1aacb7b 100644
--- a/lib/librte_eventdev/rte_eventdev.c
+++ b/lib/librte_eventdev/rte_eventdev.c
@@ -1123,6 +1123,23 @@ rte_event_dev_start(uint8_t dev_id)
 	return 0;
 }
 
+int
+rte_event_dev_stop_flush_callback_register(uint8_t dev_id,
+		eventdev_stop_flush_t callback, void *userdata)
+{
+	struct rte_eventdev *dev;
+
+	RTE_EDEV_LOG_DEBUG("Stop flush register dev_id=%" PRIu8, dev_id);
+
+	RTE_EVENTDEV_VALID_DEVID_OR_ERR_RET(dev_id, -EINVAL);
+	dev = &rte_eventdevs[dev_id];
+
+	dev->dev_stop_flush = callback;
+	dev->dev_stop_flush_arg = userdata;
+
+	return 0;
+}
+
 void
 rte_event_dev_stop(uint8_t dev_id)
 {
diff --git a/lib/librte_eventdev/rte_eventdev.h b/lib/librte_eventdev/rte_eventdev.h
index b21c271..ff62bab 100644
--- a/lib/librte_eventdev/rte_eventdev.h
+++ b/lib/librte_eventdev/rte_eventdev.h
@@ -835,11 +835,23 @@ int
 rte_event_dev_start(uint8_t dev_id);
 
 /**
- * Stop an event device. The device can be restarted with a call to
- * rte_event_dev_start()
+ * Stop an event device.
+ *
+ * This function causes all queued events to be drained. While draining events
+ * out of the device, this function calls the user-provided flush callback
+ * (if one was registered) once per event.
+ *
+ * This function does not drain events from event ports; the application is
+ * responsible for flushing events from all ports before stopping the device.
+ *
+ * The device can be restarted with a call to rte_event_dev_start(). Threads
+ * that continue to enqueue/dequeue while the device is stopped, or being
+ * stopped, will result in undefined behavior.
  *
  * @param dev_id
  *   Event device identifier.
+ *
+ * @see rte_event_dev_stop_flush_callback_register()
  */
 void
 rte_event_dev_stop(uint8_t dev_id);
@@ -1115,6 +1127,12 @@ typedef uint16_t (*event_dequeue_burst_t)(void *port, struct rte_event ev[],
 		uint16_t nb_events, uint64_t timeout_ticks);
 /**< @internal Dequeue burst of events from port of a device */
 
+typedef void (*eventdev_stop_flush_t)(uint8_t dev_id, struct rte_event event,
+		void *arg);
+/**< Callback function called during rte_event_dev_stop(), invoked once per
+ * flushed event.
+ */
+
 #define RTE_EVENTDEV_NAME_MAX_LEN	(64)
 /**< @internal Max length of name of event PMD */
 
@@ -1176,6 +1194,11 @@ struct rte_eventdev {
 	event_dequeue_burst_t dequeue_burst;
 	/**< Pointer to PMD dequeue burst function. */
 
+	eventdev_stop_flush_t dev_stop_flush;
+	/**< Optional, user-provided event flush function */
+	void *dev_stop_flush_arg;
+	/**< User-provided argument for event flush function */
+
 	struct rte_eventdev_data *data;
 	/**< Pointer to device data */
 	const struct rte_eventdev_ops *dev_ops;
@@ -1822,6 +1845,34 @@ rte_event_dev_xstats_reset(uint8_t dev_id,
  */
 int rte_event_dev_selftest(uint8_t dev_id);
 
+/**
+ * Registers a callback function to be invoked during rte_event_dev_stop() for
+ * each flushed event. This function can be used to properly dispose of queued
+ * events, for example events containing memory pointers.
+ *
+ * The callback function is only registered for the calling process. The
+ * callback function must be registered in every process that can call
+ * rte_event_dev_stop().
+ *
+ * To unregister a callback, call this function with a NULL callback pointer.
+ *
+ * @param dev_id
+ *   The identifier of the device.
+ * @param callback
+ *   Callback function invoked once per flushed event.
+ * @param userdata
+ *   Argument supplied to callback.
+ *
+ * @return
+ *  - 0 on success.
+ *  - -EINVAL if *dev_id* is invalid
+ *
+ * @see rte_event_dev_stop()
+ */
+int
+rte_event_dev_stop_flush_callback_register(uint8_t dev_id,
+		eventdev_stop_flush_t callback, void *userdata);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_eventdev/rte_eventdev_version.map b/lib/librte_eventdev/rte_eventdev_version.map
index 2aef470..4396536 100644
--- a/lib/librte_eventdev/rte_eventdev_version.map
+++ b/lib/librte_eventdev/rte_eventdev_version.map
@@ -74,3 +74,9 @@ DPDK_18.02 {
 
 	rte_event_dev_selftest;
 } DPDK_17.11;
+
+DPDK_18.05 {
+	global:
+
+	rte_event_dev_stop_flush_callback_register;
+} DPDK_18.02;
-- 
2.7.4

[PATCH v2 2/2] event/sw: support device stop flush callback

[Gage Eads]

This commit also adds a flush callback test to the sw eventdev's selftest
suite.

Signed-off-by: Gage Eads <gage.eads@intel.com>
---
 drivers/event/sw/sw_evdev.c          | 25 ++++++++++-
 drivers/event/sw/sw_evdev_selftest.c | 80 +++++++++++++++++++++++++++++++++++-
 2 files changed, 102 insertions(+), 3 deletions(-)

diff --git a/drivers/event/sw/sw_evdev.c b/drivers/event/sw/sw_evdev.c
index 6672fd8..4b57e5b 100644
--- a/drivers/event/sw/sw_evdev.c
+++ b/drivers/event/sw/sw_evdev.c
@@ -362,8 +362,25 @@ sw_init_qid_iqs(struct sw_evdev *sw)
 }
 
 static void
-sw_clean_qid_iqs(struct sw_evdev *sw)
+sw_flush_iq(struct rte_eventdev *dev, struct sw_iq *iq)
 {
+	struct sw_evdev *sw = sw_pmd_priv(dev);
+
+	while (iq_count(iq) > 0) {
+		struct rte_event event;
+
+		iq_dequeue_burst(sw, iq, &event, 1);
+
+		dev->dev_stop_flush(dev->data->dev_id,
+				    event,
+				    dev->dev_stop_flush_arg);
+	}
+}
+
+static void
+sw_clean_qid_iqs(struct rte_eventdev *dev)
+{
+	struct sw_evdev *sw = sw_pmd_priv(dev);
 	int i, j;
 
 	/* Release the IQ memory of all configured qids */
@@ -373,7 +390,11 @@ sw_clean_qid_iqs(struct sw_evdev *sw)
 		for (j = 0; j < SW_IQS_MAX; j++) {
 			if (!qid->iq[j].head)
 				continue;
+
+			if (dev->dev_stop_flush)
+				sw_flush_iq(dev, &qid->iq[j]);
 			iq_free_chunk_list(sw, qid->iq[j].head);
+
 			qid->iq[j].head = NULL;
 		}
 	}
@@ -702,7 +723,7 @@ static void
 sw_stop(struct rte_eventdev *dev)
 {
 	struct sw_evdev *sw = sw_pmd_priv(dev);
-	sw_clean_qid_iqs(sw);
+	sw_clean_qid_iqs(dev);
 	sw_xstats_uninit(sw);
 	sw->started = 0;
 	rte_smp_wmb();
diff --git a/drivers/event/sw/sw_evdev_selftest.c b/drivers/event/sw/sw_evdev_selftest.c
index 78d30e0..0d3b998 100644
--- a/drivers/event/sw/sw_evdev_selftest.c
+++ b/drivers/event/sw/sw_evdev_selftest.c
@@ -28,6 +28,7 @@
 #define MAX_PORTS 16
 #define MAX_QIDS 16
 #define NUM_PACKETS (1<<18)
+#define DEQUEUE_DEPTH 128
 
 static int evdev;
 
@@ -147,7 +148,7 @@ init(struct test *t, int nb_queues, int nb_ports)
 			.nb_event_ports = nb_ports,
 			.nb_event_queue_flows = 1024,
 			.nb_events_limit = 4096,
-			.nb_event_port_dequeue_depth = 128,
+			.nb_event_port_dequeue_depth = DEQUEUE_DEPTH,
 			.nb_event_port_enqueue_depth = 128,
 	};
 	int ret;
@@ -2807,6 +2808,77 @@ holb(struct test *t) /* test to check we avoid basic head-of-line blocking */
 	return -1;
 }
 
+static void
+flush(uint8_t dev_id __rte_unused, struct rte_event event, void *arg)
+{
+	*((uint8_t *) arg) += (event.u64 == 0xCA11BACC) ? 1 : 0;
+}
+
+static int
+dev_stop_flush(struct test *t) /* test to check we can properly flush events */
+{
+	const struct rte_event new_ev = {
+			.op = RTE_EVENT_OP_NEW,
+			.u64 = 0xCA11BACC
+			/* all other fields zero */
+	};
+	struct rte_event ev = new_ev;
+	uint8_t count = 0;
+	int i;
+
+	if (init(t, 1, 1) < 0 ||
+	    create_ports(t, 1) < 0 ||
+	    create_atomic_qids(t, 1) < 0) {
+		printf("%d: Error initializing device\n", __LINE__);
+		return -1;
+	}
+
+	/* Link the queue so *_start() doesn't error out */
+	if (rte_event_port_link(evdev, t->port[0], NULL, NULL, 0) != 1) {
+		printf("%d: Error linking queue to port\n", __LINE__);
+		goto err;
+	}
+
+	if (rte_event_dev_start(evdev) < 0) {
+		printf("%d: Error with start call\n", __LINE__);
+		goto err;
+	}
+
+	for (i = 0; i < DEQUEUE_DEPTH + 1; i++) {
+		if (rte_event_enqueue_burst(evdev, t->port[0], &ev, 1) != 1) {
+			printf("%d: Error enqueuing events\n", __LINE__);
+			goto err;
+		}
+	}
+
+	/* Schedule the events from the port to the IQ. At least one event
+	 * should be remaining in the queue.
+	 */
+	rte_service_run_iter_on_app_lcore(t->service_id, 1);
+
+	if (rte_event_dev_stop_flush_callback_register(evdev, flush, &count)) {
+		printf("%d: Error installing the flush callback\n", __LINE__);
+		goto err;
+	}
+
+	cleanup(t);
+
+	if (count == 0) {
+		printf("%d: Error executing the flush callback\n", __LINE__);
+		goto err;
+	}
+
+	if (rte_event_dev_stop_flush_callback_register(evdev, NULL, NULL)) {
+		printf("%d: Error uninstalling the flush callback\n", __LINE__);
+		goto err;
+	}
+
+	return 0;
+err:
+	rte_event_dev_dump(evdev, stdout);
+	cleanup(t);
+	return -1;
+}
 static int
 worker_loopback_worker_fn(void *arg)
 {
@@ -3211,6 +3283,12 @@ test_sw_eventdev(void)
 		printf("ERROR - Head-of-line-blocking test FAILED.\n");
 		goto test_fail;
 	}
+	printf("*** Running Stop Flush test...\n");
+	ret = dev_stop_flush(t);
+	if (ret != 0) {
+		printf("ERROR - Stop Flush test FAILED.\n");
+		goto test_fail;
+	}
 	if (rte_lcore_count() >= 3) {
 		printf("*** Running Worker loopback test...\n");
 		ret = worker_loopback(t, 0);
-- 
2.7.4

Re: [PATCH v2 1/2] eventdev: add device stop flush callback

[Jerin Jacob]

-----Original Message-----
> When an event device is stopped, it drains all event queues. These events
> may contain pointers, so to prevent memory leaks eventdev now supports a
> user-provided flush callback that is called during the queue drain process.
> This callback is stored in process memory, so the callback must be
> registered by any process that may call rte_event_dev_stop().
> 
> This commit also clarifies the behavior of rte_event_dev_stop().
> 
> This follows this mailing list discussion:
> http://dpdk.org/ml/archives/dev/2018-January/087484.html
> 
> Signed-off-by: Gage Eads <gage.eads@intel.com>
> ---
> v2: allow a NULL callback pointer to unregister the callback
> 
>  lib/librte_eventdev/rte_eventdev.c           | 17 +++++++++
>  lib/librte_eventdev/rte_eventdev.h           | 55 +++++++++++++++++++++++++++-
>  lib/librte_eventdev/rte_eventdev_version.map |  6 +++
>  3 files changed, 76 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_eventdev/rte_eventdev.c b/lib/librte_eventdev/rte_eventdev.c
> index 851a119..1aacb7b 100644
> --- a/lib/librte_eventdev/rte_eventdev.c
> +++ b/lib/librte_eventdev/rte_eventdev.c
> @@ -1123,6 +1123,23 @@ rte_event_dev_start(uint8_t dev_id)
>  	return 0;
>  }
>  
> +typedef void (*eventdev_stop_flush_t)(uint8_t dev_id, struct rte_event event,
> +		void *arg);
> +/**< Callback function called during rte_event_dev_stop(), invoked once per
> + * flushed event.
> + */
> +
>  #define RTE_EVENTDEV_NAME_MAX_LEN	(64)
>  /**< @internal Max length of name of event PMD */
>  
> @@ -1176,6 +1194,11 @@ struct rte_eventdev {
>  	event_dequeue_burst_t dequeue_burst;
>  	/**< Pointer to PMD dequeue burst function. */
>  
> +	eventdev_stop_flush_t dev_stop_flush;
> +	/**< Optional, user-provided event flush function */
> +	void *dev_stop_flush_arg;
> +	/**< User-provided argument for event flush function */
> +

I think, we can move this additions to the internal rte_eventdev_data structure. Reasons are
1) Changes to "struct rte_eventdev" would call for ABI change
2) We can keep "struct rte_eventdev" only for fast path functions,
slow path functions can have additional redirection.

>  	struct rte_eventdev_data *data;
>  	/**< Pointer to device data */
>  	const struct rte_eventdev_ops *dev_ops;
> @@ -1822,6 +1845,34 @@ rte_event_dev_xstats_reset(uint8_t dev_id,
>   */
>  int rte_event_dev_selftest(uint8_t dev_id);
>  
> +/**
> + * Registers a callback function to be invoked during rte_event_dev_stop() for
> + * each flushed event. This function can be used to properly dispose of queued
> + * events, for example events containing memory pointers.
> + *
> + * The callback function is only registered for the calling process. The
> + * callback function must be registered in every process that can call
> + * rte_event_dev_stop().
> + *
> + * To unregister a callback, call this function with a NULL callback pointer.
> + *
> + * @param dev_id
> + *   The identifier of the device.
> + * @param callback
> + *   Callback function invoked once per flushed event.
> + * @param userdata
> + *   Argument supplied to callback.
> + *
> + * @return
> + *  - 0 on success.
> + *  - -EINVAL if *dev_id* is invalid
> + *
> + * @see rte_event_dev_stop()
> + */
> +int
> +rte_event_dev_stop_flush_callback_register(uint8_t dev_id,
> +		eventdev_stop_flush_t callback, void *userdata);
> +
IMO, It would be better if we place this function near to rte_event_dev_stop().

Other than above minor changes, It looks good to me.

Re: [PATCH v2 1/2] eventdev: add device stop flush callback

[Eads, Gage]

> -----Original Message-----
> From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> Sent: Monday, March 12, 2018 1:25 AM
> To: Eads, Gage <gage.eads@intel.com>
> Cc: dev@dpdk.org; Van Haaren, Harry <harry.van.haaren@intel.com>;
> hemant.agrawal@nxp.com; Richardson, Bruce <bruce.richardson@intel.com>;
> santosh.shukla@caviumnetworks.com; nipun.gupta@nxp.com
> Subject: Re: [PATCH v2 1/2] eventdev: add device stop flush callback
> 
> -----Original Message-----
> > When an event device is stopped, it drains all event queues. These
> > events may contain pointers, so to prevent memory leaks eventdev now
> > supports a user-provided flush callback that is called during the queue drain
> process.
> > This callback is stored in process memory, so the callback must be
> > registered by any process that may call rte_event_dev_stop().
> >
> > This commit also clarifies the behavior of rte_event_dev_stop().
> >
> > This follows this mailing list discussion:
> > http://dpdk.org/ml/archives/dev/2018-January/087484.html
> >
> > Signed-off-by: Gage Eads <gage.eads@intel.com>
> > ---
> > v2: allow a NULL callback pointer to unregister the callback
> >
> >  lib/librte_eventdev/rte_eventdev.c           | 17 +++++++++
> >  lib/librte_eventdev/rte_eventdev.h           | 55
> +++++++++++++++++++++++++++-
> >  lib/librte_eventdev/rte_eventdev_version.map |  6 +++
> >  3 files changed, 76 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/librte_eventdev/rte_eventdev.c
> > b/lib/librte_eventdev/rte_eventdev.c
> > index 851a119..1aacb7b 100644
> > --- a/lib/librte_eventdev/rte_eventdev.c
> > +++ b/lib/librte_eventdev/rte_eventdev.c
> > @@ -1123,6 +1123,23 @@ rte_event_dev_start(uint8_t dev_id)
> >  	return 0;
> >  }
> >
> > +typedef void (*eventdev_stop_flush_t)(uint8_t dev_id, struct rte_event event,
> > +		void *arg);
> > +/**< Callback function called during rte_event_dev_stop(), invoked
> > +once per
> > + * flushed event.
> > + */
> > +
> >  #define RTE_EVENTDEV_NAME_MAX_LEN	(64)
> >  /**< @internal Max length of name of event PMD */
> >
> > @@ -1176,6 +1194,11 @@ struct rte_eventdev {
> >  	event_dequeue_burst_t dequeue_burst;
> >  	/**< Pointer to PMD dequeue burst function. */
> >
> > +	eventdev_stop_flush_t dev_stop_flush;
> > +	/**< Optional, user-provided event flush function */
> > +	void *dev_stop_flush_arg;
> > +	/**< User-provided argument for event flush function */
> > +
> 
> I think, we can move this additions to the internal rte_eventdev_data structure.
> Reasons are
> 1) Changes to "struct rte_eventdev" would call for ABI change
> 2) We can keep "struct rte_eventdev" only for fast path functions, slow path
> functions can have additional redirection.
> 

Good points -- I hadn't considered the ABI impact of modifying rte_eventdev. rte_eventdev_data is in shared memory, though, so it's not multi-process friendly for function pointers. How about putting it in rte_eventdev_ops?

> >  	struct rte_eventdev_data *data;
> >  	/**< Pointer to device data */
> >  	const struct rte_eventdev_ops *dev_ops; @@ -1822,6 +1845,34 @@
> > rte_event_dev_xstats_reset(uint8_t dev_id,
> >   */
> >  int rte_event_dev_selftest(uint8_t dev_id);
> >
> > +/**
> > + * Registers a callback function to be invoked during
> > +rte_event_dev_stop() for
> > + * each flushed event. This function can be used to properly dispose
> > +of queued
> > + * events, for example events containing memory pointers.
> > + *
> > + * The callback function is only registered for the calling process.
> > +The
> > + * callback function must be registered in every process that can
> > +call
> > + * rte_event_dev_stop().
> > + *
> > + * To unregister a callback, call this function with a NULL callback pointer.
> > + *
> > + * @param dev_id
> > + *   The identifier of the device.
> > + * @param callback
> > + *   Callback function invoked once per flushed event.
> > + * @param userdata
> > + *   Argument supplied to callback.
> > + *
> > + * @return
> > + *  - 0 on success.
> > + *  - -EINVAL if *dev_id* is invalid
> > + *
> > + * @see rte_event_dev_stop()
> > + */
> > +int
> > +rte_event_dev_stop_flush_callback_register(uint8_t dev_id,
> > +		eventdev_stop_flush_t callback, void *userdata);
> > +
> IMO, It would be better if we place this function near to rte_event_dev_stop().
> 
> Other than above minor changes, It looks good to me.

Ok, will address in v3.

Thanks,
Gage

Re: [PATCH v2 1/2] eventdev: add device stop flush callback

[Jerin Jacob]

-----Original Message-----
> Date: Mon, 12 Mar 2018 14:30:49 +0000
> From: "Eads, Gage" <gage.eads@intel.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> CC: "dev@dpdk.org" <dev@dpdk.org>, "Van Haaren, Harry"
>  <harry.van.haaren@intel.com>, "hemant.agrawal@nxp.com"
>  <hemant.agrawal@nxp.com>, "Richardson, Bruce"
>  <bruce.richardson@intel.com>, "santosh.shukla@caviumnetworks.com"
>  <santosh.shukla@caviumnetworks.com>, "nipun.gupta@nxp.com"
>  <nipun.gupta@nxp.com>
> Subject: RE: [PATCH v2 1/2] eventdev: add device stop flush callback
> 
> 
> 
> > -----Original Message-----
> > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> > Sent: Monday, March 12, 2018 1:25 AM
> > To: Eads, Gage <gage.eads@intel.com>
> > Cc: dev@dpdk.org; Van Haaren, Harry <harry.van.haaren@intel.com>;
> > hemant.agrawal@nxp.com; Richardson, Bruce <bruce.richardson@intel.com>;
> > santosh.shukla@caviumnetworks.com; nipun.gupta@nxp.com
> > Subject: Re: [PATCH v2 1/2] eventdev: add device stop flush callback
> > 
> > -----Original Message-----
> > > When an event device is stopped, it drains all event queues. These
> > > events may contain pointers, so to prevent memory leaks eventdev now
> > > supports a user-provided flush callback that is called during the queue drain
> > process.
> > > This callback is stored in process memory, so the callback must be
> > > registered by any process that may call rte_event_dev_stop().
> > >
> > > This commit also clarifies the behavior of rte_event_dev_stop().
> > >
> > > This follows this mailing list discussion:
> > > http://dpdk.org/ml/archives/dev/2018-January/087484.html
> > >
> > > Signed-off-by: Gage Eads <gage.eads@intel.com>
> > > ---
> > > v2: allow a NULL callback pointer to unregister the callback
> > >
> > >  lib/librte_eventdev/rte_eventdev.c           | 17 +++++++++
> > >  lib/librte_eventdev/rte_eventdev.h           | 55
> > +++++++++++++++++++++++++++-
> > >  lib/librte_eventdev/rte_eventdev_version.map |  6 +++
> > >  3 files changed, 76 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/lib/librte_eventdev/rte_eventdev.c
> > > b/lib/librte_eventdev/rte_eventdev.c
> > > index 851a119..1aacb7b 100644
> > > --- a/lib/librte_eventdev/rte_eventdev.c
> > > +++ b/lib/librte_eventdev/rte_eventdev.c
> > > @@ -1123,6 +1123,23 @@ rte_event_dev_start(uint8_t dev_id)
> > >  	return 0;
> > >  }
> > >
> > > +typedef void (*eventdev_stop_flush_t)(uint8_t dev_id, struct rte_event event,
> > > +		void *arg);
> > > +/**< Callback function called during rte_event_dev_stop(), invoked
> > > +once per
> > > + * flushed event.
> > > + */
> > > +
> > >  #define RTE_EVENTDEV_NAME_MAX_LEN	(64)
> > >  /**< @internal Max length of name of event PMD */
> > >
> > > @@ -1176,6 +1194,11 @@ struct rte_eventdev {
> > >  	event_dequeue_burst_t dequeue_burst;
> > >  	/**< Pointer to PMD dequeue burst function. */
> > >
> > > +	eventdev_stop_flush_t dev_stop_flush;
> > > +	/**< Optional, user-provided event flush function */
> > > +	void *dev_stop_flush_arg;
> > > +	/**< User-provided argument for event flush function */
> > > +
> > 
> > I think, we can move this additions to the internal rte_eventdev_data structure.
> > Reasons are
> > 1) Changes to "struct rte_eventdev" would call for ABI change
> > 2) We can keep "struct rte_eventdev" only for fast path functions, slow path
> > functions can have additional redirection.
> > 
> 
> Good points -- I hadn't considered the ABI impact of modifying rte_eventdev. rte_eventdev_data is in shared memory, though, so it's not multi-process friendly for function pointers. How about putting it in rte_eventdev_ops?

Yes. Make sense to move to rte_eventdev_ops. But need to take care
updating the those function pointers in secondary process.

[PATCH v3 1/2] eventdev: add device stop flush callback

[Gage Eads]

When an event device is stopped, it drains all event queues. These events
may contain pointers, so to prevent memory leaks eventdev now supports a
user-provided flush callback that is called during the queue drain process.
This callback is stored in process memory, so the callback must be
registered by any process that may call rte_event_dev_stop().

This commit also clarifies the behavior of rte_event_dev_stop().

This follows this mailing list discussion:
http://dpdk.org/ml/archives/dev/2018-January/087484.html

Signed-off-by: Gage Eads <gage.eads@intel.com>
---
v2: allow a NULL callback pointer to unregister the callback
v3: move the callback pointer to struct rte_eventdev_ops and
    the user argument to struct rte_eventdev_data.

 drivers/event/dpaa/dpaa_eventdev.c           |  3 +-
 drivers/event/dpaa2/dpaa2_eventdev.c         |  3 +-
 drivers/event/octeontx/ssovf_evdev.c         |  6 ++-
 drivers/event/opdl/opdl_evdev.c              |  4 +-
 drivers/event/skeleton/skeleton_eventdev.c   |  5 ++-
 drivers/event/sw/sw_evdev.c                  |  4 +-
 lib/librte_eventdev/rte_eventdev.c           | 17 +++++++++
 lib/librte_eventdev/rte_eventdev.h           | 55 ++++++++++++++++++++++++++--
 lib/librte_eventdev/rte_eventdev_pmd.h       |  3 ++
 lib/librte_eventdev/rte_eventdev_version.map |  6 +++
 10 files changed, 95 insertions(+), 11 deletions(-)

diff --git a/drivers/event/dpaa/dpaa_eventdev.c b/drivers/event/dpaa/dpaa_eventdev.c
index 0006801..4bf1918 100644
--- a/drivers/event/dpaa/dpaa_eventdev.c
+++ b/drivers/event/dpaa/dpaa_eventdev.c
@@ -571,7 +571,7 @@ dpaa_event_eth_rx_adapter_stop(const struct rte_eventdev *dev,
 	return 0;
 }
 
-static const struct rte_eventdev_ops dpaa_eventdev_ops = {
+static struct rte_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,
@@ -591,6 +591,7 @@ static const struct rte_eventdev_ops dpaa_eventdev_ops = {
 	.eth_rx_adapter_queue_del = dpaa_event_eth_rx_adapter_queue_del,
 	.eth_rx_adapter_start = dpaa_event_eth_rx_adapter_start,
 	.eth_rx_adapter_stop = dpaa_event_eth_rx_adapter_stop,
+	.dev_stop_flush   = NULL,
 };
 
 static int
diff --git a/drivers/event/dpaa2/dpaa2_eventdev.c b/drivers/event/dpaa2/dpaa2_eventdev.c
index c3e6fbf..363837a 100644
--- a/drivers/event/dpaa2/dpaa2_eventdev.c
+++ b/drivers/event/dpaa2/dpaa2_eventdev.c
@@ -693,7 +693,7 @@ dpaa2_eventdev_eth_stop(const struct rte_eventdev *dev,
 	return 0;
 }
 
-static const struct rte_eventdev_ops dpaa2_eventdev_ops = {
+static struct rte_eventdev_ops dpaa2_eventdev_ops = {
 	.dev_infos_get    = dpaa2_eventdev_info_get,
 	.dev_configure    = dpaa2_eventdev_configure,
 	.dev_start        = dpaa2_eventdev_start,
@@ -714,6 +714,7 @@ static const struct rte_eventdev_ops dpaa2_eventdev_ops = {
 	.eth_rx_adapter_queue_del = dpaa2_eventdev_eth_queue_del,
 	.eth_rx_adapter_start = dpaa2_eventdev_eth_start,
 	.eth_rx_adapter_stop = dpaa2_eventdev_eth_stop,
+	.dev_stop_flush   = NULL,
 };
 
 static int
diff --git a/drivers/event/octeontx/ssovf_evdev.c b/drivers/event/octeontx/ssovf_evdev.c
index a108607..713983b 100644
--- a/drivers/event/octeontx/ssovf_evdev.c
+++ b/drivers/event/octeontx/ssovf_evdev.c
@@ -591,7 +591,7 @@ ssovf_selftest(const char *key __rte_unused, const char *value,
 }
 
 /* Initialize and register event driver with DPDK Application */
-static const struct rte_eventdev_ops ssovf_ops = {
+static struct rte_eventdev_ops ssovf_ops = {
 	.dev_infos_get    = ssovf_info_get,
 	.dev_configure    = ssovf_configure,
 	.queue_def_conf   = ssovf_queue_def_conf,
@@ -615,7 +615,9 @@ static const struct rte_eventdev_ops ssovf_ops = {
 	.dump             = ssovf_dump,
 	.dev_start        = ssovf_start,
 	.dev_stop         = ssovf_stop,
-	.dev_close        = ssovf_close
+	.dev_close        = ssovf_close,
+
+	.dev_stop_flush   = NULL
 };
 
 static int
diff --git a/drivers/event/opdl/opdl_evdev.c b/drivers/event/opdl/opdl_evdev.c
index 7708369..477e102 100644
--- a/drivers/event/opdl/opdl_evdev.c
+++ b/drivers/event/opdl/opdl_evdev.c
@@ -607,7 +607,7 @@ set_do_test(const char *key __rte_unused, const char *value, void *opaque)
 static int
 opdl_probe(struct rte_vdev_device *vdev)
 {
-	static const struct rte_eventdev_ops evdev_opdl_ops = {
+	static struct rte_eventdev_ops evdev_opdl_ops = {
 		.dev_configure = opdl_dev_configure,
 		.dev_infos_get = opdl_info_get,
 		.dev_close = opdl_close,
@@ -629,6 +629,8 @@ opdl_probe(struct rte_vdev_device *vdev)
 		.xstats_get_names = opdl_xstats_get_names,
 		.xstats_get_by_name = opdl_xstats_get_by_name,
 		.xstats_reset = opdl_xstats_reset,
+
+		.dev_stop_flush = NULL,
 	};
 
 	static const char *const args[] = {
diff --git a/drivers/event/skeleton/skeleton_eventdev.c b/drivers/event/skeleton/skeleton_eventdev.c
index 7f46756..c627f01 100644
--- a/drivers/event/skeleton/skeleton_eventdev.c
+++ b/drivers/event/skeleton/skeleton_eventdev.c
@@ -319,7 +319,7 @@ skeleton_eventdev_dump(struct rte_eventdev *dev, FILE *f)
 
 
 /* Initialize and register event driver with DPDK Application */
-static const struct rte_eventdev_ops skeleton_eventdev_ops = {
+static struct rte_eventdev_ops skeleton_eventdev_ops = {
 	.dev_infos_get    = skeleton_eventdev_info_get,
 	.dev_configure    = skeleton_eventdev_configure,
 	.dev_start        = skeleton_eventdev_start,
@@ -334,7 +334,8 @@ static const struct rte_eventdev_ops skeleton_eventdev_ops = {
 	.port_link        = skeleton_eventdev_port_link,
 	.port_unlink      = skeleton_eventdev_port_unlink,
 	.timeout_ticks    = skeleton_eventdev_timeout_ticks,
-	.dump             = skeleton_eventdev_dump
+	.dump             = skeleton_eventdev_dump,
+	.dev_stop_flush   = NULL
 };
 
 static int
diff --git a/drivers/event/sw/sw_evdev.c b/drivers/event/sw/sw_evdev.c
index 6672fd8..bda2e21 100644
--- a/drivers/event/sw/sw_evdev.c
+++ b/drivers/event/sw/sw_evdev.c
@@ -772,7 +772,7 @@ static int32_t sw_sched_service_func(void *args)
 static int
 sw_probe(struct rte_vdev_device *vdev)
 {
-	static const struct rte_eventdev_ops evdev_sw_ops = {
+	static struct rte_eventdev_ops evdev_sw_ops = {
 			.dev_configure = sw_dev_configure,
 			.dev_infos_get = sw_info_get,
 			.dev_close = sw_close,
@@ -797,6 +797,8 @@ sw_probe(struct rte_vdev_device *vdev)
 			.xstats_reset = sw_xstats_reset,
 
 			.dev_selftest = test_sw_eventdev,
+
+			.dev_stop_flush = NULL,
 	};
 
 	static const char *const args[] = {
diff --git a/lib/librte_eventdev/rte_eventdev.c b/lib/librte_eventdev/rte_eventdev.c
index 851a119..2de8d9a 100644
--- a/lib/librte_eventdev/rte_eventdev.c
+++ b/lib/librte_eventdev/rte_eventdev.c
@@ -1123,6 +1123,23 @@ rte_event_dev_start(uint8_t dev_id)
 	return 0;
 }
 
+int
+rte_event_dev_stop_flush_callback_register(uint8_t dev_id,
+		eventdev_stop_flush_t callback, void *userdata)
+{
+	struct rte_eventdev *dev;
+
+	RTE_EDEV_LOG_DEBUG("Stop flush register dev_id=%" PRIu8, dev_id);
+
+	RTE_EVENTDEV_VALID_DEVID_OR_ERR_RET(dev_id, -EINVAL);
+	dev = &rte_eventdevs[dev_id];
+
+	dev->dev_ops->dev_stop_flush = callback;
+	dev->data->dev_stop_flush_arg = userdata;
+
+	return 0;
+}
+
 void
 rte_event_dev_stop(uint8_t dev_id)
 {
diff --git a/lib/librte_eventdev/rte_eventdev.h b/lib/librte_eventdev/rte_eventdev.h
index b21c271..197b059 100644
--- a/lib/librte_eventdev/rte_eventdev.h
+++ b/lib/librte_eventdev/rte_eventdev.h
@@ -244,6 +244,7 @@ extern "C" {
 #include <rte_errno.h>
 
 struct rte_mbuf; /* we just use mbuf pointers; no need to include rte_mbuf.h */
+struct rte_event;
 
 /* Event device capability bitmap flags */
 #define RTE_EVENT_DEV_CAP_QUEUE_QOS           (1ULL << 0)
@@ -835,15 +836,61 @@ int
 rte_event_dev_start(uint8_t dev_id);
 
 /**
- * Stop an event device. The device can be restarted with a call to
- * rte_event_dev_start()
+ * Stop an event device.
+ *
+ * This function causes all queued events to be drained. While draining events
+ * out of the device, this function calls the user-provided flush callback
+ * (if one was registered) once per event.
+ *
+ * This function does not drain events from event ports; the application is
+ * responsible for flushing events from all ports before stopping the device.
+ *
+ * The device can be restarted with a call to rte_event_dev_start(). Threads
+ * that continue to enqueue/dequeue while the device is stopped, or being
+ * stopped, will result in undefined behavior.
  *
  * @param dev_id
  *   Event device identifier.
+ *
+ * @see rte_event_dev_stop_flush_callback_register()
  */
 void
 rte_event_dev_stop(uint8_t dev_id);
 
+typedef void (*eventdev_stop_flush_t)(uint8_t dev_id, struct rte_event event,
+		void *arg);
+/**< Callback function called during rte_event_dev_stop(), invoked once per
+ * flushed event.
+ */
+
+/**
+ * Registers a callback function to be invoked during rte_event_dev_stop() for
+ * each flushed event. This function can be used to properly dispose of queued
+ * events, for example events containing memory pointers.
+ *
+ * The callback function is only registered for the calling process. The
+ * callback function must be registered in every process that can call
+ * rte_event_dev_stop().
+ *
+ * To unregister a callback, call this function with a NULL callback pointer.
+ *
+ * @param dev_id
+ *   The identifier of the device.
+ * @param callback
+ *   Callback function invoked once per flushed event.
+ * @param userdata
+ *   Argument supplied to callback.
+ *
+ * @return
+ *  - 0 on success.
+ *  - -EINVAL if *dev_id* is invalid
+ *
+ * @see rte_event_dev_stop()
+ */
+int
+rte_event_dev_stop_flush_callback_register(uint8_t dev_id,
+		eventdev_stop_flush_t callback, void *userdata);
+
 /**
  * Close an event device. The device cannot be restarted!
  *
@@ -1152,6 +1199,8 @@ struct rte_eventdev_data {
 	/* Service initialization state */
 	uint32_t service_id;
 	/* Service ID*/
+	void *dev_stop_flush_arg;
+	/**< User-provided argument for event flush function */
 
 	RTE_STD_C11
 	uint8_t dev_started : 1;
@@ -1178,7 +1227,7 @@ struct rte_eventdev {
 
 	struct rte_eventdev_data *data;
 	/**< Pointer to device data */
-	const struct rte_eventdev_ops *dev_ops;
+	struct rte_eventdev_ops *dev_ops;
 	/**< Functions exported by PMD */
 	struct rte_device *dev;
 	/**< Device info. supplied by probing */
diff --git a/lib/librte_eventdev/rte_eventdev_pmd.h b/lib/librte_eventdev/rte_eventdev_pmd.h
index 31343b5..3a8ddd7 100644
--- a/lib/librte_eventdev/rte_eventdev_pmd.h
+++ b/lib/librte_eventdev/rte_eventdev_pmd.h
@@ -642,6 +642,9 @@ struct rte_eventdev_ops {
 
 	eventdev_selftest dev_selftest;
 	/**< Start eventdev Selftest */
+
+	eventdev_stop_flush_t dev_stop_flush;
+	/**< User-provided event flush function */
 };
 
 /**
diff --git a/lib/librte_eventdev/rte_eventdev_version.map b/lib/librte_eventdev/rte_eventdev_version.map
index 2aef470..4396536 100644
--- a/lib/librte_eventdev/rte_eventdev_version.map
+++ b/lib/librte_eventdev/rte_eventdev_version.map
@@ -74,3 +74,9 @@ DPDK_18.02 {
 
 	rte_event_dev_selftest;
 } DPDK_17.11;
+
+DPDK_18.05 {
+	global:
+
+	rte_event_dev_stop_flush_callback_register;
+} DPDK_18.02;
-- 
2.7.4

[PATCH v3 2/2] event/sw: support device stop flush callback

[Gage Eads]

This commit also adds a flush callback test to the sw eventdev's selftest
suite.

Signed-off-by: Gage Eads <gage.eads@intel.com>
---
 drivers/event/sw/sw_evdev.c          | 25 ++++++++++-
 drivers/event/sw/sw_evdev_selftest.c | 80 +++++++++++++++++++++++++++++++++++-
 2 files changed, 102 insertions(+), 3 deletions(-)

diff --git a/drivers/event/sw/sw_evdev.c b/drivers/event/sw/sw_evdev.c
index bda2e21..8d7fe8a 100644
--- a/drivers/event/sw/sw_evdev.c
+++ b/drivers/event/sw/sw_evdev.c
@@ -362,8 +362,25 @@ sw_init_qid_iqs(struct sw_evdev *sw)
 }
 
 static void
-sw_clean_qid_iqs(struct sw_evdev *sw)
+sw_flush_iq(struct rte_eventdev *dev, struct sw_iq *iq)
 {
+	struct sw_evdev *sw = sw_pmd_priv(dev);
+
+	while (iq_count(iq) > 0) {
+		struct rte_event event;
+
+		iq_dequeue_burst(sw, iq, &event, 1);
+
+		dev->dev_ops->dev_stop_flush(dev->data->dev_id,
+					     event,
+					     dev->data->dev_stop_flush_arg);
+	}
+}
+
+static void
+sw_clean_qid_iqs(struct rte_eventdev *dev)
+{
+	struct sw_evdev *sw = sw_pmd_priv(dev);
 	int i, j;
 
 	/* Release the IQ memory of all configured qids */
@@ -373,7 +390,11 @@ sw_clean_qid_iqs(struct sw_evdev *sw)
 		for (j = 0; j < SW_IQS_MAX; j++) {
 			if (!qid->iq[j].head)
 				continue;
+
+			if (dev->dev_ops->dev_stop_flush)
+				sw_flush_iq(dev, &qid->iq[j]);
 			iq_free_chunk_list(sw, qid->iq[j].head);
+
 			qid->iq[j].head = NULL;
 		}
 	}
@@ -702,7 +723,7 @@ static void
 sw_stop(struct rte_eventdev *dev)
 {
 	struct sw_evdev *sw = sw_pmd_priv(dev);
-	sw_clean_qid_iqs(sw);
+	sw_clean_qid_iqs(dev);
 	sw_xstats_uninit(sw);
 	sw->started = 0;
 	rte_smp_wmb();
diff --git a/drivers/event/sw/sw_evdev_selftest.c b/drivers/event/sw/sw_evdev_selftest.c
index 78d30e0..0d3b998 100644
--- a/drivers/event/sw/sw_evdev_selftest.c
+++ b/drivers/event/sw/sw_evdev_selftest.c
@@ -28,6 +28,7 @@
 #define MAX_PORTS 16
 #define MAX_QIDS 16
 #define NUM_PACKETS (1<<18)
+#define DEQUEUE_DEPTH 128
 
 static int evdev;
 
@@ -147,7 +148,7 @@ init(struct test *t, int nb_queues, int nb_ports)
 			.nb_event_ports = nb_ports,
 			.nb_event_queue_flows = 1024,
 			.nb_events_limit = 4096,
-			.nb_event_port_dequeue_depth = 128,
+			.nb_event_port_dequeue_depth = DEQUEUE_DEPTH,
 			.nb_event_port_enqueue_depth = 128,
 	};
 	int ret;
@@ -2807,6 +2808,77 @@ holb(struct test *t) /* test to check we avoid basic head-of-line blocking */
 	return -1;
 }
 
+static void
+flush(uint8_t dev_id __rte_unused, struct rte_event event, void *arg)
+{
+	*((uint8_t *) arg) += (event.u64 == 0xCA11BACC) ? 1 : 0;
+}
+
+static int
+dev_stop_flush(struct test *t) /* test to check we can properly flush events */
+{
+	const struct rte_event new_ev = {
+			.op = RTE_EVENT_OP_NEW,
+			.u64 = 0xCA11BACC
+			/* all other fields zero */
+	};
+	struct rte_event ev = new_ev;
+	uint8_t count = 0;
+	int i;
+
+	if (init(t, 1, 1) < 0 ||
+	    create_ports(t, 1) < 0 ||
+	    create_atomic_qids(t, 1) < 0) {
+		printf("%d: Error initializing device\n", __LINE__);
+		return -1;
+	}
+
+	/* Link the queue so *_start() doesn't error out */
+	if (rte_event_port_link(evdev, t->port[0], NULL, NULL, 0) != 1) {
+		printf("%d: Error linking queue to port\n", __LINE__);
+		goto err;
+	}
+
+	if (rte_event_dev_start(evdev) < 0) {
+		printf("%d: Error with start call\n", __LINE__);
+		goto err;
+	}
+
+	for (i = 0; i < DEQUEUE_DEPTH + 1; i++) {
+		if (rte_event_enqueue_burst(evdev, t->port[0], &ev, 1) != 1) {
+			printf("%d: Error enqueuing events\n", __LINE__);
+			goto err;
+		}
+	}
+
+	/* Schedule the events from the port to the IQ. At least one event
+	 * should be remaining in the queue.
+	 */
+	rte_service_run_iter_on_app_lcore(t->service_id, 1);
+
+	if (rte_event_dev_stop_flush_callback_register(evdev, flush, &count)) {
+		printf("%d: Error installing the flush callback\n", __LINE__);
+		goto err;
+	}
+
+	cleanup(t);
+
+	if (count == 0) {
+		printf("%d: Error executing the flush callback\n", __LINE__);
+		goto err;
+	}
+
+	if (rte_event_dev_stop_flush_callback_register(evdev, NULL, NULL)) {
+		printf("%d: Error uninstalling the flush callback\n", __LINE__);
+		goto err;
+	}
+
+	return 0;
+err:
+	rte_event_dev_dump(evdev, stdout);
+	cleanup(t);
+	return -1;
+}
 static int
 worker_loopback_worker_fn(void *arg)
 {
@@ -3211,6 +3283,12 @@ test_sw_eventdev(void)
 		printf("ERROR - Head-of-line-blocking test FAILED.\n");
 		goto test_fail;
 	}
+	printf("*** Running Stop Flush test...\n");
+	ret = dev_stop_flush(t);
+	if (ret != 0) {
+		printf("ERROR - Stop Flush test FAILED.\n");
+		goto test_fail;
+	}
 	if (rte_lcore_count() >= 3) {
 		printf("*** Running Worker loopback test...\n");
 		ret = worker_loopback(t, 0);
-- 
2.7.4

Re: [PATCH v3 1/2] eventdev: add device stop flush callback

[Jerin Jacob]

-----Original Message-----
> Date: Wed, 14 Mar 2018 23:12:09 -0500
> From: Gage Eads <gage.eads@intel.com>
> To: dev@dpdk.org
> CC: jerin.jacob@caviumnetworks.com, harry.van.haaren@intel.com,
>  hemant.agrawal@nxp.com, bruce.richardson@intel.com,
>  santosh.shukla@caviumnetworks.com, nipun.gupta@nxp.com
> Subject: [PATCH v3 1/2] eventdev: add device stop flush callback
> X-Mailer: git-send-email 2.7.4
> 
> When an event device is stopped, it drains all event queues. These events
> may contain pointers, so to prevent memory leaks eventdev now supports a
> user-provided flush callback that is called during the queue drain process.
> This callback is stored in process memory, so the callback must be
> registered by any process that may call rte_event_dev_stop().
> 
> This commit also clarifies the behavior of rte_event_dev_stop().
> 
> This follows this mailing list discussion:
> http://dpdk.org/ml/archives/dev/2018-January/087484.html
> 
> Signed-off-by: Gage Eads <gage.eads@intel.com>
> ---
> v2: allow a NULL callback pointer to unregister the callback
> v3: move the callback pointer to struct rte_eventdev_ops and
>     the user argument to struct rte_eventdev_data.
> 
>  drivers/event/dpaa/dpaa_eventdev.c           |  3 +-
>  drivers/event/dpaa2/dpaa2_eventdev.c         |  3 +-
>  drivers/event/octeontx/ssovf_evdev.c         |  6 ++-
>  drivers/event/opdl/opdl_evdev.c              |  4 +-
>  drivers/event/skeleton/skeleton_eventdev.c   |  5 ++-
>  drivers/event/sw/sw_evdev.c                  |  4 +-
>  lib/librte_eventdev/rte_eventdev.c           | 17 +++++++++
>  lib/librte_eventdev/rte_eventdev.h           | 55 ++++++++++++++++++++++++++--
>  lib/librte_eventdev/rte_eventdev_pmd.h       |  3 ++
>  lib/librte_eventdev/rte_eventdev_version.map |  6 +++
>  10 files changed, 95 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/event/dpaa/dpaa_eventdev.c b/drivers/event/dpaa/dpaa_eventdev.c
> index 0006801..4bf1918 100644
> --- a/drivers/event/dpaa/dpaa_eventdev.c
> +++ b/drivers/event/dpaa/dpaa_eventdev.c
> @@ -571,7 +571,7 @@ dpaa_event_eth_rx_adapter_stop(const struct rte_eventdev *dev,
>  	return 0;
>  }
>  
> -static const struct rte_eventdev_ops dpaa_eventdev_ops = {
> +static struct rte_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,
> @@ -591,6 +591,7 @@ static const struct rte_eventdev_ops dpaa_eventdev_ops = {
>  	.eth_rx_adapter_queue_del = dpaa_event_eth_rx_adapter_queue_del,
>  	.eth_rx_adapter_start = dpaa_event_eth_rx_adapter_start,
>  	.eth_rx_adapter_stop = dpaa_event_eth_rx_adapter_stop,
> +	.dev_stop_flush   = NULL,

Do we need explicit NULL assignment here. It will be NULL by default.

>  };
>  
>  static int
> diff --git a/drivers/event/dpaa2/dpaa2_eventdev.c b/drivers/event/dpaa2/dpaa2_eventdev.c
> index c3e6fbf..363837a 100644
> --- a/drivers/event/dpaa2/dpaa2_eventdev.c
> +++ b/drivers/event/dpaa2/dpaa2_eventdev.c
> @@ -693,7 +693,7 @@ dpaa2_eventdev_eth_stop(const struct rte_eventdev *dev,
>  	return 0;
>  }
>  
> -static const struct rte_eventdev_ops dpaa2_eventdev_ops = {
> +static struct rte_eventdev_ops dpaa2_eventdev_ops = {
>  	.dev_infos_get    = dpaa2_eventdev_info_get,
>  	.dev_configure    = dpaa2_eventdev_configure,
>  	.dev_start        = dpaa2_eventdev_start,
> @@ -714,6 +714,7 @@ static const struct rte_eventdev_ops dpaa2_eventdev_ops = {
>  	.eth_rx_adapter_queue_del = dpaa2_eventdev_eth_queue_del,
>  	.eth_rx_adapter_start = dpaa2_eventdev_eth_start,
>  	.eth_rx_adapter_stop = dpaa2_eventdev_eth_stop,
> +	.dev_stop_flush   = NULL,

Same as above comment.

>  };
>  
>  static int
> diff --git a/drivers/event/octeontx/ssovf_evdev.c b/drivers/event/octeontx/ssovf_evdev.c
> index a108607..713983b 100644
> --- a/drivers/event/octeontx/ssovf_evdev.c
> +++ b/drivers/event/octeontx/ssovf_evdev.c
> @@ -591,7 +591,7 @@ ssovf_selftest(const char *key __rte_unused, const char *value,
>  }
>  
>  /* Initialize and register event driver with DPDK Application */
> -static const struct rte_eventdev_ops ssovf_ops = {
> +static struct rte_eventdev_ops ssovf_ops = {
>  	.dev_infos_get    = ssovf_info_get,
>  	.dev_configure    = ssovf_configure,
>  	.queue_def_conf   = ssovf_queue_def_conf,
> @@ -615,7 +615,9 @@ static const struct rte_eventdev_ops ssovf_ops = {
>  	.dump             = ssovf_dump,
>  	.dev_start        = ssovf_start,
>  	.dev_stop         = ssovf_stop,
> -	.dev_close        = ssovf_close
> +	.dev_close        = ssovf_close,
> +
> +	.dev_stop_flush   = NULL

Same as above comment.

>  };
>  
>  static int
> diff --git a/drivers/event/opdl/opdl_evdev.c b/drivers/event/opdl/opdl_evdev.c
> index 7708369..477e102 100644
> --- a/drivers/event/opdl/opdl_evdev.c
> +++ b/drivers/event/opdl/opdl_evdev.c
> @@ -607,7 +607,7 @@ set_do_test(const char *key __rte_unused, const char *value, void *opaque)
>  static int
>  opdl_probe(struct rte_vdev_device *vdev)
>  {
> -	static const struct rte_eventdev_ops evdev_opdl_ops = {
> +	static struct rte_eventdev_ops evdev_opdl_ops = {
>  		.dev_configure = opdl_dev_configure,
>  		.dev_infos_get = opdl_info_get,
>  		.dev_close = opdl_close,
> @@ -629,6 +629,8 @@ opdl_probe(struct rte_vdev_device *vdev)
>  		.xstats_get_names = opdl_xstats_get_names,
>  		.xstats_get_by_name = opdl_xstats_get_by_name,
>  		.xstats_reset = opdl_xstats_reset,
> +
> +		.dev_stop_flush = NULL,

Same as above comment.

>  	};
>  
>  	static const char *const args[] = {
> diff --git a/drivers/event/skeleton/skeleton_eventdev.c b/drivers/event/skeleton/skeleton_eventdev.c
> index 7f46756..c627f01 100644
> --- a/drivers/event/skeleton/skeleton_eventdev.c
> +++ b/drivers/event/skeleton/skeleton_eventdev.c
> @@ -319,7 +319,7 @@ skeleton_eventdev_dump(struct rte_eventdev *dev, FILE *f)
>  
>  
>  /* Initialize and register event driver with DPDK Application */
> -static const struct rte_eventdev_ops skeleton_eventdev_ops = {
> +static struct rte_eventdev_ops skeleton_eventdev_ops = {
>  	.dev_infos_get    = skeleton_eventdev_info_get,
>  	.dev_configure    = skeleton_eventdev_configure,
>  	.dev_start        = skeleton_eventdev_start,
> @@ -334,7 +334,8 @@ static const struct rte_eventdev_ops skeleton_eventdev_ops = {
>  	.port_link        = skeleton_eventdev_port_link,
>  	.port_unlink      = skeleton_eventdev_port_unlink,
>  	.timeout_ticks    = skeleton_eventdev_timeout_ticks,
> -	.dump             = skeleton_eventdev_dump
> +	.dump             = skeleton_eventdev_dump,
> +	.dev_stop_flush   = NULL

Same as above comment.

>  };
>  
>  static int
> diff --git a/drivers/event/sw/sw_evdev.c b/drivers/event/sw/sw_evdev.c
> index 6672fd8..bda2e21 100644
> --- a/drivers/event/sw/sw_evdev.c
> +++ b/drivers/event/sw/sw_evdev.c
> @@ -772,7 +772,7 @@ static int32_t sw_sched_service_func(void *args)
>  static int
>  sw_probe(struct rte_vdev_device *vdev)
>  {
> -	static const struct rte_eventdev_ops evdev_sw_ops = {
> +	static struct rte_eventdev_ops evdev_sw_ops = {
>  			.dev_configure = sw_dev_configure,
>  			.dev_infos_get = sw_info_get,
>  			.dev_close = sw_close,
> @@ -797,6 +797,8 @@ sw_probe(struct rte_vdev_device *vdev)
>  			.xstats_reset = sw_xstats_reset,
>  
>  			.dev_selftest = test_sw_eventdev,
> +
> +			.dev_stop_flush = NULL,

Same as above comment.

>  	};
>  
>  	static const char *const args[] = {
> diff --git a/lib/librte_eventdev/rte_eventdev.c b/lib/librte_eventdev/rte_eventdev.c
> index 851a119..2de8d9a 100644
> --- a/lib/librte_eventdev/rte_eventdev.c
> +++ b/lib/librte_eventdev/rte_eventdev.c
> @@ -1123,6 +1123,23 @@ rte_event_dev_start(uint8_t dev_id)
>  	return 0;
>  }
>  
> +int
> +rte_event_dev_stop_flush_callback_register(uint8_t dev_id,
> +		eventdev_stop_flush_t callback, void *userdata)
> +{
> +	struct rte_eventdev *dev;
> +
> +	RTE_EDEV_LOG_DEBUG("Stop flush register dev_id=%" PRIu8, dev_id);
> +
> +	RTE_EVENTDEV_VALID_DEVID_OR_ERR_RET(dev_id, -EINVAL);
> +	dev = &rte_eventdevs[dev_id];
> +
> +	dev->dev_ops->dev_stop_flush = callback;
> +	dev->data->dev_stop_flush_arg = userdata;
> +
> +	return 0;
> +}

Other than above minor comments, everything else looks good to me.

Re: [PATCH v3 1/2] eventdev: add device stop flush callback

[Eads, Gage]

> -----Original Message-----
> From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> Sent: Tuesday, March 20, 2018 2:44 AM
> To: Eads, Gage <gage.eads@intel.com>
> Cc: dev@dpdk.org; Van Haaren, Harry <harry.van.haaren@intel.com>;
> hemant.agrawal@nxp.com; Richardson, Bruce <bruce.richardson@intel.com>;
> santosh.shukla@caviumnetworks.com; nipun.gupta@nxp.com
> Subject: Re: [PATCH v3 1/2] eventdev: add device stop flush callback
> 
> -----Original Message-----
> > Date: Wed, 14 Mar 2018 23:12:09 -0500
> > From: Gage Eads <gage.eads@intel.com>
> > To: dev@dpdk.org
> > CC: jerin.jacob@caviumnetworks.com, harry.van.haaren@intel.com,
> > hemant.agrawal@nxp.com, bruce.richardson@intel.com,
> > santosh.shukla@caviumnetworks.com, nipun.gupta@nxp.com
> > Subject: [PATCH v3 1/2] eventdev: add device stop flush callback
> > X-Mailer: git-send-email 2.7.4
> >
> > When an event device is stopped, it drains all event queues. These
> > events may contain pointers, so to prevent memory leaks eventdev now
> > supports a user-provided flush callback that is called during the queue drain
> process.
> > This callback is stored in process memory, so the callback must be
> > registered by any process that may call rte_event_dev_stop().
> >
> > This commit also clarifies the behavior of rte_event_dev_stop().
> >
> > This follows this mailing list discussion:
> > http://dpdk.org/ml/archives/dev/2018-January/087484.html
> >
> > Signed-off-by: Gage Eads <gage.eads@intel.com>
> > ---
> > v2: allow a NULL callback pointer to unregister the callback
> > v3: move the callback pointer to struct rte_eventdev_ops and
> >     the user argument to struct rte_eventdev_data.
> >
> >  drivers/event/dpaa/dpaa_eventdev.c           |  3 +-
> >  drivers/event/dpaa2/dpaa2_eventdev.c         |  3 +-
> >  drivers/event/octeontx/ssovf_evdev.c         |  6 ++-
> >  drivers/event/opdl/opdl_evdev.c              |  4 +-
> >  drivers/event/skeleton/skeleton_eventdev.c   |  5 ++-
> >  drivers/event/sw/sw_evdev.c                  |  4 +-
> >  lib/librte_eventdev/rte_eventdev.c           | 17 +++++++++
> >  lib/librte_eventdev/rte_eventdev.h           | 55
> ++++++++++++++++++++++++++--
> >  lib/librte_eventdev/rte_eventdev_pmd.h       |  3 ++
> >  lib/librte_eventdev/rte_eventdev_version.map |  6 +++
> >  10 files changed, 95 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/event/dpaa/dpaa_eventdev.c
> > b/drivers/event/dpaa/dpaa_eventdev.c
> > index 0006801..4bf1918 100644
> > --- a/drivers/event/dpaa/dpaa_eventdev.c
> > +++ b/drivers/event/dpaa/dpaa_eventdev.c
> > @@ -571,7 +571,7 @@ dpaa_event_eth_rx_adapter_stop(const struct
> rte_eventdev *dev,
> >  	return 0;
> >  }
> >
> > -static const struct rte_eventdev_ops dpaa_eventdev_ops = {
> > +static struct rte_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,
> > @@ -591,6 +591,7 @@ static const struct rte_eventdev_ops
> dpaa_eventdev_ops = {
> >  	.eth_rx_adapter_queue_del = dpaa_event_eth_rx_adapter_queue_del,
> >  	.eth_rx_adapter_start = dpaa_event_eth_rx_adapter_start,
> >  	.eth_rx_adapter_stop = dpaa_event_eth_rx_adapter_stop,
> > +	.dev_stop_flush   = NULL,
> 
> Do we need explicit NULL assignment here. It will be NULL by default.
> 

Good point. I will fix and resubmit a v4.

[PATCH v4 1/2] eventdev: add device stop flush callback

[Gage Eads]

When an event device is stopped, it drains all event queues. These events
may contain pointers, so to prevent memory leaks eventdev now supports a
user-provided flush callback that is called during the queue drain process.
This callback is stored in process memory, so the callback must be
registered by any process that may call rte_event_dev_stop().

This commit also clarifies the behavior of rte_event_dev_stop().

This follows this mailing list discussion:
http://dpdk.org/ml/archives/dev/2018-January/087484.html

Signed-off-by: Gage Eads <gage.eads@intel.com>
---
v2: allow a NULL callback pointer to unregister the callback
v3: move the callback pointer to struct rte_eventdev_ops and
    the user argument to struct rte_eventdev_data
v4: remove redundant callback NULL initialization

 drivers/event/dpaa/dpaa_eventdev.c           |  2 +-
 drivers/event/dpaa2/dpaa2_eventdev.c         |  2 +-
 drivers/event/octeontx/ssovf_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/librte_eventdev/rte_eventdev.c           | 17 +++++++++
 lib/librte_eventdev/rte_eventdev.h           | 55 ++++++++++++++++++++++++++--
 lib/librte_eventdev/rte_eventdev_pmd.h       |  3 ++
 lib/librte_eventdev/rte_eventdev_version.map |  6 +++
 10 files changed, 84 insertions(+), 9 deletions(-)

diff --git a/drivers/event/dpaa/dpaa_eventdev.c b/drivers/event/dpaa/dpaa_eventdev.c
index 0006801..fcb648d 100644
--- a/drivers/event/dpaa/dpaa_eventdev.c
+++ b/drivers/event/dpaa/dpaa_eventdev.c
@@ -571,7 +571,7 @@ dpaa_event_eth_rx_adapter_stop(const struct rte_eventdev *dev,
 	return 0;
 }
 
-static const struct rte_eventdev_ops dpaa_eventdev_ops = {
+static struct rte_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 c3e6fbf..5270dc0 100644
--- a/drivers/event/dpaa2/dpaa2_eventdev.c
+++ b/drivers/event/dpaa2/dpaa2_eventdev.c
@@ -693,7 +693,7 @@ dpaa2_eventdev_eth_stop(const struct rte_eventdev *dev,
 	return 0;
 }
 
-static const struct rte_eventdev_ops dpaa2_eventdev_ops = {
+static struct rte_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/octeontx/ssovf_evdev.c b/drivers/event/octeontx/ssovf_evdev.c
index a108607..bc1f05f 100644
--- a/drivers/event/octeontx/ssovf_evdev.c
+++ b/drivers/event/octeontx/ssovf_evdev.c
@@ -591,7 +591,7 @@ ssovf_selftest(const char *key __rte_unused, const char *value,
 }
 
 /* Initialize and register event driver with DPDK Application */
-static const struct rte_eventdev_ops ssovf_ops = {
+static struct rte_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/opdl/opdl_evdev.c b/drivers/event/opdl/opdl_evdev.c
index 7708369..ef9fb30 100644
--- a/drivers/event/opdl/opdl_evdev.c
+++ b/drivers/event/opdl/opdl_evdev.c
@@ -607,7 +607,7 @@ set_do_test(const char *key __rte_unused, const char *value, void *opaque)
 static int
 opdl_probe(struct rte_vdev_device *vdev)
 {
-	static const struct rte_eventdev_ops evdev_opdl_ops = {
+	static struct rte_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 7f46756..c889220 100644
--- a/drivers/event/skeleton/skeleton_eventdev.c
+++ b/drivers/event/skeleton/skeleton_eventdev.c
@@ -319,7 +319,7 @@ skeleton_eventdev_dump(struct rte_eventdev *dev, FILE *f)
 
 
 /* Initialize and register event driver with DPDK Application */
-static const struct rte_eventdev_ops skeleton_eventdev_ops = {
+static struct rte_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 6672fd8..0e89f11 100644
--- a/drivers/event/sw/sw_evdev.c
+++ b/drivers/event/sw/sw_evdev.c
@@ -772,7 +772,7 @@ static int32_t sw_sched_service_func(void *args)
 static int
 sw_probe(struct rte_vdev_device *vdev)
 {
-	static const struct rte_eventdev_ops evdev_sw_ops = {
+	static struct rte_eventdev_ops evdev_sw_ops = {
 			.dev_configure = sw_dev_configure,
 			.dev_infos_get = sw_info_get,
 			.dev_close = sw_close,
diff --git a/lib/librte_eventdev/rte_eventdev.c b/lib/librte_eventdev/rte_eventdev.c
index 851a119..2de8d9a 100644
--- a/lib/librte_eventdev/rte_eventdev.c
+++ b/lib/librte_eventdev/rte_eventdev.c
@@ -1123,6 +1123,23 @@ rte_event_dev_start(uint8_t dev_id)
 	return 0;
 }
 
+int
+rte_event_dev_stop_flush_callback_register(uint8_t dev_id,
+		eventdev_stop_flush_t callback, void *userdata)
+{
+	struct rte_eventdev *dev;
+
+	RTE_EDEV_LOG_DEBUG("Stop flush register dev_id=%" PRIu8, dev_id);
+
+	RTE_EVENTDEV_VALID_DEVID_OR_ERR_RET(dev_id, -EINVAL);
+	dev = &rte_eventdevs[dev_id];
+
+	dev->dev_ops->dev_stop_flush = callback;
+	dev->data->dev_stop_flush_arg = userdata;
+
+	return 0;
+}
+
 void
 rte_event_dev_stop(uint8_t dev_id)
 {
diff --git a/lib/librte_eventdev/rte_eventdev.h b/lib/librte_eventdev/rte_eventdev.h
index b21c271..197b059 100644
--- a/lib/librte_eventdev/rte_eventdev.h
+++ b/lib/librte_eventdev/rte_eventdev.h
@@ -244,6 +244,7 @@ extern "C" {
 #include <rte_errno.h>
 
 struct rte_mbuf; /* we just use mbuf pointers; no need to include rte_mbuf.h */
+struct rte_event;
 
 /* Event device capability bitmap flags */
 #define RTE_EVENT_DEV_CAP_QUEUE_QOS           (1ULL << 0)
@@ -835,15 +836,61 @@ int
 rte_event_dev_start(uint8_t dev_id);
 
 /**
- * Stop an event device. The device can be restarted with a call to
- * rte_event_dev_start()
+ * Stop an event device.
+ *
+ * This function causes all queued events to be drained. While draining events
+ * out of the device, this function calls the user-provided flush callback
+ * (if one was registered) once per event.
+ *
+ * This function does not drain events from event ports; the application is
+ * responsible for flushing events from all ports before stopping the device.
+ *
+ * The device can be restarted with a call to rte_event_dev_start(). Threads
+ * that continue to enqueue/dequeue while the device is stopped, or being
+ * stopped, will result in undefined behavior.
  *
  * @param dev_id
  *   Event device identifier.
+ *
+ * @see rte_event_dev_stop_flush_callback_register()
  */
 void
 rte_event_dev_stop(uint8_t dev_id);
 
+typedef void (*eventdev_stop_flush_t)(uint8_t dev_id, struct rte_event event,
+		void *arg);
+/**< Callback function called during rte_event_dev_stop(), invoked once per
+ * flushed event.
+ */
+
+/**
+ * Registers a callback function to be invoked during rte_event_dev_stop() for
+ * each flushed event. This function can be used to properly dispose of queued
+ * events, for example events containing memory pointers.
+ *
+ * The callback function is only registered for the calling process. The
+ * callback function must be registered in every process that can call
+ * rte_event_dev_stop().
+ *
+ * To unregister a callback, call this function with a NULL callback pointer.
+ *
+ * @param dev_id
+ *   The identifier of the device.
+ * @param callback
+ *   Callback function invoked once per flushed event.
+ * @param userdata
+ *   Argument supplied to callback.
+ *
+ * @return
+ *  - 0 on success.
+ *  - -EINVAL if *dev_id* is invalid
+ *
+ * @see rte_event_dev_stop()
+ */
+int
+rte_event_dev_stop_flush_callback_register(uint8_t dev_id,
+		eventdev_stop_flush_t callback, void *userdata);
+
 /**
  * Close an event device. The device cannot be restarted!
  *
@@ -1152,6 +1199,8 @@ struct rte_eventdev_data {
 	/* Service initialization state */
 	uint32_t service_id;
 	/* Service ID*/
+	void *dev_stop_flush_arg;
+	/**< User-provided argument for event flush function */
 
 	RTE_STD_C11
 	uint8_t dev_started : 1;
@@ -1178,7 +1227,7 @@ struct rte_eventdev {
 
 	struct rte_eventdev_data *data;
 	/**< Pointer to device data */
-	const struct rte_eventdev_ops *dev_ops;
+	struct rte_eventdev_ops *dev_ops;
 	/**< Functions exported by PMD */
 	struct rte_device *dev;
 	/**< Device info. supplied by probing */
diff --git a/lib/librte_eventdev/rte_eventdev_pmd.h b/lib/librte_eventdev/rte_eventdev_pmd.h
index 31343b5..3a8ddd7 100644
--- a/lib/librte_eventdev/rte_eventdev_pmd.h
+++ b/lib/librte_eventdev/rte_eventdev_pmd.h
@@ -642,6 +642,9 @@ struct rte_eventdev_ops {
 
 	eventdev_selftest dev_selftest;
 	/**< Start eventdev Selftest */
+
+	eventdev_stop_flush_t dev_stop_flush;
+	/**< User-provided event flush function */
 };
 
 /**
diff --git a/lib/librte_eventdev/rte_eventdev_version.map b/lib/librte_eventdev/rte_eventdev_version.map
index 2aef470..4396536 100644
--- a/lib/librte_eventdev/rte_eventdev_version.map
+++ b/lib/librte_eventdev/rte_eventdev_version.map
@@ -74,3 +74,9 @@ DPDK_18.02 {
 
 	rte_event_dev_selftest;
 } DPDK_17.11;
+
+DPDK_18.05 {
+	global:
+
+	rte_event_dev_stop_flush_callback_register;
+} DPDK_18.02;
-- 
2.7.4

[PATCH v4 2/2] event/sw: support device stop flush callback

[Gage Eads]

This commit also adds a flush callback test to the sw eventdev's selftest
suite.

Signed-off-by: Gage Eads <gage.eads@intel.com>
---
 drivers/event/sw/sw_evdev.c          | 25 ++++++++++-
 drivers/event/sw/sw_evdev_selftest.c | 80 +++++++++++++++++++++++++++++++++++-
 2 files changed, 102 insertions(+), 3 deletions(-)

diff --git a/drivers/event/sw/sw_evdev.c b/drivers/event/sw/sw_evdev.c
index 0e89f11..11f394f 100644
--- a/drivers/event/sw/sw_evdev.c
+++ b/drivers/event/sw/sw_evdev.c
@@ -362,8 +362,25 @@ sw_init_qid_iqs(struct sw_evdev *sw)
 }
 
 static void
-sw_clean_qid_iqs(struct sw_evdev *sw)
+sw_flush_iq(struct rte_eventdev *dev, struct sw_iq *iq)
 {
+	struct sw_evdev *sw = sw_pmd_priv(dev);
+
+	while (iq_count(iq) > 0) {
+		struct rte_event event;
+
+		iq_dequeue_burst(sw, iq, &event, 1);
+
+		dev->dev_ops->dev_stop_flush(dev->data->dev_id,
+					     event,
+					     dev->data->dev_stop_flush_arg);
+	}
+}
+
+static void
+sw_clean_qid_iqs(struct rte_eventdev *dev)
+{
+	struct sw_evdev *sw = sw_pmd_priv(dev);
 	int i, j;
 
 	/* Release the IQ memory of all configured qids */
@@ -373,7 +390,11 @@ sw_clean_qid_iqs(struct sw_evdev *sw)
 		for (j = 0; j < SW_IQS_MAX; j++) {
 			if (!qid->iq[j].head)
 				continue;
+
+			if (dev->dev_ops->dev_stop_flush)
+				sw_flush_iq(dev, &qid->iq[j]);
 			iq_free_chunk_list(sw, qid->iq[j].head);
+
 			qid->iq[j].head = NULL;
 		}
 	}
@@ -702,7 +723,7 @@ static void
 sw_stop(struct rte_eventdev *dev)
 {
 	struct sw_evdev *sw = sw_pmd_priv(dev);
-	sw_clean_qid_iqs(sw);
+	sw_clean_qid_iqs(dev);
 	sw_xstats_uninit(sw);
 	sw->started = 0;
 	rte_smp_wmb();
diff --git a/drivers/event/sw/sw_evdev_selftest.c b/drivers/event/sw/sw_evdev_selftest.c
index 78d30e0..0d3b998 100644
--- a/drivers/event/sw/sw_evdev_selftest.c
+++ b/drivers/event/sw/sw_evdev_selftest.c
@@ -28,6 +28,7 @@
 #define MAX_PORTS 16
 #define MAX_QIDS 16
 #define NUM_PACKETS (1<<18)
+#define DEQUEUE_DEPTH 128
 
 static int evdev;
 
@@ -147,7 +148,7 @@ init(struct test *t, int nb_queues, int nb_ports)
 			.nb_event_ports = nb_ports,
 			.nb_event_queue_flows = 1024,
 			.nb_events_limit = 4096,
-			.nb_event_port_dequeue_depth = 128,
+			.nb_event_port_dequeue_depth = DEQUEUE_DEPTH,
 			.nb_event_port_enqueue_depth = 128,
 	};
 	int ret;
@@ -2807,6 +2808,77 @@ holb(struct test *t) /* test to check we avoid basic head-of-line blocking */
 	return -1;
 }
 
+static void
+flush(uint8_t dev_id __rte_unused, struct rte_event event, void *arg)
+{
+	*((uint8_t *) arg) += (event.u64 == 0xCA11BACC) ? 1 : 0;
+}
+
+static int
+dev_stop_flush(struct test *t) /* test to check we can properly flush events */
+{
+	const struct rte_event new_ev = {
+			.op = RTE_EVENT_OP_NEW,
+			.u64 = 0xCA11BACC
+			/* all other fields zero */
+	};
+	struct rte_event ev = new_ev;
+	uint8_t count = 0;
+	int i;
+
+	if (init(t, 1, 1) < 0 ||
+	    create_ports(t, 1) < 0 ||
+	    create_atomic_qids(t, 1) < 0) {
+		printf("%d: Error initializing device\n", __LINE__);
+		return -1;
+	}
+
+	/* Link the queue so *_start() doesn't error out */
+	if (rte_event_port_link(evdev, t->port[0], NULL, NULL, 0) != 1) {
+		printf("%d: Error linking queue to port\n", __LINE__);
+		goto err;
+	}
+
+	if (rte_event_dev_start(evdev) < 0) {
+		printf("%d: Error with start call\n", __LINE__);
+		goto err;
+	}
+
+	for (i = 0; i < DEQUEUE_DEPTH + 1; i++) {
+		if (rte_event_enqueue_burst(evdev, t->port[0], &ev, 1) != 1) {
+			printf("%d: Error enqueuing events\n", __LINE__);
+			goto err;
+		}
+	}
+
+	/* Schedule the events from the port to the IQ. At least one event
+	 * should be remaining in the queue.
+	 */
+	rte_service_run_iter_on_app_lcore(t->service_id, 1);
+
+	if (rte_event_dev_stop_flush_callback_register(evdev, flush, &count)) {
+		printf("%d: Error installing the flush callback\n", __LINE__);
+		goto err;
+	}
+
+	cleanup(t);
+
+	if (count == 0) {
+		printf("%d: Error executing the flush callback\n", __LINE__);
+		goto err;
+	}
+
+	if (rte_event_dev_stop_flush_callback_register(evdev, NULL, NULL)) {
+		printf("%d: Error uninstalling the flush callback\n", __LINE__);
+		goto err;
+	}
+
+	return 0;
+err:
+	rte_event_dev_dump(evdev, stdout);
+	cleanup(t);
+	return -1;
+}
 static int
 worker_loopback_worker_fn(void *arg)
 {
@@ -3211,6 +3283,12 @@ test_sw_eventdev(void)
 		printf("ERROR - Head-of-line-blocking test FAILED.\n");
 		goto test_fail;
 	}
+	printf("*** Running Stop Flush test...\n");
+	ret = dev_stop_flush(t);
+	if (ret != 0) {
+		printf("ERROR - Stop Flush test FAILED.\n");
+		goto test_fail;
+	}
 	if (rte_lcore_count() >= 3) {
 		printf("*** Running Worker loopback test...\n");
 		ret = worker_loopback(t, 0);
-- 
2.7.4

Re: [PATCH v4 1/2] eventdev: add device stop flush callback

[Van Haaren, Harry]

> From: Eads, Gage
> Sent: Tuesday, March 20, 2018 2:13 PM
> To: dev@dpdk.org
> Cc: jerin.jacob@caviumnetworks.com; Van Haaren, Harry
> <harry.van.haaren@intel.com>; hemant.agrawal@nxp.com; Richardson, Bruce
> <bruce.richardson@intel.com>; santosh.shukla@caviumnetworks.com;
> nipun.gupta@nxp.com
> Subject: [PATCH v4 1/2] eventdev: add device stop flush callback
> 
> When an event device is stopped, it drains all event queues. These events
> may contain pointers, so to prevent memory leaks eventdev now supports a
> user-provided flush callback that is called during the queue drain process.
> This callback is stored in process memory, so the callback must be
> registered by any process that may call rte_event_dev_stop().
> 
> This commit also clarifies the behavior of rte_event_dev_stop().
> 
> This follows this mailing list discussion:
> http://dpdk.org/ml/archives/dev/2018-January/087484.html
> 
> Signed-off-by: Gage Eads <gage.eads@intel.com>

<snip most of the code - looks good!>

>  /**
> - * Stop an event device. The device can be restarted with a call to
> - * rte_event_dev_start()
> + * Stop an event device.
> + *
> + * This function causes all queued events to be drained. While draining
> events
> + * out of the device, this function calls the user-provided flush callback
> + * (if one was registered) once per event.
> + *
> + * This function does not drain events from event ports; the application is
> + * responsible for flushing events from all ports before stopping the
> device.


Question about how an application is expected to correctly cleanup all the
events here. Note in particular the last part: "application is responsible
for flushing events from all ports **BEFORE** stopping the device".

Given the event device is still running, how can the application be sure it has
flushed all the events (from the dequeue side in particular)?


In order to drain all events from the ports, I was expecting the following:

// stop scheduling new events to worker cores
rte_event_dev_stop()
---> callback gets called for each event

// to dequeue events from each port, and app cleans them up?
FOR_EACH_PORT( rte_event_dev_dequeue(..., port_id, ...) )


I'd like to avoid the dequeue-each-port() approach in application, as it adds
extra burden to clean up correctly...

What if we say that dequeue() returns zero after stop() (leaving events possibly
in the port-dequeue side SW buffers), and these events which were about to
be dequeued by the worker core are also passed to the dev_stop_flush callback?

Re: [PATCH v4 2/2] event/sw: support device stop flush callback

[Van Haaren, Harry]

> From: Eads, Gage
> Sent: Tuesday, March 20, 2018 2:13 PM
> To: dev@dpdk.org
> Cc: jerin.jacob@caviumnetworks.com; Van Haaren, Harry
> <harry.van.haaren@intel.com>; hemant.agrawal@nxp.com; Richardson, Bruce
> <bruce.richardson@intel.com>; santosh.shukla@caviumnetworks.com;
> nipun.gupta@nxp.com
> Subject: [PATCH v4 2/2] event/sw: support device stop flush callback
> 
> This commit also adds a flush callback test to the sw eventdev's selftest
> suite.
> 
> Signed-off-by: Gage Eads <gage.eads@intel.com>
> ---
>  drivers/event/sw/sw_evdev.c          | 25 ++++++++++-
>  drivers/event/sw/sw_evdev_selftest.c | 80
> +++++++++++++++++++++++++++++++++++-
>  2 files changed, 102 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/event/sw/sw_evdev.c b/drivers/event/sw/sw_evdev.c
> index 0e89f11..11f394f 100644
> --- a/drivers/event/sw/sw_evdev.c
> +++ b/drivers/event/sw/sw_evdev.c
> @@ -362,8 +362,25 @@ sw_init_qid_iqs(struct sw_evdev *sw)
>  }
> 
>  static void
> -sw_clean_qid_iqs(struct sw_evdev *sw)
> +sw_flush_iq(struct rte_eventdev *dev, struct sw_iq *iq)
>  {
> +	struct sw_evdev *sw = sw_pmd_priv(dev);
> +
> +	while (iq_count(iq) > 0) {
> +		struct rte_event event;
> +
> +		iq_dequeue_burst(sw, iq, &event, 1);
> +
> +		dev->dev_ops->dev_stop_flush(dev->data->dev_id,
> +					     event,
> +					     dev->data->dev_stop_flush_arg);


Adding check that dev_stop_flush() is non-NULL?

[Update: Ah I see you do this below already. Still, better check twice
I think, the data path isn't running here anyway in case future me
decides to call sw_flush_iq() without performing the check]

if(dev->dev_ops->dev_stop_flush)
   dev->dev_ops->dev_stop_flush(...);




> @@ -702,7 +723,7 @@ static void
>  sw_stop(struct rte_eventdev *dev)
>  {
>  	struct sw_evdev *sw = sw_pmd_priv(dev);
> -	sw_clean_qid_iqs(sw);
> +	sw_clean_qid_iqs(dev);

Based on the port buffers comment on 1/2, we probably need a
sw_clean_port_buffers(sw); here to return any events in the port
owned SW buffers?


Rest looks good to me!

Re: [PATCH v4 1/2] eventdev: add device stop flush callback

[Eads, Gage]

> -----Original Message-----
> From: Van Haaren, Harry
> Sent: Friday, March 23, 2018 11:57 AM
> To: Eads, Gage <gage.eads@intel.com>; dev@dpdk.org
> Cc: jerin.jacob@caviumnetworks.com; hemant.agrawal@nxp.com; Richardson,
> Bruce <bruce.richardson@intel.com>; santosh.shukla@caviumnetworks.com;
> nipun.gupta@nxp.com
> Subject: RE: [PATCH v4 1/2] eventdev: add device stop flush callback
> 
> > From: Eads, Gage
> > Sent: Tuesday, March 20, 2018 2:13 PM
> > To: dev@dpdk.org
> > Cc: jerin.jacob@caviumnetworks.com; Van Haaren, Harry
> > <harry.van.haaren@intel.com>; hemant.agrawal@nxp.com; Richardson,
> > Bruce <bruce.richardson@intel.com>; santosh.shukla@caviumnetworks.com;
> > nipun.gupta@nxp.com
> > Subject: [PATCH v4 1/2] eventdev: add device stop flush callback
> >
> > When an event device is stopped, it drains all event queues. These
> > events may contain pointers, so to prevent memory leaks eventdev now
> > supports a user-provided flush callback that is called during the queue drain
> process.
> > This callback is stored in process memory, so the callback must be
> > registered by any process that may call rte_event_dev_stop().
> >
> > This commit also clarifies the behavior of rte_event_dev_stop().
> >
> > This follows this mailing list discussion:
> > http://dpdk.org/ml/archives/dev/2018-January/087484.html
> >
> > Signed-off-by: Gage Eads <gage.eads@intel.com>
> 
> <snip most of the code - looks good!>
> 
> >  /**
> > - * Stop an event device. The device can be restarted with a call to
> > - * rte_event_dev_start()
> > + * Stop an event device.
> > + *
> > + * This function causes all queued events to be drained. While
> > + draining
> > events
> > + * out of the device, this function calls the user-provided flush
> > + callback
> > + * (if one was registered) once per event.
> > + *
> > + * This function does not drain events from event ports; the
> > + application is
> > + * responsible for flushing events from all ports before stopping the
> > device.
> 
> 
> Question about how an application is expected to correctly cleanup all the
> events here. Note in particular the last part: "application is responsible for
> flushing events from all ports **BEFORE** stopping the device".
> 
> Given the event device is still running, how can the application be sure it has
> flushed all the events (from the dequeue side in particular)?
> 

Appreciate the feedback -- good points all around.

I was expecting that the application would unlink queues from the ports, and then dequeue until each port has no events. However, there are PMDs for which runtime port link/unlink is not supported, so I see that this is not a viable approach. Plus, this adds the application burden that you describe below.

> 
> In order to drain all events from the ports, I was expecting the following:
> 
> // stop scheduling new events to worker cores
> rte_event_dev_stop()
> ---> callback gets called for each event
> 
> // to dequeue events from each port, and app cleans them up?
> FOR_EACH_PORT( rte_event_dev_dequeue(..., port_id, ...) )
> 
> 
> I'd like to avoid the dequeue-each-port() approach in application, as it adds extra
> burden to clean up correctly...

Agreed, but for a different reason: that approach means we'd have to change the documented eventdev behavior. rte_eventdev.h states that the "schedule, enqueue and dequeue functions should not be invoked when the device is stopped," and this patch reiterates that in the rte_event_dev_stop() documentation ("Threads that continue to enqueue/dequeue while the device is stopped, or being stopped, will result in undefined behavior"). Since a PMD's stop cleanup code could just be repeated calls to a PMD's dequeue code, allowing applications to dequeue simultaneously could be troublesome.

> 
> What if we say that dequeue() returns zero after stop() (leaving events possibly
> in the port-dequeue side SW buffers), and these events which were about to be
> dequeued by the worker core are also passed to the dev_stop_flush callback?

I'd prefer to have dequeue-while-stopped be unsupported, so we don't need an additional check or synchronization in the datapath, but passing the events in a port to the callback should work (for the sw PMD, at least). How does that sound?

Thanks,
Gage

Re: [PATCH v4 2/2] event/sw: support device stop flush callback

[Eads, Gage]

> -----Original Message-----
> From: Van Haaren, Harry
> Sent: Friday, March 23, 2018 12:06 PM
> To: Eads, Gage <gage.eads@intel.com>; dev@dpdk.org
> Cc: jerin.jacob@caviumnetworks.com; hemant.agrawal@nxp.com; Richardson,
> Bruce <bruce.richardson@intel.com>; santosh.shukla@caviumnetworks.com;
> nipun.gupta@nxp.com
> Subject: RE: [PATCH v4 2/2] event/sw: support device stop flush callback
> 
> > From: Eads, Gage
> > Sent: Tuesday, March 20, 2018 2:13 PM
> > To: dev@dpdk.org
> > Cc: jerin.jacob@caviumnetworks.com; Van Haaren, Harry
> > <harry.van.haaren@intel.com>; hemant.agrawal@nxp.com; Richardson,
> > Bruce <bruce.richardson@intel.com>; santosh.shukla@caviumnetworks.com;
> > nipun.gupta@nxp.com
> > Subject: [PATCH v4 2/2] event/sw: support device stop flush callback
> >
> > This commit also adds a flush callback test to the sw eventdev's
> > selftest suite.
> >
> > Signed-off-by: Gage Eads <gage.eads@intel.com>
> > ---
> >  drivers/event/sw/sw_evdev.c          | 25 ++++++++++-
> >  drivers/event/sw/sw_evdev_selftest.c | 80
> > +++++++++++++++++++++++++++++++++++-
> >  2 files changed, 102 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/event/sw/sw_evdev.c b/drivers/event/sw/sw_evdev.c
> > index 0e89f11..11f394f 100644
> > --- a/drivers/event/sw/sw_evdev.c
> > +++ b/drivers/event/sw/sw_evdev.c
> > @@ -362,8 +362,25 @@ sw_init_qid_iqs(struct sw_evdev *sw)  }
> >
> >  static void
> > -sw_clean_qid_iqs(struct sw_evdev *sw)
> > +sw_flush_iq(struct rte_eventdev *dev, struct sw_iq *iq)
> >  {
> > +	struct sw_evdev *sw = sw_pmd_priv(dev);
> > +
> > +	while (iq_count(iq) > 0) {
> > +		struct rte_event event;
> > +
> > +		iq_dequeue_burst(sw, iq, &event, 1);
> > +
> > +		dev->dev_ops->dev_stop_flush(dev->data->dev_id,
> > +					     event,
> > +					     dev->data->dev_stop_flush_arg);
> 
> 
> Adding check that dev_stop_flush() is non-NULL?
> 
> [Update: Ah I see you do this below already. Still, better check twice I think, the
> data path isn't running here anyway in case future me decides to call
> sw_flush_iq() without performing the check]

Agreed, will fix in v5.

> 
> if(dev->dev_ops->dev_stop_flush)
>    dev->dev_ops->dev_stop_flush(...);
> 
> 
> 
> 
> > @@ -702,7 +723,7 @@ static void
> >  sw_stop(struct rte_eventdev *dev)
> >  {
> >  	struct sw_evdev *sw = sw_pmd_priv(dev);
> > -	sw_clean_qid_iqs(sw);
> > +	sw_clean_qid_iqs(dev);
> 
> Based on the port buffers comment on 1/2, we probably need a
> sw_clean_port_buffers(sw); here to return any events in the port owned SW
> buffers?
> 

Agreed, if there is buy-in on the approach discussed in patch #1.

Re: [PATCH v4 1/2] eventdev: add device stop flush callback

[Van Haaren, Harry]

> From: Eads, Gage
> Sent: Monday, March 26, 2018 10:59 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>; dev@dpdk.org
> Cc: jerin.jacob@caviumnetworks.com; hemant.agrawal@nxp.com; Richardson, Bruce
> <bruce.richardson@intel.com>; santosh.shukla@caviumnetworks.com;
> nipun.gupta@nxp.com
> Subject: RE: [PATCH v4 1/2] eventdev: add device stop flush callback
> 
> 
> 
> > -----Original Message-----
> > From: Van Haaren, Harry
> > Sent: Friday, March 23, 2018 11:57 AM
> > To: Eads, Gage <gage.eads@intel.com>; dev@dpdk.org
> > Cc: jerin.jacob@caviumnetworks.com; hemant.agrawal@nxp.com; Richardson,
> > Bruce <bruce.richardson@intel.com>; santosh.shukla@caviumnetworks.com;
> > nipun.gupta@nxp.com
> > Subject: RE: [PATCH v4 1/2] eventdev: add device stop flush callback
> >
> > > From: Eads, Gage
> > > Sent: Tuesday, March 20, 2018 2:13 PM
> > > To: dev@dpdk.org
> > > Cc: jerin.jacob@caviumnetworks.com; Van Haaren, Harry
> > > <harry.van.haaren@intel.com>; hemant.agrawal@nxp.com; Richardson,
> > > Bruce <bruce.richardson@intel.com>; santosh.shukla@caviumnetworks.com;
> > > nipun.gupta@nxp.com
> > > Subject: [PATCH v4 1/2] eventdev: add device stop flush callback
> > >
> > > When an event device is stopped, it drains all event queues. These
> > > events may contain pointers, so to prevent memory leaks eventdev now
> > > supports a user-provided flush callback that is called during the queue
> drain
> > process.
> > > This callback is stored in process memory, so the callback must be
> > > registered by any process that may call rte_event_dev_stop().
> > >
> > > This commit also clarifies the behavior of rte_event_dev_stop().
> > >
> > > This follows this mailing list discussion:
> > > http://dpdk.org/ml/archives/dev/2018-January/087484.html
> > >
> > > Signed-off-by: Gage Eads <gage.eads@intel.com>
> >
> > <snip most of the code - looks good!>
> >
> > >  /**
> > > - * Stop an event device. The device can be restarted with a call to
> > > - * rte_event_dev_start()
> > > + * Stop an event device.
> > > + *
> > > + * This function causes all queued events to be drained. While
> > > + draining
> > > events
> > > + * out of the device, this function calls the user-provided flush
> > > + callback
> > > + * (if one was registered) once per event.
> > > + *
> > > + * This function does not drain events from event ports; the
> > > + application is
> > > + * responsible for flushing events from all ports before stopping the
> > > device.
> >
> >
> > Question about how an application is expected to correctly cleanup all the
> > events here. Note in particular the last part: "application is responsible
> for
> > flushing events from all ports **BEFORE** stopping the device".
> >
> > Given the event device is still running, how can the application be sure it
> has
> > flushed all the events (from the dequeue side in particular)?
> >
> 
> Appreciate the feedback -- good points all around.
> 
> I was expecting that the application would unlink queues from the ports, and
> then dequeue until each port has no events. However, there are PMDs for which
> runtime port link/unlink is not supported, so I see that this is not a viable
> approach. Plus, this adds the application burden that you describe below.

+1.

> >
> > In order to drain all events from the ports, I was expecting the following:
> >
> > // stop scheduling new events to worker cores
> > rte_event_dev_stop()
> > ---> callback gets called for each event
> >
> > // to dequeue events from each port, and app cleans them up?
> > FOR_EACH_PORT( rte_event_dev_dequeue(..., port_id, ...) )
> >
> >
> > I'd like to avoid the dequeue-each-port() approach in application, as it
> adds extra
> > burden to clean up correctly...
> 
> Agreed, but for a different reason: that approach means we'd have to change
> the documented eventdev behavior. rte_eventdev.h states that the "schedule,
> enqueue and dequeue functions should not be invoked when the device is
> stopped," and this patch reiterates that in the rte_event_dev_stop()
> documentation ("Threads that continue to enqueue/dequeue while the device is
> stopped, or being stopped, will result in undefined behavior"). Since a PMD's
> stop cleanup code could just be repeated calls to a PMD's dequeue code,
> allowing applications to dequeue simultaneously could be troublesome.

All +1 too, good point about the header stating it is undefined behavior.


> > What if we say that dequeue() returns zero after stop() (leaving events
> possibly
> > in the port-dequeue side SW buffers), and these events which were about to
> be
> > dequeued by the worker core are also passed to the dev_stop_flush callback?
> 
> I'd prefer to have dequeue-while-stopped be unsupported, so we don't need an
> additional check or synchronization in the datapath, but passing the events in
> a port to the callback should work (for the sw PMD, at least). How does that
> sound?


That's fine with me, both from design point of view, and SW PMD.

@HW PMD maintainers, would the above approach work for you?

Re: [PATCH v4 1/2] eventdev: add device stop flush callback

[Van Haaren, Harry]

(+Liang Ma for OPDL maintainer)

Ping to maintainers, is the below suggestion acceptable for your PMDs?

Summary of suggestion:
- After event_dev_stop() dequeue() is no longer allowed on any thread
- All events in the system (queues, ports, intermediary buffers) will be passed to a user-supplied callback for cleaning up events.



> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Van Haaren, Harry
> Sent: Tuesday, March 27, 2018 9:20 AM
> To: Eads, Gage <gage.eads@intel.com>; jerin.jacob@caviumnetworks.com;
> hemant.agrawal@nxp.com
> Cc: Richardson, Bruce <bruce.richardson@intel.com>;
> santosh.shukla@caviumnetworks.com; nipun.gupta@nxp.com; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v4 1/2] eventdev: add device stop flush
> callback
> 
> > From: Eads, Gage
> > Sent: Monday, March 26, 2018 10:59 PM
> > To: Van Haaren, Harry <harry.van.haaren@intel.com>; dev@dpdk.org
> > Cc: jerin.jacob@caviumnetworks.com; hemant.agrawal@nxp.com; Richardson,
> Bruce
> > <bruce.richardson@intel.com>; santosh.shukla@caviumnetworks.com;
> > nipun.gupta@nxp.com
> > Subject: RE: [PATCH v4 1/2] eventdev: add device stop flush callback
> >
> >
> >
> > > -----Original Message-----
> > > From: Van Haaren, Harry
> > > Sent: Friday, March 23, 2018 11:57 AM
> > > To: Eads, Gage <gage.eads@intel.com>; dev@dpdk.org
> > > Cc: jerin.jacob@caviumnetworks.com; hemant.agrawal@nxp.com; Richardson,
> > > Bruce <bruce.richardson@intel.com>; santosh.shukla@caviumnetworks.com;
> > > nipun.gupta@nxp.com
> > > Subject: RE: [PATCH v4 1/2] eventdev: add device stop flush callback
> > >
> > > > From: Eads, Gage
> > > > Sent: Tuesday, March 20, 2018 2:13 PM
> > > > To: dev@dpdk.org
> > > > Cc: jerin.jacob@caviumnetworks.com; Van Haaren, Harry
> > > > <harry.van.haaren@intel.com>; hemant.agrawal@nxp.com; Richardson,
> > > > Bruce <bruce.richardson@intel.com>; santosh.shukla@caviumnetworks.com;
> > > > nipun.gupta@nxp.com
> > > > Subject: [PATCH v4 1/2] eventdev: add device stop flush callback
> > > >
> > > > When an event device is stopped, it drains all event queues. These
> > > > events may contain pointers, so to prevent memory leaks eventdev now
> > > > supports a user-provided flush callback that is called during the
> queue
> > drain
> > > process.
> > > > This callback is stored in process memory, so the callback must be
> > > > registered by any process that may call rte_event_dev_stop().
> > > >
> > > > This commit also clarifies the behavior of rte_event_dev_stop().
> > > >
> > > > This follows this mailing list discussion:
> > > > http://dpdk.org/ml/archives/dev/2018-January/087484.html
> > > >
> > > > Signed-off-by: Gage Eads <gage.eads@intel.com>
> > >
> > > <snip most of the code - looks good!>
> > >
> > > >  /**
> > > > - * Stop an event device. The device can be restarted with a call to
> > > > - * rte_event_dev_start()
> > > > + * Stop an event device.
> > > > + *
> > > > + * This function causes all queued events to be drained. While
> > > > + draining
> > > > events
> > > > + * out of the device, this function calls the user-provided flush
> > > > + callback
> > > > + * (if one was registered) once per event.
> > > > + *
> > > > + * This function does not drain events from event ports; the
> > > > + application is
> > > > + * responsible for flushing events from all ports before stopping the
> > > > device.
> > >
> > >
> > > Question about how an application is expected to correctly cleanup all
> the
> > > events here. Note in particular the last part: "application is
> responsible
> > for
> > > flushing events from all ports **BEFORE** stopping the device".
> > >
> > > Given the event device is still running, how can the application be sure
> it
> > has
> > > flushed all the events (from the dequeue side in particular)?
> > >
> >
> > Appreciate the feedback -- good points all around.
> >
> > I was expecting that the application would unlink queues from the ports,
> and
> > then dequeue until each port has no events. However, there are PMDs for
> which
> > runtime port link/unlink is not supported, so I see that this is not a
> viable
> > approach. Plus, this adds the application burden that you describe below.
> 
> +1.
> 
> > >
> > > In order to drain all events from the ports, I was expecting the
> following:
> > >
> > > // stop scheduling new events to worker cores
> > > rte_event_dev_stop()
> > > ---> callback gets called for each event
> > >
> > > // to dequeue events from each port, and app cleans them up?
> > > FOR_EACH_PORT( rte_event_dev_dequeue(..., port_id, ...) )
> > >
> > >
> > > I'd like to avoid the dequeue-each-port() approach in application, as it
> > adds extra
> > > burden to clean up correctly...
> >
> > Agreed, but for a different reason: that approach means we'd have to
> change
> > the documented eventdev behavior. rte_eventdev.h states that the
> "schedule,
> > enqueue and dequeue functions should not be invoked when the device is
> > stopped," and this patch reiterates that in the rte_event_dev_stop()
> > documentation ("Threads that continue to enqueue/dequeue while the device
> is
> > stopped, or being stopped, will result in undefined behavior"). Since a
> PMD's
> > stop cleanup code could just be repeated calls to a PMD's dequeue code,
> > allowing applications to dequeue simultaneously could be troublesome.
> 
> All +1 too, good point about the header stating it is undefined behavior.
> 
> 
> > > What if we say that dequeue() returns zero after stop() (leaving events
> > possibly
> > > in the port-dequeue side SW buffers), and these events which were about
> to
> > be
> > > dequeued by the worker core are also passed to the dev_stop_flush
> callback?
> >
> > I'd prefer to have dequeue-while-stopped be unsupported, so we don't need
> an
> > additional check or synchronization in the datapath, but passing the
> events in
> > a port to the callback should work (for the sw PMD, at least). How does
> that
> > sound?
> 
> 
> That's fine with me, both from design point of view, and SW PMD.
> 
> @HW PMD maintainers, would the above approach work for you?
> 
> 
> 

Re: [PATCH v4 1/2] eventdev: add device stop flush callback

[Jerin Jacob]

-----Original Message-----
> Date: Thu, 29 Mar 2018 11:02:55 +0000
> From: "Van Haaren, Harry" <harry.van.haaren@intel.com>
> To: "jerin.jacob@caviumnetworks.com" <jerin.jacob@caviumnetworks.com>,
>  "hemant.agrawal@nxp.com" <hemant.agrawal@nxp.com>, "Ma, Liang J"
>  <liang.j.ma@intel.com>
> CC: "Eads, Gage" <gage.eads@intel.com>, "Richardson, Bruce"
>  <bruce.richardson@intel.com>, "santosh.shukla@caviumnetworks.com"
>  <santosh.shukla@caviumnetworks.com>, "nipun.gupta@nxp.com"
>  <nipun.gupta@nxp.com>, "dev@dpdk.org" <dev@dpdk.org>
> Subject: RE: [dpdk-dev] [PATCH v4 1/2] eventdev: add device stop flush
>  callback
> 
> (+Liang Ma for OPDL maintainer)
> 
> Ping to maintainers, is the below suggestion acceptable for your PMDs?
> 
> Summary of suggestion:
> - After event_dev_stop() dequeue() is no longer allowed on any thread
> - All events in the system (queues, ports, intermediary buffers) will be passed to a user-supplied callback for cleaning up events.

+1. That's works for octeontx eventdev PMD.

Re: [PATCH v4 1/2] eventdev: add device stop flush callback

[Liang, Ma]

On 29 Mar 04:02, Van Haaren, Harry wrote:
> (+Liang Ma for OPDL maintainer)
> 
> Ping to maintainers, is the below suggestion acceptable for your PMDs?
> 
> Summary of suggestion:
> - After event_dev_stop() dequeue() is no longer allowed on any thread
> - All events in the system (queues, ports, intermediary buffers) will be passed to a user-supplied callback for cleaning up events.
+1, this should work for OPDL
> 
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Van Haaren, Harry
> > Sent: Tuesday, March 27, 2018 9:20 AM
> > To: Eads, Gage <gage.eads@intel.com>; jerin.jacob@caviumnetworks.com;
> > hemant.agrawal@nxp.com
> > Cc: Richardson, Bruce <bruce.richardson@intel.com>;
> > santosh.shukla@caviumnetworks.com; nipun.gupta@nxp.com; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v4 1/2] eventdev: add device stop flush
> > callback
> > 
> > > From: Eads, Gage
> > > Sent: Monday, March 26, 2018 10:59 PM
> > > To: Van Haaren, Harry <harry.van.haaren@intel.com>; dev@dpdk.org
> > > Cc: jerin.jacob@caviumnetworks.com; hemant.agrawal@nxp.com; Richardson,
> > Bruce
> > > <bruce.richardson@intel.com>; santosh.shukla@caviumnetworks.com;
> > > nipun.gupta@nxp.com
> > > Subject: RE: [PATCH v4 1/2] eventdev: add device stop flush callback
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Van Haaren, Harry
> > > > Sent: Friday, March 23, 2018 11:57 AM
> > > > To: Eads, Gage <gage.eads@intel.com>; dev@dpdk.org
> > > > Cc: jerin.jacob@caviumnetworks.com; hemant.agrawal@nxp.com; Richardson,
> > > > Bruce <bruce.richardson@intel.com>; santosh.shukla@caviumnetworks.com;
> > > > nipun.gupta@nxp.com
> > > > Subject: RE: [PATCH v4 1/2] eventdev: add device stop flush callback
> > > >
> > > > > From: Eads, Gage
> > > > > Sent: Tuesday, March 20, 2018 2:13 PM
> > > > > To: dev@dpdk.org
> > > > > Cc: jerin.jacob@caviumnetworks.com; Van Haaren, Harry
> > > > > <harry.van.haaren@intel.com>; hemant.agrawal@nxp.com; Richardson,
> > > > > Bruce <bruce.richardson@intel.com>; santosh.shukla@caviumnetworks.com;
> > > > > nipun.gupta@nxp.com
> > > > > Subject: [PATCH v4 1/2] eventdev: add device stop flush callback
> > > > >
> > > > > When an event device is stopped, it drains all event queues. These
> > > > > events may contain pointers, so to prevent memory leaks eventdev now
> > > > > supports a user-provided flush callback that is called during the
> > queue
> > > drain
> > > > process.
> > > > > This callback is stored in process memory, so the callback must be
> > > > > registered by any process that may call rte_event_dev_stop().
> > > > >
> > > > > This commit also clarifies the behavior of rte_event_dev_stop().
> > > > >
> > > > > This follows this mailing list discussion:
> > > > > http://dpdk.org/ml/archives/dev/2018-January/087484.html
> > > > >
> > > > > Signed-off-by: Gage Eads <gage.eads@intel.com>
> > > >
> > > > <snip most of the code - looks good!>
> > > >
> > > > >  /**
> > > > > - * Stop an event device. The device can be restarted with a call to
> > > > > - * rte_event_dev_start()
> > > > > + * Stop an event device.
> > > > > + *
> > > > > + * This function causes all queued events to be drained. While
> > > > > + draining
> > > > > events
> > > > > + * out of the device, this function calls the user-provided flush
> > > > > + callback
> > > > > + * (if one was registered) once per event.
> > > > > + *
> > > > > + * This function does not drain events from event ports; the
> > > > > + application is
> > > > > + * responsible for flushing events from all ports before stopping the
> > > > > device.
> > > >
> > > >
> > > > Question about how an application is expected to correctly cleanup all
> > the
> > > > events here. Note in particular the last part: "application is
> > responsible
> > > for
> > > > flushing events from all ports **BEFORE** stopping the device".
> > > >
> > > > Given the event device is still running, how can the application be sure
> > it
> > > has
> > > > flushed all the events (from the dequeue side in particular)?
> > > >
> > >
> > > Appreciate the feedback -- good points all around.
> > >
> > > I was expecting that the application would unlink queues from the ports,
> > and
> > > then dequeue until each port has no events. However, there are PMDs for
> > which
> > > runtime port link/unlink is not supported, so I see that this is not a
> > viable
> > > approach. Plus, this adds the application burden that you describe below.
> > 
> > +1.
> > 
> > > >
> > > > In order to drain all events from the ports, I was expecting the
> > following:
> > > >
> > > > // stop scheduling new events to worker cores
> > > > rte_event_dev_stop()
> > > > ---> callback gets called for each event
> > > >
> > > > // to dequeue events from each port, and app cleans them up?
> > > > FOR_EACH_PORT( rte_event_dev_dequeue(..., port_id, ...) )
> > > >
> > > >
> > > > I'd like to avoid the dequeue-each-port() approach in application, as it
> > > adds extra
> > > > burden to clean up correctly...
> > >
> > > Agreed, but for a different reason: that approach means we'd have to
> > change
> > > the documented eventdev behavior. rte_eventdev.h states that the
> > "schedule,
> > > enqueue and dequeue functions should not be invoked when the device is
> > > stopped," and this patch reiterates that in the rte_event_dev_stop()
> > > documentation ("Threads that continue to enqueue/dequeue while the device
> > is
> > > stopped, or being stopped, will result in undefined behavior"). Since a
> > PMD's
> > > stop cleanup code could just be repeated calls to a PMD's dequeue code,
> > > allowing applications to dequeue simultaneously could be troublesome.
> > 
> > All +1 too, good point about the header stating it is undefined behavior.
> > 
> > 
> > > > What if we say that dequeue() returns zero after stop() (leaving events
> > > possibly
> > > > in the port-dequeue side SW buffers), and these events which were about
> > to
> > > be
> > > > dequeued by the worker core are also passed to the dev_stop_flush
> > callback?
> > >
> > > I'd prefer to have dequeue-while-stopped be unsupported, so we don't need
> > an
> > > additional check or synchronization in the datapath, but passing the
> > events in
> > > a port to the callback should work (for the sw PMD, at least). How does
> > that
> > > sound?
> > 
> > 
> > That's fine with me, both from design point of view, and SW PMD.
> > 
> > @HW PMD maintainers, would the above approach work for you?
> > 
> > 
> > 
> 

Re: [PATCH v4 1/2] eventdev: add device stop flush callback

[Jerin Jacob]

-----Original Message-----
> Date: Tue, 20 Mar 2018 09:13:06 -0500
> From: Gage Eads <gage.eads@intel.com>
> To: dev@dpdk.org
> CC: jerin.jacob@caviumnetworks.com, harry.van.haaren@intel.com,
>  hemant.agrawal@nxp.com, bruce.richardson@intel.com,
>  santosh.shukla@caviumnetworks.com, nipun.gupta@nxp.com
> Subject: [PATCH v4 1/2] eventdev: add device stop flush callback
> X-Mailer: git-send-email 2.7.4
> 
> When an event device is stopped, it drains all event queues. These events
> may contain pointers, so to prevent memory leaks eventdev now supports a
> user-provided flush callback that is called during the queue drain process.
> This callback is stored in process memory, so the callback must be
> registered by any process that may call rte_event_dev_stop().
> 
> This commit also clarifies the behavior of rte_event_dev_stop().
> 
> This follows this mailing list discussion:
> http://dpdk.org/ml/archives/dev/2018-January/087484.html
> 
> Signed-off-by: Gage Eads <gage.eads@intel.com>

Acked-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>

Re: [PATCH v4 2/2] event/sw: support device stop flush callback

[Jerin Jacob]

> > [Update: Ah I see you do this below already. Still, better check twice I think, the
> > data path isn't running here anyway in case future me decides to call
> > sw_flush_iq() without performing the check]
>
> Agreed, will fix in v5.

Gage,

Could please send the v5 so that I can include it in RC1 pull request.

Re: [PATCH v4 2/2] event/sw: support device stop flush callback

[Eads, Gage]

If it's ok with you, I'll resubmit these two patches separately. The sw implementation has some unresolved race conditions with the scheduler service that may take some time to fix, but this shouldn't block the first patch. That patch needs a fix too (v4 includes a now-incorrect comment about stop not draining events from ports), so I'll fix and resubmit that.

Thanks,
Gage

> -----Original Message-----
> From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> Sent: Monday, April 2, 2018 3:03 AM
> To: Eads, Gage <gage.eads@intel.com>
> Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; dev@dpdk.org;
> hemant.agrawal@nxp.com; Richardson, Bruce <bruce.richardson@intel.com>;
> santosh.shukla@caviumnetworks.com; nipun.gupta@nxp.com
> Subject: Re: [PATCH v4 2/2] event/sw: support device stop flush callback
> 
> > > [Update: Ah I see you do this below already. Still, better check
> > > twice I think, the data path isn't running here anyway in case
> > > future me decides to call
> > > sw_flush_iq() without performing the check]
> >
> > Agreed, will fix in v5.
> 
> Gage,
> 
> Could please send the v5 so that I can include it in RC1 pull request.
> 

Re: [PATCH v4 2/2] event/sw: support device stop flush callback

[Jerin Jacob]

-----Original Message-----
> Date: Mon, 2 Apr 2018 15:50:42 +0000
> From: "Eads, Gage" <gage.eads@intel.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> CC: "Van Haaren, Harry" <harry.van.haaren@intel.com>, "dev@dpdk.org"
>  <dev@dpdk.org>, "hemant.agrawal@nxp.com" <hemant.agrawal@nxp.com>,
>  "Richardson, Bruce" <bruce.richardson@intel.com>,
>  "santosh.shukla@caviumnetworks.com" <santosh.shukla@caviumnetworks.com>,
>  "nipun.gupta@nxp.com" <nipun.gupta@nxp.com>
> Subject: RE: [PATCH v4 2/2] event/sw: support device stop flush callback
> 
> If it's ok with you, I'll resubmit these two patches separately. The sw implementation has some unresolved race conditions with the scheduler service that may take some time to fix, but this shouldn't block the first patch. That patch needs a fix too (v4 includes a now-incorrect comment about stop not draining events from ports), so I'll fix and resubmit that.

OK. So that we can merge this common code and octeontx changes.

How about changing the following comment rte_event_dev_stop()

exiting:

+ * This function does not drain events from event ports; the
application is responsible for flushing events from all ports before stopping the
device.

proposed:(Just to share the view)
# You can add the fact that, all events across the ports will show up in
callback as we concluded.

#Application may invoke stop operation on the event adapter to
avoid event being generated.(or something similar) i.e adapter needs to
stopped before stopping the eventdev to avoid continuous looping on
getting the events(over rte_event_dev_register_callback()) from event source(ethdev Rx adapter).


> 
> Thanks,
> Gage
> 
> > -----Original Message-----
> > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> > Sent: Monday, April 2, 2018 3:03 AM
> > To: Eads, Gage <gage.eads@intel.com>
> > Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; dev@dpdk.org;
> > hemant.agrawal@nxp.com; Richardson, Bruce <bruce.richardson@intel.com>;
> > santosh.shukla@caviumnetworks.com; nipun.gupta@nxp.com
> > Subject: Re: [PATCH v4 2/2] event/sw: support device stop flush callback
> > 
> > > > [Update: Ah I see you do this below already. Still, better check
> > > > twice I think, the data path isn't running here anyway in case
> > > > future me decides to call
> > > > sw_flush_iq() without performing the check]
> > >
> > > Agreed, will fix in v5.
> > 
> > Gage,
> > 
> > Could please send the v5 so that I can include it in RC1 pull request.
> > 
> 

[PATCH v5] eventdev: add device stop flush callback

[Gage Eads]

When an event device is stopped, it drains all event queues and ports.
These events may contain pointers, so to prevent memory leaks eventdev now
supports a user-provided flush callback that is called during the queue
drain process. This callback is stored in process memory, so the callback
must be registered by any process that may call rte_event_dev_stop().

This commit also clarifies the behavior of rte_event_dev_stop().

This follows this mailing list discussion:
http://dpdk.org/ml/archives/dev/2018-January/087484.html

Signed-off-by: Gage Eads <gage.eads@intel.com>
---
v2: allow a NULL callback pointer to unregister the callback
v3: move the callback pointer to struct rte_eventdev_ops and
    the user argument to struct rte_eventdev_data
v4: remove redundant callback NULL initialization
v5: add comments about draining events from ports and stopping
    event adapters, and drop the event/sw implementation from
    the patchset until scheduler race conditions are fixed.

 drivers/event/dpaa/dpaa_eventdev.c           |  2 +-
 drivers/event/dpaa2/dpaa2_eventdev.c         |  2 +-
 drivers/event/octeontx/ssovf_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/librte_eventdev/rte_eventdev.c           | 17 +++++++++
 lib/librte_eventdev/rte_eventdev.h           | 54 ++++++++++++++++++++++++++--
 lib/librte_eventdev/rte_eventdev_pmd.h       |  3 ++
 lib/librte_eventdev/rte_eventdev_version.map |  6 ++++
 10 files changed, 83 insertions(+), 9 deletions(-)

diff --git a/drivers/event/dpaa/dpaa_eventdev.c b/drivers/event/dpaa/dpaa_eventdev.c
index 0006801..fcb648d 100644
--- a/drivers/event/dpaa/dpaa_eventdev.c
+++ b/drivers/event/dpaa/dpaa_eventdev.c
@@ -571,7 +571,7 @@ dpaa_event_eth_rx_adapter_stop(const struct rte_eventdev *dev,
 	return 0;
 }
 
-static const struct rte_eventdev_ops dpaa_eventdev_ops = {
+static struct rte_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 c3e6fbf..5270dc0 100644
--- a/drivers/event/dpaa2/dpaa2_eventdev.c
+++ b/drivers/event/dpaa2/dpaa2_eventdev.c
@@ -693,7 +693,7 @@ dpaa2_eventdev_eth_stop(const struct rte_eventdev *dev,
 	return 0;
 }
 
-static const struct rte_eventdev_ops dpaa2_eventdev_ops = {
+static struct rte_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/octeontx/ssovf_evdev.c b/drivers/event/octeontx/ssovf_evdev.c
index a108607..bc1f05f 100644
--- a/drivers/event/octeontx/ssovf_evdev.c
+++ b/drivers/event/octeontx/ssovf_evdev.c
@@ -591,7 +591,7 @@ ssovf_selftest(const char *key __rte_unused, const char *value,
 }
 
 /* Initialize and register event driver with DPDK Application */
-static const struct rte_eventdev_ops ssovf_ops = {
+static struct rte_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/opdl/opdl_evdev.c b/drivers/event/opdl/opdl_evdev.c
index 7708369..ef9fb30 100644
--- a/drivers/event/opdl/opdl_evdev.c
+++ b/drivers/event/opdl/opdl_evdev.c
@@ -607,7 +607,7 @@ set_do_test(const char *key __rte_unused, const char *value, void *opaque)
 static int
 opdl_probe(struct rte_vdev_device *vdev)
 {
-	static const struct rte_eventdev_ops evdev_opdl_ops = {
+	static struct rte_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 7f46756..c889220 100644
--- a/drivers/event/skeleton/skeleton_eventdev.c
+++ b/drivers/event/skeleton/skeleton_eventdev.c
@@ -319,7 +319,7 @@ skeleton_eventdev_dump(struct rte_eventdev *dev, FILE *f)
 
 
 /* Initialize and register event driver with DPDK Application */
-static const struct rte_eventdev_ops skeleton_eventdev_ops = {
+static struct rte_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 6672fd8..0e89f11 100644
--- a/drivers/event/sw/sw_evdev.c
+++ b/drivers/event/sw/sw_evdev.c
@@ -772,7 +772,7 @@ static int32_t sw_sched_service_func(void *args)
 static int
 sw_probe(struct rte_vdev_device *vdev)
 {
-	static const struct rte_eventdev_ops evdev_sw_ops = {
+	static struct rte_eventdev_ops evdev_sw_ops = {
 			.dev_configure = sw_dev_configure,
 			.dev_infos_get = sw_info_get,
 			.dev_close = sw_close,
diff --git a/lib/librte_eventdev/rte_eventdev.c b/lib/librte_eventdev/rte_eventdev.c
index 851a119..2de8d9a 100644
--- a/lib/librte_eventdev/rte_eventdev.c
+++ b/lib/librte_eventdev/rte_eventdev.c
@@ -1123,6 +1123,23 @@ rte_event_dev_start(uint8_t dev_id)
 	return 0;
 }
 
+int
+rte_event_dev_stop_flush_callback_register(uint8_t dev_id,
+		eventdev_stop_flush_t callback, void *userdata)
+{
+	struct rte_eventdev *dev;
+
+	RTE_EDEV_LOG_DEBUG("Stop flush register dev_id=%" PRIu8, dev_id);
+
+	RTE_EVENTDEV_VALID_DEVID_OR_ERR_RET(dev_id, -EINVAL);
+	dev = &rte_eventdevs[dev_id];
+
+	dev->dev_ops->dev_stop_flush = callback;
+	dev->data->dev_stop_flush_arg = userdata;
+
+	return 0;
+}
+
 void
 rte_event_dev_stop(uint8_t dev_id)
 {
diff --git a/lib/librte_eventdev/rte_eventdev.h b/lib/librte_eventdev/rte_eventdev.h
index b21c271..a20077c 100644
--- a/lib/librte_eventdev/rte_eventdev.h
+++ b/lib/librte_eventdev/rte_eventdev.h
@@ -244,6 +244,7 @@ extern "C" {
 #include <rte_errno.h>
 
 struct rte_mbuf; /* we just use mbuf pointers; no need to include rte_mbuf.h */
+struct rte_event;
 
 /* Event device capability bitmap flags */
 #define RTE_EVENT_DEV_CAP_QUEUE_QOS           (1ULL << 0)
@@ -835,15 +836,60 @@ int
 rte_event_dev_start(uint8_t dev_id);
 
 /**
- * Stop an event device. The device can be restarted with a call to
- * rte_event_dev_start()
+ * Stop an event device.
+ *
+ * This function causes all queued events to be drained, including those
+ * residing in event ports. While draining events out of the device, this
+ * function calls the user-provided flush callback (if one was registered) once
+ * per event.
+ *
+ * The device can be restarted with a call to rte_event_dev_start(). Threads
+ * that continue to enqueue/dequeue while the device is stopped, or being
+ * stopped, will result in undefined behavior. This includes event adapters,
+ * which must be stopped prior to stopping the eventdev.
  *
  * @param dev_id
  *   Event device identifier.
+ *
+ * @see rte_event_dev_stop_flush_callback_register()
  */
 void
 rte_event_dev_stop(uint8_t dev_id);
 
+typedef void (*eventdev_stop_flush_t)(uint8_t dev_id, struct rte_event event,
+		void *arg);
+/**< Callback function called during rte_event_dev_stop(), invoked once per
+ * flushed event.
+ */
+
+/**
+ * Registers a callback function to be invoked during rte_event_dev_stop() for
+ * each flushed event. This function can be used to properly dispose of queued
+ * events, for example events containing memory pointers.
+ *
+ * The callback function is only registered for the calling process. The
+ * callback function must be registered in every process that can call
+ * rte_event_dev_stop().
+ *
+ * To unregister a callback, call this function with a NULL callback pointer.
+ *
+ * @param dev_id
+ *   The identifier of the device.
+ * @param callback
+ *   Callback function invoked once per flushed event.
+ * @param userdata
+ *   Argument supplied to callback.
+ *
+ * @return
+ *  - 0 on success.
+ *  - -EINVAL if *dev_id* is invalid
+ *
+ * @see rte_event_dev_stop()
+ */
+int
+rte_event_dev_stop_flush_callback_register(uint8_t dev_id,
+		eventdev_stop_flush_t callback, void *userdata);
+
 /**
  * Close an event device. The device cannot be restarted!
  *
@@ -1152,6 +1198,8 @@ struct rte_eventdev_data {
 	/* Service initialization state */
 	uint32_t service_id;
 	/* Service ID*/
+	void *dev_stop_flush_arg;
+	/**< User-provided argument for event flush function */
 
 	RTE_STD_C11
 	uint8_t dev_started : 1;
@@ -1178,7 +1226,7 @@ struct rte_eventdev {
 
 	struct rte_eventdev_data *data;
 	/**< Pointer to device data */
-	const struct rte_eventdev_ops *dev_ops;
+	struct rte_eventdev_ops *dev_ops;
 	/**< Functions exported by PMD */
 	struct rte_device *dev;
 	/**< Device info. supplied by probing */
diff --git a/lib/librte_eventdev/rte_eventdev_pmd.h b/lib/librte_eventdev/rte_eventdev_pmd.h
index 31343b5..3a8ddd7 100644
--- a/lib/librte_eventdev/rte_eventdev_pmd.h
+++ b/lib/librte_eventdev/rte_eventdev_pmd.h
@@ -642,6 +642,9 @@ struct rte_eventdev_ops {
 
 	eventdev_selftest dev_selftest;
 	/**< Start eventdev Selftest */
+
+	eventdev_stop_flush_t dev_stop_flush;
+	/**< User-provided event flush function */
 };
 
 /**
diff --git a/lib/librte_eventdev/rte_eventdev_version.map b/lib/librte_eventdev/rte_eventdev_version.map
index 2aef470..4396536 100644
--- a/lib/librte_eventdev/rte_eventdev_version.map
+++ b/lib/librte_eventdev/rte_eventdev_version.map
@@ -74,3 +74,9 @@ DPDK_18.02 {
 
 	rte_event_dev_selftest;
 } DPDK_17.11;
+
+DPDK_18.05 {
+	global:
+
+	rte_event_dev_stop_flush_callback_register;
+} DPDK_18.02;
-- 
2.7.4

Re: [PATCH v5] eventdev: add device stop flush callback

[Jerin Jacob]

-----Original Message-----
> Date: Mon, 2 Apr 2018 13:03:30 -0500
> From: Gage Eads <gage.eads@intel.com>
> To: dev@dpdk.org
> CC: jerin.jacob@caviumnetworks.com, harry.van.haaren@intel.com,
>  hemant.agrawal@nxp.com, bruce.richardson@intel.com,
>  santosh.shukla@caviumnetworks.com, nipun.gupta@nxp.com,
>  liang.j.ma@intel.com
> Subject: [PATCH v5] eventdev: add device stop flush callback
> X-Mailer: git-send-email 2.7.4
> 
> When an event device is stopped, it drains all event queues and ports.
> These events may contain pointers, so to prevent memory leaks eventdev now
> supports a user-provided flush callback that is called during the queue
> drain process. This callback is stored in process memory, so the callback
> must be registered by any process that may call rte_event_dev_stop().
> 
> This commit also clarifies the behavior of rte_event_dev_stop().
> 
> This follows this mailing list discussion:
> http://dpdk.org/ml/archives/dev/2018-January/087484.html
> 
> Signed-off-by: Gage Eads <gage.eads@intel.com>

Acked-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>

Re: [PATCH v5] eventdev: add device stop flush callback

[Jerin Jacob]

-----Original Message-----
> Date: Tue, 3 Apr 2018 06:56:49 +0530
> From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> To: Gage Eads <gage.eads@intel.com>
> CC: dev@dpdk.org, harry.van.haaren@intel.com, hemant.agrawal@nxp.com,
>  bruce.richardson@intel.com, santosh.shukla@caviumnetworks.com,
>  nipun.gupta@nxp.com, liang.j.ma@intel.com
> Subject: Re: [dpdk-dev] [PATCH v5] eventdev: add device stop flush callback
> User-Agent: Mutt/1.9.4 (2018-02-28)
> 
> -----Original Message-----
> > Date: Mon, 2 Apr 2018 13:03:30 -0500
> > From: Gage Eads <gage.eads@intel.com>
> > To: dev@dpdk.org
> > CC: jerin.jacob@caviumnetworks.com, harry.van.haaren@intel.com,
> >  hemant.agrawal@nxp.com, bruce.richardson@intel.com,
> >  santosh.shukla@caviumnetworks.com, nipun.gupta@nxp.com,
> >  liang.j.ma@intel.com
> > Subject: [PATCH v5] eventdev: add device stop flush callback
> > X-Mailer: git-send-email 2.7.4
> > 
> > When an event device is stopped, it drains all event queues and ports.
> > These events may contain pointers, so to prevent memory leaks eventdev now
> > supports a user-provided flush callback that is called during the queue
> > drain process. This callback is stored in process memory, so the callback
> > must be registered by any process that may call rte_event_dev_stop().
> > 
> > This commit also clarifies the behavior of rte_event_dev_stop().
> > 
> > This follows this mailing list discussion:
> > http://dpdk.org/ml/archives/dev/2018-January/087484.html
> > 
> > Signed-off-by: Gage Eads <gage.eads@intel.com>
> 
> Acked-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>

Applied to dpdk-next-eventdev/master. Thanks.