[dpdk-dev,v4,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
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(-)
Comments
> 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?
> -----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
> 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?
(+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?
>
>
>
-----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.
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?
> >
> >
> >
>
-----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>
@@ -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,
@@ -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,
@@ -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,
@@ -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,
@@ -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,
@@ -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,
@@ -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)
{
@@ -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 */
@@ -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 */
};
/**
@@ -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;