[dpdk-dev,v2,1/2] eventdev: add device stop flush callback
Checks
Commit 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(-)
Comments
-----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.
> -----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
-----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.
@@ -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)
{
@@ -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
@@ -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;