devargs: do not replace already inserted devargs
Checks
Commit Message
The devargs of a device can be replaced by a newly allocated one
when trying to probe again the same device (multi-process or
multi-ports scenarios). This is breaking some pointer references.
It can be avoided by copying the new content, freeing the new devargs,
and returning the already inserted pointer.
Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
This patch is not tested, I want to share the idea as soon as possible.
I hope it can fix some of the issues recently raised.
I think there are some mess in:
- scan functions and devargs
- vdev hotplug
We can revisit these areas in 19.02,
and consider this patch as a simple fix for 18.11.
---
drivers/bus/vdev/vdev.c | 6 ++--
lib/librte_eal/common/eal_common_dev.c | 3 +-
lib/librte_eal/common/eal_common_devargs.c | 37 +++++++++++++++++----
lib/librte_eal/common/include/rte_devargs.h | 4 ++-
4 files changed, 39 insertions(+), 11 deletions(-)
Comments
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, November 8, 2018 12:21 AM
> To: dev@dpdk.org
> Cc: gaetan.rivet@6wind.com; Zhang, Qi Z <qi.z.zhang@intel.com>; Guo, Jia
> <jia.guo@intel.com>; Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>
> Subject: [PATCH] devargs: do not replace already inserted devargs
>
> The devargs of a device can be replaced by a newly allocated one
> when trying to probe again the same device (multi-process or
> multi-ports scenarios). This is breaking some pointer references.
>
> It can be avoided by copying the new content, freeing the new devargs,
> and returning the already inserted pointer.
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
Tested-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
08/11/2018 12:25, Stojaczyk, Dariusz:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> >
> > The devargs of a device can be replaced by a newly allocated one
> > when trying to probe again the same device (multi-process or
> > multi-ports scenarios). This is breaking some pointer references.
> >
> > It can be avoided by copying the new content, freeing the new devargs,
> > and returning the already inserted pointer.
> >
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
>
> Tested-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Is it fixing any use case?
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, November 8, 2018 1:26 PM
> To: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>
> Cc: dev@dpdk.org; gaetan.rivet@6wind.com; Zhang, Qi Z
> <qi.z.zhang@intel.com>; Guo, Jia <jia.guo@intel.com>
> Subject: Re: [PATCH] devargs: do not replace already inserted devargs
>
> 08/11/2018 12:25, Stojaczyk, Dariusz:
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > >
> > > The devargs of a device can be replaced by a newly allocated one
> > > when trying to probe again the same device (multi-process or
> > > multi-ports scenarios). This is breaking some pointer references.
> > >
> > > It can be avoided by copying the new content, freeing the new devargs,
> > > and returning the already inserted pointer.
> > >
> > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> >
> > Tested-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
>
> Is it fixing any use case?
>
Of course it is. I was previously seeing a regression with the following scenario:
1. hotplug device in the primary process
2. start a secodary process
3. hotplug device in secondary -> primary segfaults
And now it's working.
D.
08/11/2018 13:35, Stojaczyk, Dariusz:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 08/11/2018 12:25, Stojaczyk, Dariusz:
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > >
> > > > The devargs of a device can be replaced by a newly allocated one
> > > > when trying to probe again the same device (multi-process or
> > > > multi-ports scenarios). This is breaking some pointer references.
> > > >
> > > > It can be avoided by copying the new content, freeing the new devargs,
> > > > and returning the already inserted pointer.
> > > >
> > > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > >
> > > Tested-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> >
> > Is it fixing any use case?
>
> Of course it is. I was previously seeing a regression with the following scenario:
> 1. hotplug device in the primary process
> 2. start a secodary process
> 3. hotplug device in secondary -> primary segfaults
>
> And now it's working.
Good to know!
Thank you
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Stojaczyk, Dariusz
> Sent: Thursday, November 8, 2018 13:25
> To: Thomas Monjalon <thomas@monjalon.net>; dev@dpdk.org
> Cc: gaetan.rivet@6wind.com; Zhang, Qi Z <qi.z.zhang@intel.com>; Guo, Jia
> <jia.guo@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] devargs: do not replace already inserted
> devargs
>
>
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Sent: Thursday, November 8, 2018 12:21 AM
> > To: dev@dpdk.org
> > Cc: gaetan.rivet@6wind.com; Zhang, Qi Z <qi.z.zhang@intel.com>; Guo,
> > Jia <jia.guo@intel.com>; Stojaczyk, Dariusz
> > <dariusz.stojaczyk@intel.com>
> > Subject: [PATCH] devargs: do not replace already inserted devargs
> >
> > The devargs of a device can be replaced by a newly allocated one when
> > trying to probe again the same device (multi-process or multi-ports
> > scenarios). This is breaking some pointer references.
> >
> > It can be avoided by copying the new content, freeing the new devargs,
> > and returning the already inserted pointer.
> >
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > ---
>
> Tested-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Attaching the representors caused segmentation fault.
The scenario:
- setup with SRIOV enabled, PF plus N VF
- switchdev mode enabled (have representor for E-Switch ports)
- run testpmd specifying PF only, one device probed and one port is created
- trying to attach representor with port attach command
- segmentation fault occurs within rte_eth_devargs_parse ()
The patch fixes this issue.
Tested-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, November 8, 2018 5:26 AM
> To: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>
> Cc: dev@dpdk.org; gaetan.rivet@6wind.com; Zhang, Qi Z
> <qi.z.zhang@intel.com>; Guo, Jia <jia.guo@intel.com>
> Subject: Re: [PATCH] devargs: do not replace already inserted devargs
>
> 08/11/2018 12:25, Stojaczyk, Dariusz:
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > >
> > > The devargs of a device can be replaced by a newly allocated one
> > > when trying to probe again the same device (multi-process or
> > > multi-ports scenarios). This is breaking some pointer references.
> > >
> > > It can be avoided by copying the new content, freeing the new
> > > devargs, and returning the already inserted pointer.
> > >
> > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> >
> > Tested-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
>
> Is it fixing any use case?
Tested-by: Qi Zhang <qi.z.zhang@intel.com>
The patch also fix below scenario
attach net_af_packet,iface=enp50s0f0
attach net_af_packet,iface=enp50s0f0 (failed as expected)
detach net_af_packet (failed)
Thanks!
Qi
>
08/11/2018 18:12, Zhang, Qi Z:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 08/11/2018 12:25, Stojaczyk, Dariusz:
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > >
> > > > The devargs of a device can be replaced by a newly allocated one
> > > > when trying to probe again the same device (multi-process or
> > > > multi-ports scenarios). This is breaking some pointer references.
> > > >
> > > > It can be avoided by copying the new content, freeing the new
> > > > devargs, and returning the already inserted pointer.
> > > >
> > > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > >
> > > Tested-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> >
> > Is it fixing any use case?
>
> Tested-by: Qi Zhang <qi.z.zhang@intel.com>
>
> The patch also fix below scenario
>
> attach net_af_packet,iface=enp50s0f0
> attach net_af_packet,iface=enp50s0f0 (failed as expected)
> detach net_af_packet (failed)
Applied
@@ -224,7 +224,6 @@ insert_vdev(const char *name, const char *args,
}
dev->device.bus = &rte_vdev_bus;
- dev->device.devargs = devargs;
dev->device.numa_node = SOCKET_ID_ANY;
dev->device.name = devargs->name;
@@ -238,9 +237,10 @@ insert_vdev(const char *name, const char *args,
goto fail;
}
- TAILQ_INSERT_TAIL(&vdev_device_list, dev, next);
if (init)
- rte_devargs_insert(devargs);
+ rte_devargs_insert(&devargs);
+ dev->device.devargs = devargs;
+ TAILQ_INSERT_TAIL(&vdev_device_list, dev, next);
if (p_dev)
*p_dev = dev;
@@ -150,10 +150,11 @@ local_dev_probe(const char *devargs, struct rte_device **new_dev)
goto err_devarg;
}
- ret = rte_devargs_insert(da);
+ ret = rte_devargs_insert(&da);
if (ret)
goto err_devarg;
+ /* the rte_devargs will be referenced in the matching rte_device */
ret = da->bus->scan();
if (ret)
goto err_devarg;
@@ -263,14 +263,39 @@ rte_devargs_parsef(struct rte_devargs *da, const char *format, ...)
}
int __rte_experimental
-rte_devargs_insert(struct rte_devargs *da)
+rte_devargs_insert(struct rte_devargs **da)
{
- int ret;
+ struct rte_devargs *listed_da;
+ void *tmp;
+
+ if (*da == NULL || (*da)->bus == NULL)
+ return -1;
+
+ TAILQ_FOREACH_SAFE(listed_da, &devargs_list, next, tmp) {
+ if (listed_da == *da)
+ /* devargs already in the list */
+ return 0;
+ if (strcmp(listed_da->bus->name, (*da)->bus->name) == 0 &&
+ strcmp(listed_da->name, (*da)->name) == 0) {
+ /* device already in devargs list, must be updated */
+ listed_da->type = (*da)->type;
+ listed_da->policy = (*da)->policy;
+ free(listed_da->args);
+ listed_da->args = (*da)->args;
+ listed_da->bus = (*da)->bus;
+ listed_da->cls = (*da)->cls;
+ listed_da->bus_str = (*da)->bus_str;
+ listed_da->cls_str = (*da)->cls_str;
+ listed_da->data = (*da)->data;
+ /* replace provided devargs with found one */
+ free(*da);
+ *da = listed_da;
+ return 0;
+ }
+ }
- ret = rte_devargs_remove(da);
- if (ret < 0)
- return ret;
- TAILQ_INSERT_TAIL(&devargs_list, da, next);
+ /* new devargs in the list */
+ TAILQ_INSERT_TAIL(&devargs_list, *da, next);
return 0;
}
@@ -146,6 +146,8 @@ __attribute__((format(printf, 2, 0)));
*
* @param da
* The devargs structure to insert.
+ * If a devargs for the same device is already inserted,
+ * it will be updated and returned. It means *da pointer can change.
*
* @return
* - 0 on success
@@ -153,7 +155,7 @@ __attribute__((format(printf, 2, 0)));
*/
__rte_experimental
int
-rte_devargs_insert(struct rte_devargs *da);
+rte_devargs_insert(struct rte_devargs **da);
/**
* Add a device to the user device list