diff mbox

[dpdk-dev,v3,2/8] devargs: introduce removal function

Message ID 7d819b3f9666620afa3324e47de7698a7e203328.1499814957.git.gaetan.rivet@6wind.com (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Gaëtan Rivet July 11, 2017, 11:25 p.m. UTC 
  Hotplug support introduces the possibility of removing devices from the
system. Allocated resources must be freed.

Extend the rte_devargs API to allow freeing allocated resources.

This API is experimental and bound to change. It is currently designed
as a symetrical to rte_eal_devargs_add(), but the latter will evolve
shortly anyway.

Its DEVTYPE parameter is currently only used to specify scan policies,
and those will evolve in the next release. This evolution should
rationalize the rte_devargs API.

As such, the proposed API here is not the most convenient, but is
taylored to follow the current design and integrate easily with its main
use within rte_eal_hotplug_* functions.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
 lib/librte_eal/common/eal_common_devargs.c      | 19 +++++++++++++++++++
 lib/librte_eal/common/include/rte_devargs.h     | 18 ++++++++++++++++++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
 4 files changed, 39 insertions(+)

Comments

Jan Blunck July 12, 2017, 8:27 a.m. UTC | #1 
On Tue, Jul 11, 2017 at 7:25 PM, Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
> Hotplug support introduces the possibility of removing devices from the
> system. Allocated resources must be freed.
>
> Extend the rte_devargs API to allow freeing allocated resources.
>
> This API is experimental and bound to change. It is currently designed
> as a symetrical to rte_eal_devargs_add(), but the latter will evolve
> shortly anyway.
>
> Its DEVTYPE parameter is currently only used to specify scan policies,
> and those will evolve in the next release. This evolution should
> rationalize the rte_devargs API.
>
> As such, the proposed API here is not the most convenient, but is
> taylored to follow the current design and integrate easily with its main
> use within rte_eal_hotplug_* functions.

I don't think that this is safe to do. The rte_device is using a
reference to the allocated rte_devargs so before we remove it we need
to make sure that the device is gone.

Besides that the comment says that its current user is inside the EAL
so I wonder why it is exported.

> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> ---
>  lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
>  lib/librte_eal/common/eal_common_devargs.c      | 19 +++++++++++++++++++
>  lib/librte_eal/common/include/rte_devargs.h     | 18 ++++++++++++++++++
>  lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
>  4 files changed, 39 insertions(+)
>
> diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> index 381f895..40cd523 100644
> --- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> +++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> @@ -207,6 +207,7 @@ EXPERIMENTAL {
>         global:
>
>         rte_eal_devargs_parse;
> +       rte_eal_devargs_remove;
>         rte_eal_hotplug_add;
>         rte_eal_hotplug_remove;
>
> diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
> index 49d43a3..bcdee13 100644
> --- a/lib/librte_eal/common/eal_common_devargs.c
> +++ b/lib/librte_eal/common/eal_common_devargs.c
> @@ -41,6 +41,7 @@
>  #include <string.h>
>
>  #include <rte_devargs.h>
> +#include <rte_tailq.h>
>  #include "eal_private.h"
>
>  /** Global list of user devices */
> @@ -182,6 +183,24 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
>         return -1;
>  }
>
> +int
> +rte_eal_devargs_remove(const char *busname, const char *devname)
> +{
> +       struct rte_devargs *d;
> +       void *tmp;
> +
> +       TAILQ_FOREACH_SAFE(d, &devargs_list, next, tmp) {
> +               if (strcmp(d->bus->name, busname) == 0 &&
> +                   strcmp(d->name, devname) == 0) {
> +                       TAILQ_REMOVE(&devargs_list, d, next);
> +                       free(d->args);
> +                       free(d);
> +                       return 0;
> +               }
> +       }
> +       return 1;
> +}
> +
>  /* count the number of devices of a specified type */
>  unsigned int
>  rte_eal_devargs_type_count(enum rte_devtype devtype)
> diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
> index a0427cd..36453b6 100644
> --- a/lib/librte_eal/common/include/rte_devargs.h
> +++ b/lib/librte_eal/common/include/rte_devargs.h
> @@ -163,6 +163,24 @@ rte_eal_devargs_parse(const char *dev,
>  int rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str);
>
>  /**
> + * Remove a device from the user device list.
> + * Its resources are freed.
> + * If the devargs cannot be found, nothing happens.
> + *
> + * @param busname
> + *   bus name of the devargs to remove.
> + *
> + * @param devname
> + *   device name of the devargs to remove.
> + *
> + * @return
> + *   0 on success.
> + *   <0 on error.
> + *   >0 if the devargs was not within the user device list.
> + */
> +int rte_eal_devargs_remove(const char *busname, const char *devname);
> +
> +/**
>   * Count the number of user devices of a specified type
>   *
>   * @param devtype
> diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> index 0f9e009..a8ee349 100644
> --- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> +++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> @@ -212,6 +212,7 @@ EXPERIMENTAL {
>         global:
>
>         rte_eal_devargs_parse;
> +       rte_eal_devargs_remove;
>         rte_eal_hotplug_add;
>         rte_eal_hotplug_remove;
>
> --
> 2.1.4
>
Gaëtan Rivet July 12, 2017, 5:22 p.m. UTC | #2 
On Wed, Jul 12, 2017 at 04:27:34AM -0400, Jan Blunck wrote:
> On Tue, Jul 11, 2017 at 7:25 PM, Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
> > Hotplug support introduces the possibility of removing devices from the
> > system. Allocated resources must be freed.
> >
> > Extend the rte_devargs API to allow freeing allocated resources.
> >
> > This API is experimental and bound to change. It is currently designed
> > as a symetrical to rte_eal_devargs_add(), but the latter will evolve
> > shortly anyway.
> >
> > Its DEVTYPE parameter is currently only used to specify scan policies,
> > and those will evolve in the next release. This evolution should
> > rationalize the rte_devargs API.
> >
> > As such, the proposed API here is not the most convenient, but is
> > taylored to follow the current design and integrate easily with its main
> > use within rte_eal_hotplug_* functions.
> 
> I don't think that this is safe to do. The rte_device is using a
> reference to the allocated rte_devargs so before we remove it we need
> to make sure that the device is gone.
> 

I think you're right. No bus should need to use the rte_devargs to
remove a device, but we cannot be sure.

> Besides that the comment says that its current user is inside the EAL
> so I wonder why it is exported.
> 

I agree that there isn't much use outside. However, the converse
rte_eal_devargs_add is exposed so it makes sense to also offer the
possibility of removing one.

But it will all change with the deprecation of rte_eal_devargs_add
anyway, so let's not expose it if there is no need.

> > Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> > ---
> >  lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
> >  lib/librte_eal/common/eal_common_devargs.c      | 19 +++++++++++++++++++
> >  lib/librte_eal/common/include/rte_devargs.h     | 18 ++++++++++++++++++
> >  lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
> >  4 files changed, 39 insertions(+)
> >
> > diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> > index 381f895..40cd523 100644
> > --- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> > +++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> > @@ -207,6 +207,7 @@ EXPERIMENTAL {
> >         global:
> >
> >         rte_eal_devargs_parse;
> > +       rte_eal_devargs_remove;
> >         rte_eal_hotplug_add;
> >         rte_eal_hotplug_remove;
> >
> > diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
> > index 49d43a3..bcdee13 100644
> > --- a/lib/librte_eal/common/eal_common_devargs.c
> > +++ b/lib/librte_eal/common/eal_common_devargs.c
> > @@ -41,6 +41,7 @@
> >  #include <string.h>
> >
> >  #include <rte_devargs.h>
> > +#include <rte_tailq.h>
> >  #include "eal_private.h"
> >
> >  /** Global list of user devices */
> > @@ -182,6 +183,24 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
> >         return -1;
> >  }
> >
> > +int
> > +rte_eal_devargs_remove(const char *busname, const char *devname)
> > +{
> > +       struct rte_devargs *d;
> > +       void *tmp;
> > +
> > +       TAILQ_FOREACH_SAFE(d, &devargs_list, next, tmp) {
> > +               if (strcmp(d->bus->name, busname) == 0 &&
> > +                   strcmp(d->name, devname) == 0) {
> > +                       TAILQ_REMOVE(&devargs_list, d, next);
> > +                       free(d->args);
> > +                       free(d);
> > +                       return 0;
> > +               }
> > +       }
> > +       return 1;
> > +}
> > +
> >  /* count the number of devices of a specified type */
> >  unsigned int
> >  rte_eal_devargs_type_count(enum rte_devtype devtype)
> > diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
> > index a0427cd..36453b6 100644
> > --- a/lib/librte_eal/common/include/rte_devargs.h
> > +++ b/lib/librte_eal/common/include/rte_devargs.h
> > @@ -163,6 +163,24 @@ rte_eal_devargs_parse(const char *dev,
> >  int rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str);
> >
> >  /**
> > + * Remove a device from the user device list.
> > + * Its resources are freed.
> > + * If the devargs cannot be found, nothing happens.
> > + *
> > + * @param busname
> > + *   bus name of the devargs to remove.
> > + *
> > + * @param devname
> > + *   device name of the devargs to remove.
> > + *
> > + * @return
> > + *   0 on success.
> > + *   <0 on error.
> > + *   >0 if the devargs was not within the user device list.
> > + */
> > +int rte_eal_devargs_remove(const char *busname, const char *devname);
> > +
> > +/**
> >   * Count the number of user devices of a specified type
> >   *
> >   * @param devtype
> > diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> > index 0f9e009..a8ee349 100644
> > --- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> > +++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> > @@ -212,6 +212,7 @@ EXPERIMENTAL {
> >         global:
> >
> >         rte_eal_devargs_parse;
> > +       rte_eal_devargs_remove;
> >         rte_eal_hotplug_add;
> >         rte_eal_hotplug_remove;
> >
> > --
> > 2.1.4
> >
diff mbox

Patch

diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
index 381f895..40cd523 100644
--- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
@@ -207,6 +207,7 @@  EXPERIMENTAL {
 	global:
 
 	rte_eal_devargs_parse;
+	rte_eal_devargs_remove;
 	rte_eal_hotplug_add;
 	rte_eal_hotplug_remove;
 
diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index 49d43a3..bcdee13 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -41,6 +41,7 @@ 
 #include <string.h>
 
 #include <rte_devargs.h>
+#include <rte_tailq.h>
 #include "eal_private.h"
 
 /** Global list of user devices */
