Re: [dpdk-dev] [PATCH v3 1/4] mempool: add event callbacks

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 10/12/21 3:04 AM, Dmitry Kozlyuk wrote:
> Data path performance can benefit if the PMD knows which memory it will
> need to handle in advance, before the first mbuf is sent to the PMD.
> It is impractical, however, to consider all allocated memory for this
> purpose. Most often mbuf memory comes from mempools that can come and
> go. PMD can enumerate existing mempools on device start, but it also
> needs to track creation and destruction of mempools after the forwarding
> starts but before an mbuf from the new mempool is sent to the device.
> 
> Add an internal API to register callback for mempool life cycle events:
> * rte_mempool_event_callback_register()
> * rte_mempool_event_callback_unregister()
> Currently tracked events are:
> * RTE_MEMPOOL_EVENT_READY (after populating a mempool)
> * RTE_MEMPOOL_EVENT_DESTROY (before freeing a mempool)
> Provide a unit test for the new API.

Good idea.

> Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> Acked-by: Matan Azrad <matan@nvidia.com>

[snip]

I think it would be very useful to test two callbacks as well
including new mempool creation after one of callbacks
unregister. Plus register/unregister callbacks from a callback
itself. Feel free to drop it, since increasing test coverage
is almost endless :)

> diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
> index f57ecbd6fc..e2bf40aa09 100644
> --- a/lib/mempool/rte_mempool.h
> +++ b/lib/mempool/rte_mempool.h
> @@ -1774,6 +1774,62 @@ void rte_mempool_walk(void (*func)(struct rte_mempool *, void *arg),
>  int
>  rte_mempool_get_page_size(struct rte_mempool *mp, size_t *pg_sz);
>  
> +/**
> + * Mempool event type.
> + * @internal
> + */
> +enum rte_mempool_event {
> +	/** Occurs after a mempool is successfully populated. */

successfully -> fully ?

> +	RTE_MEMPOOL_EVENT_READY = 0,
> +	/** Occurs before destruction of a mempool begins. */
> +	RTE_MEMPOOL_EVENT_DESTROY = 1,
> +};
> +
> +/**
> + * @internal
> + * Mempool event callback.
> + */
> +typedef void (rte_mempool_event_callback)(
> +		enum rte_mempool_event event,
> +		struct rte_mempool *mp,
> +		void *user_data);
> +
> +/**
> + * @internal

I'd like to understand why the API is internal (not
experimental). I think reasons should be clear from
function description.

> + * Register a callback invoked on mempool life cycle event.
> + * Callbacks will be invoked in the process that creates the mempool.
> + *
> + * @param func
> + *   Callback function.
> + * @param user_data
> + *   User data.
> + *
> + * @return
> + *   0 on success, negative on failure and rte_errno is set.
> + */
> +__rte_internal
> +int
> +rte_mempool_event_callback_register(rte_mempool_event_callback *func,
> +				    void *user_data);
> +
> +/**
> + * @internal
> + * Unregister a callback added with rte_mempool_event_callback_register().
> + * @p func and @p user_data must exactly match registration parameters.
> + *
> + * @param func
> + *   Callback function.
> + * @param user_data
> + *   User data.
> + *
> + * @return
> + *   0 on success, negative on failure and rte_errno is set.
> + */
> +__rte_internal
> +int
> +rte_mempool_event_callback_unregister(rte_mempool_event_callback *func,
> +				      void *user_data);
> +
>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/lib/mempool/version.map b/lib/mempool/version.map
> index 9f77da6fff..1b7d7c5456 100644
> --- a/lib/mempool/version.map
> +++ b/lib/mempool/version.map
> @@ -64,3 +64,11 @@ EXPERIMENTAL {
>  	__rte_mempool_trace_ops_free;
>  	__rte_mempool_trace_set_ops_byname;
>  };
> +
> +INTERNAL {
> +	global:
> +
> +	# added in 21.11
> +	rte_mempool_event_callback_register;
> +	rte_mempool_event_callback_unregister;
> +};
>