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

[Jerin Jacob <jerin.jacob@caviumnetworks.com>]

-----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.