On Thu, Oct 3, 2024 at 11:46 AM Bruce Richardson
<bruce.richard...@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 <rja...@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
