[PATCH 2/2] ethdev: fix race on ports for telemetry commands

[Bruce Richardson]

On Wed, Oct 02, 2024 at 09:26:10PM +0200, Robin Jarry wrote:
> David Marchand, Oct 02, 2024 at 21:18:
> > On Wed, Oct 2, 2024 at 9:09 PM Robin Jarry <rjarry at redhat.com> wrote:
> > > I was going to suggest adding a rte_spinlock_t* parameter to a new
> > > telemetry register function that would need to be held while the
> > > callback is invoked. Or if we want to keep doors open to other kinds of
> > > lock, a wrapper callback.
> > 
> > Well, as you had experimented this approach, we know this does not
> > work: the ethdev lock is in dpdk shared memory which is not available
> > yet at the time RTE_INIT() is called.
> > 
> > A single callback is strange, I guess you mean pre/post callbacks then.
> 
> It could be a single function that will wrap the callbacks. E.g.:
> 
> static int
> eth_dev_telemetry_with_lock(
>    telemetry_cb fn, const char *cmd, const char *params, struct rte_tel_data *d)
> {
>    int ret;
>    rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
>    ret = fn(cmd, params, d);
>    rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
>    return ret;
> }
> 
> RTE_INIT(ethdev_init_telemetry)
> {
>    ....
>    rte_telemetry_register_cmd("/ethdev/stats", eth_dev_handle_port_stats,
>            "Returns the common stats for a port. Parameters: int port_id",
>            eth_dev_telemetry_with_lock);
>    ....
> }
> 
> I'm not sure which solution is the uglier :D
>

I don't actually mind this latter solution, except that the order of the
parameters should be reversed (and it breaks the ABI, unless we add a
special new function for it) For me, the wrapper function should be the
main callback, and the real (unwrapped) function the extra parameter to be
called. That extra parameter to callbacks should just be a generic pointer,
so it can be data or function that is passed around.

	rte_telemetry_register_param_cmd(const char *cmd, telemetry_cb fn, 
			void *param, const char *help)

Or more specifically:


    rte_telemetry_register_param_cmd("/ethdev/stats",
            eth_dev_telemetry_with_lock, /* callback */
            eth_dev_handle_port_stats,   /* parameter */
            "Returns the common stats for a port. Parameters: int port_id");

/Bruce

PS: Yes, I realise that using void * as function pointers is not always
recommended, but we already use dlsym which does so!