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

[David Marchand <david.marchand@redhat.com>]

On Thu, Oct 3, 2024 at 11:46 AM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> 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@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");

Ok, this way seems nicer.
I'll let Robin submit a v2.


-- 
David Marchand