@@ -182,6 +183,24 @@  rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 	return -1;
 }
 
+int
+rte_eal_devargs_remove(const char *busname, const char *devname)
+{
+	struct rte_devargs *d;
+	void *tmp;
+
+	TAILQ_FOREACH_SAFE(d, &devargs_list, next, tmp) {
+		if (strcmp(d->bus->name, busname) == 0 &&
+		    strcmp(d->name, devname) == 0) {
+			TAILQ_REMOVE(&devargs_list, d, next);
+			free(d->args);
+			free(d);
+			return 0;
+		}
+	}
+	return 1;
+}
+
 /* count the number of devices of a specified type */
 unsigned int
 rte_eal_devargs_type_count(enum rte_devtype devtype)
diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
index a0427cd..36453b6 100644
--- a/lib/librte_eal/common/include/rte_devargs.h
+++ b/lib/librte_eal/common/include/rte_devargs.h
@@ -163,6 +163,24 @@  rte_eal_devargs_parse(const char *dev,
 int rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str);
 
 /**
+ * Remove a device from the user device list.
+ * Its resources are freed.
+ * If the devargs cannot be found, nothing happens.
+ *
+ * @param busname
+ *   bus name of the devargs to remove.
+ *
+ * @param devname
+ *   device name of the devargs to remove.
+ *
+ * @return
+ *   0 on success.
+ *   <0 on error.
+ *   >0 if the devargs was not within the user device list.
+ */
+int rte_eal_devargs_remove(const char *busname, const char *devname);
+
+/**
  * Count the number of user devices of a specified type
  *
  * @param devtype
diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
index 0f9e009..a8ee349 100644
--- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
@@ -212,6 +212,7 @@  EXPERIMENTAL {
 	global:
 
 	rte_eal_devargs_parse;
+	rte_eal_devargs_remove;
 	rte_eal_hotplug_add;
 	rte_eal_hotplug_remove;