[dpdk-dev,v7,13/15] eal: add hotplug add/remove functions
Checks
Commit Message
Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
lib/librte_eal/bsdapp/eal/rte_eal_version.map | 2 +
lib/librte_eal/common/eal_common_dev.c | 68 +++++++++++++++++++++++++
lib/librte_eal/common/include/rte_dev.h | 28 ++++++++++
lib/librte_eal/linuxapp/eal/rte_eal_version.map | 2 +
4 files changed, 100 insertions(+)
Comments
29/06/2017 20:22, Jan Blunck:
> /**
> + * Hotplug add a given device to a specific bus.
> + *
> + * @param busname
> + * The bus name the device is added to.
> + * @param devname
> + * The device name. Based on this device name, eal will identify a driver
> + * capable of handling it and pass it to the driver probing function.
> + * @param devargs
> + * Device arguments to be passed to the driver.
> + * @return
> + * 0 on success, negative on error.
> + */
> +int rte_eal_hotplug_add(const char *busname, const char *devname,
> + const char *devargs);
After the hotplug, we may need to get the rte_device.
Should we add a struct **rte_device as parameter,
or should we add a helper function to get the rte_device
from busname and devname?
On Fri, Jun 30, 2017 at 11:06:03AM +0200, Thomas Monjalon wrote:
> 29/06/2017 20:22, Jan Blunck:
> > /**
> > + * Hotplug add a given device to a specific bus.
> > + *
> > + * @param busname
> > + * The bus name the device is added to.
> > + * @param devname
> > + * The device name. Based on this device name, eal will identify a driver
> > + * capable of handling it and pass it to the driver probing function.
> > + * @param devargs
> > + * Device arguments to be passed to the driver.
> > + * @return
> > + * 0 on success, negative on error.
> > + */
> > +int rte_eal_hotplug_add(const char *busname, const char *devname,
> > + const char *devargs);
>
> After the hotplug, we may need to get the rte_device.
> Should we add a struct **rte_device as parameter,
> or should we add a helper function to get the rte_device
> from busname and devname?
Also possible: return a struct *rte_device and set rte_errno on error.
On Fri, Jun 30, 2017 at 11:11:42AM +0200, Gaëtan Rivet wrote:
> On Fri, Jun 30, 2017 at 11:06:03AM +0200, Thomas Monjalon wrote:
> > 29/06/2017 20:22, Jan Blunck:
> > > /**
> > > + * Hotplug add a given device to a specific bus.
> > > + *
> > > + * @param busname
> > > + * The bus name the device is added to.
> > > + * @param devname
> > > + * The device name. Based on this device name, eal will identify a driver
> > > + * capable of handling it and pass it to the driver probing function.
> > > + * @param devargs
> > > + * Device arguments to be passed to the driver.
> > > + * @return
> > > + * 0 on success, negative on error.
> > > + */
> > > +int rte_eal_hotplug_add(const char *busname, const char *devname,
> > > + const char *devargs);
> >
> > After the hotplug, we may need to get the rte_device.
> > Should we add a struct **rte_device as parameter,
> > or should we add a helper function to get the rte_device
> > from busname and devname?
>
> Also possible: return a struct *rte_device and set rte_errno on error.
>
+1 for this option.
It seems a function is missing in this patch:
29/06/2017 20:22, Jan Blunck:
> +int rte_eal_hotplug_add(const char *busname, const char *devname,
> + const char *devargs)
> +{
> + struct rte_bus *bus;
> + struct rte_device *dev;
> + int ret;
> +
> + bus = rte_bus_find_by_name(busname);
> + if (!bus) {
> + RTE_LOG(ERR, EAL, "Cannot find bus (%s)\n", busname);
> + return -ENOENT;
> + }
> +
> + if (!bus->plug) {
> + RTE_LOG(ERR, EAL, "Function plug not supported by bus (%s)\n",
> + bus->name);
> + return -ENOTSUP;
> + }
> +
> + ret = bus->scan();
> + if (ret)
> + return ret;
> +
> + dev = bus->find_device(NULL, cmp_detached_dev_name, devname);
fatal error: use of undeclared identifier 'cmp_detached_dev_name'
On Fri, Jun 30, 2017 at 2:54 PM, Thomas Monjalon <thomas@monjalon.net> wrote:
> It seems a function is missing in this patch:
>
> 29/06/2017 20:22, Jan Blunck:
>> +int rte_eal_hotplug_add(const char *busname, const char *devname,
>> + const char *devargs)
>> +{
>> + struct rte_bus *bus;
>> + struct rte_device *dev;
>> + int ret;
>> +
>> + bus = rte_bus_find_by_name(busname);
>> + if (!bus) {
>> + RTE_LOG(ERR, EAL, "Cannot find bus (%s)\n", busname);
>> + return -ENOENT;
>> + }
>> +
>> + if (!bus->plug) {
>> + RTE_LOG(ERR, EAL, "Function plug not supported by bus (%s)\n",
>> + bus->name);
>> + return -ENOTSUP;
>> + }
>> +
>> + ret = bus->scan();
>> + if (ret)
>> + return ret;
>> +
>> + dev = bus->find_device(NULL, cmp_detached_dev_name, devname);
>
> fatal error: use of undeclared identifier 'cmp_detached_dev_name'
>
Sorry. I'll make them bisectable with the next drop today.
On Fri, Jun 30, 2017 at 11:20 AM, Bruce Richardson
<bruce.richardson@intel.com> wrote:
> On Fri, Jun 30, 2017 at 11:11:42AM +0200, Gaėtan Rivet wrote:
>> On Fri, Jun 30, 2017 at 11:06:03AM +0200, Thomas Monjalon wrote:
>> > 29/06/2017 20:22, Jan Blunck:
>> > > /**
>> > > + * Hotplug add a given device to a specific bus.
>> > > + *
>> > > + * @param busname
>> > > + * The bus name the device is added to.
>> > > + * @param devname
>> > > + * The device name. Based on this device name, eal will identify a driver
>> > > + * capable of handling it and pass it to the driver probing function.
>> > > + * @param devargs
>> > > + * Device arguments to be passed to the driver.
>> > > + * @return
>> > > + * 0 on success, negative on error.
>> > > + */
>> > > +int rte_eal_hotplug_add(const char *busname, const char *devname,
>> > > + const char *devargs);
>> >
>> > After the hotplug, we may need to get the rte_device.
>> > Should we add a struct **rte_device as parameter,
>> > or should we add a helper function to get the rte_device
>> > from busname and devname?
>>
>> Also possible: return a struct *rte_device and set rte_errno on error.
>>
> +1 for this option.
Given that the caller of this is usually something that injects events
from the system I wonder what it is going to do with a rte_device
reference. Additionally to what the caller knows anyway (name,
numa_node, devargs) it can check if a driver got assigned. Sure the
caller could upcast conditionally based on the busname ...
At this point I guess the control plane would anyway want to get
access to a high-level object, e.g. the rte_ethdev. I believe it is
better to decouple this through callbacks that can get registered with
individual buses.
30/06/2017 17:44, Jan Blunck:
> On Fri, Jun 30, 2017 at 11:20 AM, Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> > On Fri, Jun 30, 2017 at 11:11:42AM +0200, Gaėtan Rivet wrote:
> >> On Fri, Jun 30, 2017 at 11:06:03AM +0200, Thomas Monjalon wrote:
> >> > 29/06/2017 20:22, Jan Blunck:
> >> > > /**
> >> > > + * Hotplug add a given device to a specific bus.
> >> > > + *
> >> > > + * @param busname
> >> > > + * The bus name the device is added to.
> >> > > + * @param devname
> >> > > + * The device name. Based on this device name, eal will identify a driver
> >> > > + * capable of handling it and pass it to the driver probing function.
> >> > > + * @param devargs
> >> > > + * Device arguments to be passed to the driver.
> >> > > + * @return
> >> > > + * 0 on success, negative on error.
> >> > > + */
> >> > > +int rte_eal_hotplug_add(const char *busname, const char *devname,
> >> > > + const char *devargs);
> >> >
> >> > After the hotplug, we may need to get the rte_device.
> >> > Should we add a struct **rte_device as parameter,
> >> > or should we add a helper function to get the rte_device
> >> > from busname and devname?
> >>
> >> Also possible: return a struct *rte_device and set rte_errno on error.
> >>
> > +1 for this option.
>
> Given that the caller of this is usually something that injects events
> from the system I wonder what it is going to do with a rte_device
> reference. Additionally to what the caller knows anyway (name,
> numa_node, devargs) it can check if a driver got assigned. Sure the
> caller could upcast conditionally based on the busname ...
>
> At this point I guess the control plane would anyway want to get
> access to a high-level object, e.g. the rte_ethdev. I believe it is
> better to decouple this through callbacks that can get registered with
> individual buses.
I think Gaetan has an example of use of rte_device after plugging
with the failsafe PMD (managing slaves).
Anyway, it can be discussed later with a real example of use if needed.
We have a couple of weeks before freezing the API.
On Fri, Jun 30, 2017 at 06:03:17PM +0200, Thomas Monjalon wrote:
> 30/06/2017 17:44, Jan Blunck:
> > On Fri, Jun 30, 2017 at 11:20 AM, Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> > > On Fri, Jun 30, 2017 at 11:11:42AM +0200, Gaėtan Rivet wrote:
> > >> On Fri, Jun 30, 2017 at 11:06:03AM +0200, Thomas Monjalon wrote:
> > >> > 29/06/2017 20:22, Jan Blunck:
> > >> > > /**
> > >> > > + * Hotplug add a given device to a specific bus.
> > >> > > + *
> > >> > > + * @param busname
> > >> > > + * The bus name the device is added to.
> > >> > > + * @param devname
> > >> > > + * The device name. Based on this device name, eal will identify a driver
> > >> > > + * capable of handling it and pass it to the driver probing function.
> > >> > > + * @param devargs
> > >> > > + * Device arguments to be passed to the driver.
> > >> > > + * @return
> > >> > > + * 0 on success, negative on error.
> > >> > > + */
> > >> > > +int rte_eal_hotplug_add(const char *busname, const char *devname,
> > >> > > + const char *devargs);
> > >> >
> > >> > After the hotplug, we may need to get the rte_device.
> > >> > Should we add a struct **rte_device as parameter,
> > >> > or should we add a helper function to get the rte_device
> > >> > from busname and devname?
> > >>
> > >> Also possible: return a struct *rte_device and set rte_errno on error.
> > >>
> > > +1 for this option.
> >
> > Given that the caller of this is usually something that injects events
> > from the system I wonder what it is going to do with a rte_device
> > reference. Additionally to what the caller knows anyway (name,
> > numa_node, devargs) it can check if a driver got assigned. Sure the
> > caller could upcast conditionally based on the busname ...
> >
> > At this point I guess the control plane would anyway want to get
> > access to a high-level object, e.g. the rte_ethdev. I believe it is
> > better to decouple this through callbacks that can get registered with
> > individual buses.
>
> I think Gaetan has an example of use of rte_device after plugging
> with the failsafe PMD (managing slaves).
> Anyway, it can be discussed later with a real example of use if needed.
> We have a couple of weeks before freezing the API.
The rte_device should be accessible from the rte_eth_dev anyway so it
does not make much difference. As long as a handle on the device is
available. It is of course possible to add yet another callback to
search the device just plugged, but I don't see a reason here not to do
it in one pass.
On Fri, Jun 30, 2017 at 06:13:51PM +0200, Gaëtan Rivet wrote:
> On Fri, Jun 30, 2017 at 06:03:17PM +0200, Thomas Monjalon wrote:
> > 30/06/2017 17:44, Jan Blunck:
> > > On Fri, Jun 30, 2017 at 11:20 AM, Bruce Richardson
> > > <bruce.richardson@intel.com> wrote:
> > > > On Fri, Jun 30, 2017 at 11:11:42AM +0200, Gaėtan Rivet wrote:
> > > >> On Fri, Jun 30, 2017 at 11:06:03AM +0200, Thomas Monjalon wrote:
> > > >> > 29/06/2017 20:22, Jan Blunck:
> > > >> > > /**
> > > >> > > + * Hotplug add a given device to a specific bus.
> > > >> > > + *
> > > >> > > + * @param busname
> > > >> > > + * The bus name the device is added to.
> > > >> > > + * @param devname
> > > >> > > + * The device name. Based on this device name, eal will identify a driver
> > > >> > > + * capable of handling it and pass it to the driver probing function.
> > > >> > > + * @param devargs
> > > >> > > + * Device arguments to be passed to the driver.
> > > >> > > + * @return
> > > >> > > + * 0 on success, negative on error.
> > > >> > > + */
> > > >> > > +int rte_eal_hotplug_add(const char *busname, const char *devname,
> > > >> > > + const char *devargs);
> > > >> >
> > > >> > After the hotplug, we may need to get the rte_device.
> > > >> > Should we add a struct **rte_device as parameter,
> > > >> > or should we add a helper function to get the rte_device
> > > >> > from busname and devname?
> > > >>
> > > >> Also possible: return a struct *rte_device and set rte_errno on error.
> > > >>
> > > > +1 for this option.
> > >
> > > Given that the caller of this is usually something that injects events
> > > from the system I wonder what it is going to do with a rte_device
> > > reference. Additionally to what the caller knows anyway (name,
> > > numa_node, devargs) it can check if a driver got assigned. Sure the
> > > caller could upcast conditionally based on the busname ...
> > >
> > > At this point I guess the control plane would anyway want to get
> > > access to a high-level object, e.g. the rte_ethdev. I believe it is
> > > better to decouple this through callbacks that can get registered with
> > > individual buses.
> >
> > I think Gaetan has an example of use of rte_device after plugging
> > with the failsafe PMD (managing slaves).
> > Anyway, it can be discussed later with a real example of use if needed.
> > We have a couple of weeks before freezing the API.
>
> The rte_device should be accessible from the rte_eth_dev anyway so it
> does not make much difference. As long as a handle on the device is
> available. It is of course possible to add yet another callback to
> search the device just plugged, but I don't see a reason here not to do
> it in one pass.
>
At this point in the process we just need to get in what we can.
Given there is so much discussion I would suggest we apply what we have
now, but mark the new APIs as "experimental" at this point. That should
allow us to test them, and build upon them without holding us back if we
do need to change them in the next release. Once everyone is happy with
the final result, we can lock them down then. It seems premature
to do so now, with the current discussion, but on the other hand
it seems foolish to not put what work has been done thus far
into a release as a starting point.
my 2c.
/Bruce
30/06/2017 18:25, Bruce Richardson:
> On Fri, Jun 30, 2017 at 06:13:51PM +0200, Gaëtan Rivet wrote:
> > On Fri, Jun 30, 2017 at 06:03:17PM +0200, Thomas Monjalon wrote:
> > > 30/06/2017 17:44, Jan Blunck:
> > > > On Fri, Jun 30, 2017 at 11:20 AM, Bruce Richardson
> > > > <bruce.richardson@intel.com> wrote:
> > > > > On Fri, Jun 30, 2017 at 11:11:42AM +0200, Gaėtan Rivet wrote:
> > > > >> On Fri, Jun 30, 2017 at 11:06:03AM +0200, Thomas Monjalon wrote:
> > > > >> > 29/06/2017 20:22, Jan Blunck:
> > > > >> > > /**
> > > > >> > > + * Hotplug add a given device to a specific bus.
> > > > >> > > + *
> > > > >> > > + * @param busname
> > > > >> > > + * The bus name the device is added to.
> > > > >> > > + * @param devname
> > > > >> > > + * The device name. Based on this device name, eal will identify a driver
> > > > >> > > + * capable of handling it and pass it to the driver probing function.
> > > > >> > > + * @param devargs
> > > > >> > > + * Device arguments to be passed to the driver.
> > > > >> > > + * @return
> > > > >> > > + * 0 on success, negative on error.
> > > > >> > > + */
> > > > >> > > +int rte_eal_hotplug_add(const char *busname, const char *devname,
> > > > >> > > + const char *devargs);
> > > > >> >
> > > > >> > After the hotplug, we may need to get the rte_device.
> > > > >> > Should we add a struct **rte_device as parameter,
> > > > >> > or should we add a helper function to get the rte_device
> > > > >> > from busname and devname?
> > > > >>
> > > > >> Also possible: return a struct *rte_device and set rte_errno on error.
> > > > >>
> > > > > +1 for this option.
> > > >
> > > > Given that the caller of this is usually something that injects events
> > > > from the system I wonder what it is going to do with a rte_device
> > > > reference. Additionally to what the caller knows anyway (name,
> > > > numa_node, devargs) it can check if a driver got assigned. Sure the
> > > > caller could upcast conditionally based on the busname ...
> > > >
> > > > At this point I guess the control plane would anyway want to get
> > > > access to a high-level object, e.g. the rte_ethdev. I believe it is
> > > > better to decouple this through callbacks that can get registered with
> > > > individual buses.
> > >
> > > I think Gaetan has an example of use of rte_device after plugging
> > > with the failsafe PMD (managing slaves).
> > > Anyway, it can be discussed later with a real example of use if needed.
> > > We have a couple of weeks before freezing the API.
> >
> > The rte_device should be accessible from the rte_eth_dev anyway so it
> > does not make much difference. As long as a handle on the device is
> > available. It is of course possible to add yet another callback to
> > search the device just plugged, but I don't see a reason here not to do
> > it in one pass.
> >
> At this point in the process we just need to get in what we can.
>
> Given there is so much discussion I would suggest we apply what we have
> now, but mark the new APIs as "experimental" at this point. That should
> allow us to test them, and build upon them without holding us back if we
> do need to change them in the next release. Once everyone is happy with
> the final result, we can lock them down then. It seems premature
> to do so now, with the current discussion, but on the other hand
> it seems foolish to not put what work has been done thus far
> into a release as a starting point.
I agree.
Please Jan, add EXPERIMENTAL in the doxygen of the new hotplug API
for your next version. Thanks
@@ -166,6 +166,8 @@ DPDK_17.05 {
rte_bus_find_by_device;
rte_bus_find_device;
rte_cpu_is_supported;
+ rte_eal_hotplug_add;
+ rte_eal_hotplug_remove;
rte_log_dump;
rte_log_register;
rte_log_get_global_level;
@@ -92,3 +92,71 @@ int rte_eal_dev_detach(const char *name)
RTE_LOG(ERR, EAL, "Driver cannot detach the device (%s)\n", name);
return -EINVAL;
}
+
+int rte_eal_hotplug_add(const char *busname, const char *devname,
+ const char *devargs)
+{
+ struct rte_bus *bus;
+ struct rte_device *dev;
+ int ret;
+
+ bus = rte_bus_find_by_name(busname);
+ if (!bus) {
+ RTE_LOG(ERR, EAL, "Cannot find bus (%s)\n", busname);
+ return -ENOENT;
+ }
+
+ if (!bus->plug) {
+ RTE_LOG(ERR, EAL, "Function plug not supported by bus (%s)\n",
+ bus->name);
+ return -ENOTSUP;
+ }
+
+ ret = bus->scan();
+ if (ret)
+ return ret;
+
+ dev = bus->find_device(NULL, cmp_detached_dev_name, devname);
+ if (!dev) {
+ RTE_LOG(ERR, EAL, "Cannot find unplugged device (%s)\n",
+ devname);
+ return -EINVAL;
+ }
+
+ ret = bus->plug(dev, devargs);
+ if (ret)
+ RTE_LOG(ERR, EAL, "Driver cannot attach the device (%s)\n",
+ dev->name);
+ return ret;
+}
+
+int rte_eal_hotplug_remove(const char *busname, const char *devname)
+{
+ struct rte_bus *bus;
+ struct rte_device *dev;
+ int ret;
+
+ bus = rte_bus_find_by_name(busname);
+ if (!bus) {
+ RTE_LOG(ERR, EAL, "Cannot find bus (%s)\n", busname);
+ return -ENOENT;
+ }
+
+ if (!bus->unplug) {
+ RTE_LOG(ERR, EAL, "Function unplug not supported by bus (%s)\n",
+ bus->name);
+ return -ENOTSUP;
+ }
+
+ dev = bus->find_device(NULL, cmp_dev_name, devname);
+ if (!dev) {
+ RTE_LOG(ERR, EAL, "Cannot find plugged device (%s)\n", devname);
+ return -EINVAL;
+ }
+
+ ret = bus->unplug(dev);
+ if (ret)
+ RTE_LOG(ERR, EAL, "Driver cannot detach the device (%s)\n",
+ dev->name);
+ return ret;
+}
@@ -192,6 +192,34 @@ int rte_eal_dev_attach(const char *name, const char *devargs);
int rte_eal_dev_detach(const char *name);
/**
+ * Hotplug add a given device to a specific bus.
+ *
+ * @param busname
+ * The bus name the device is added to.
+ * @param devname
+ * The device name. Based on this device name, eal will identify a driver
+ * capable of handling it and pass it to the driver probing function.
+ * @param devargs
+ * Device arguments to be passed to the driver.
+ * @return
+ * 0 on success, negative on error.
+ */
+int rte_eal_hotplug_add(const char *busname, const char *devname,
+ const char *devargs);
+
+/**
+ * Hotplug remove a given device from a specific bus.
+ *
+ * @param busname
+ * The bus name the device is removed from.
+ * @param devname
+ * The device name being removed.
+ * @return
+ * 0 on success, negative on error.
+ */
+int rte_eal_hotplug_remove(const char *busname, const char *devname);
+
+/**
* Device comparison function.
*
* This type of function is used to compare an rte_device with arbitrary
@@ -170,6 +170,8 @@ DPDK_17.05 {
rte_bus_find_by_device;
rte_bus_find_device;
rte_cpu_is_supported;
+ rte_eal_hotplug_add;
+ rte_eal_hotplug_remove;
rte_intr_free_epoll_fd;
rte_log_dump;
rte_log_get_global_level;