On Mon, Oct 14, 2024 at 10:01 PM Stephen Hemminger
<step...@networkplumber.org> wrote:
>
> On Mon, 14 Oct 2024 21:32:37 +0200
> Robin Jarry <rja...@redhat.com> wrote:
>
> > While invoking telemetry commands (which may happen at any time, out of
> > control of the application), an application thread may concurrently
> > add/remove ports. The telemetry callbacks may then access partially
> > initialized/uninitialised ethdev data.
> >
> > Reuse the ethdev lock that protects port allocation/destruction and the
> > new telemetry callback register api that takes an additional private
> > argument. Pass eth_dev_telemetry_do as the main callback and the actual
> > endpoint callbacks as private argument.
> >
> > Fixes: c190daedb9b1 ("ethdev: add telemetry callbacks")
> > Cc: sta...@dpdk.org
> >
> > Signed-off-by: Robin Jarry <rja...@redhat.com>
> > Acked-by: Bruce Richardson <bruce.richard...@intel.com>
> > ---
> > lib/ethdev/rte_ethdev_telemetry.c | 66 ++++++++++++++++++++++---------
> > 1 file changed, 47 insertions(+), 19 deletions(-)
> >
> > diff --git a/lib/ethdev/rte_ethdev_telemetry.c
> > b/lib/ethdev/rte_ethdev_telemetry.c
> > index 6b873e7abe68..7599fa2852b6 100644
> > --- a/lib/ethdev/rte_ethdev_telemetry.c
> > +++ b/lib/ethdev/rte_ethdev_telemetry.c
> > @@ -1395,45 +1395,73 @@ eth_dev_handle_port_tm_node_caps(const char *cmd
> > __rte_unused,
> > return ret;
> > }
> >
> > +static int eth_dev_telemetry_do(const char *cmd, const char *params,
> > + void *arg, struct rte_tel_data *d)
> > +{
> > + int ret;
> > + telemetry_cb fn = arg;
> > + rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
> > + ret = fn(cmd, params, d);
> > + rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
> > + return ret;
> > +}
>
> If this happens often, and the function takes a long time (like doing i/o)
> it might be worth changing this to reader/writer in future.
Yes, this was an option mentionned when we discussed the issue in Montréal.
For now, a spinlock seems enough.
>
> Also, would be best to add a comment here as to what is being protected
> if you do another version.
I can add something when applying, like:
@@ -1400,6 +1400,7 @@ static int eth_dev_telemetry_do(const char *cmd,
const char *params,
{
int ret;
telemetry_cb fn = arg;
+ /* Protect against port removal while invoking callback,
calling ethdev API. */
rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
ret = fn(cmd, params, d);
rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
--
David Marchand
