[dpdk-dev] bus/vdev: add custom scan hook

  The scan callback allows to spawn a vdev automatically
given some custom scan rules.
It is especially useful to create a TAP device automatically
connected to a netdevice as remote.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
warning: to be tested
---
 drivers/bus/vdev/rte_bus_vdev.h | 29 ++++++++++++++++
 drivers/bus/vdev/vdev.c         | 75 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 104 insertions(+)
  

01/12/2017 01:36, Thomas Monjalon:
> The scan callback allows to spawn a vdev automatically
> given some custom scan rules.
> It is especially useful to create a TAP device automatically
> connected to a netdevice as remote.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
> warning: to be tested
> ---
>  drivers/bus/vdev/rte_bus_vdev.h | 29 ++++++++++++++++
>  drivers/bus/vdev/vdev.c         | 75 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 104 insertions(+)

self-review: the new functions must be added in the .map file!
  

Hi Thomas,

Please help us to understand why we need this.


On 12/1/2017 8:36 AM, Thomas Monjalon wrote:
> The scan callback allows to spawn a vdev automatically
> given some custom scan rules.

These two new APIs (rte_vdev_add_custom_scan and 
rte_vdev_remove_custom_scan) are called by applications?
If so, why not just constructing them in the parameters before passing 
to rte_eal_init?

> It is especially useful to create a TAP device automatically
> connected to a netdevice as remote.

It sounds like an use case. Without this patch, I suppose we can already 
do this?

Thanks,
Jianfeng

>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
> warning: to be tested
> ---
>   drivers/bus/vdev/rte_bus_vdev.h | 29 ++++++++++++++++
>   drivers/bus/vdev/vdev.c         | 75 +++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 104 insertions(+)
[...]
  

Hi,

01/12/2017 06:48, Tan, Jianfeng:
> Hi Thomas,
> 
> Please help us to understand why we need this.
> 
> 
> On 12/1/2017 8:36 AM, Thomas Monjalon wrote:
> > The scan callback allows to spawn a vdev automatically
> > given some custom scan rules.
> 
> These two new APIs (rte_vdev_add_custom_scan and 
> rte_vdev_remove_custom_scan) are called by applications?

Yes, the application can use it but it can also be used by a DPDK
driver or library.

> If so, why not just constructing them in the parameters before passing 
> to rte_eal_init?

It is not only for initialization because it will also work when
we will have some hotplug mechanism where the scan is run during run-time.

> > It is especially useful to create a TAP device automatically
> > connected to a netdevice as remote.
> 
> It sounds like an use case. Without this patch, I suppose we can already 
> do this?

Yes we can already update the devargs list.
But this hook will help to do it on hotplug events.
  

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Friday, December 1, 2017 4:42 PM
> To: Tan, Jianfeng
> Cc: dev@dpdk.org
> Subject: Re: [PATCH] bus/vdev: add custom scan hook
> 
> Hi,
> 
> 01/12/2017 06:48, Tan, Jianfeng:
> > Hi Thomas,
> >
> > Please help us to understand why we need this.
> >
> >
> > On 12/1/2017 8:36 AM, Thomas Monjalon wrote:
> > > The scan callback allows to spawn a vdev automatically
> > > given some custom scan rules.
> >
> > These two new APIs (rte_vdev_add_custom_scan and
> > rte_vdev_remove_custom_scan) are called by applications?
> 
> Yes, the application can use it but it can also be used by a DPDK
> driver or library.
> 
> > If so, why not just constructing them in the parameters before passing
> > to rte_eal_init?
> 
> It is not only for initialization because it will also work when
> we will have some hotplug mechanism where the scan is run during run-time.
> 
> > > It is especially useful to create a TAP device automatically
> > > connected to a netdevice as remote.
> >
> > It sounds like an use case. Without this patch, I suppose we can already
> > do this?
> 
> Yes we can already update the devargs list.
> But this hook will help to do it on hotplug events.

Do you mean, we will have the udev monitoring framework, and after monitoring the uevent (hotplug), we want to automatically add a vdev (tap) for that detected physical device which is managed by kernel?

Considering the scan hook is added in bus->parse(), how this can happen automatically?

Thanks,
Jianfeng
  

04/12/2017 09:08, Tan, Jianfeng:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 
> > Hi,
> > 
> > 01/12/2017 06:48, Tan, Jianfeng:
> > > Hi Thomas,
> > >
> > > Please help us to understand why we need this.
> > >
> > >
> > > On 12/1/2017 8:36 AM, Thomas Monjalon wrote:
> > > > The scan callback allows to spawn a vdev automatically
> > > > given some custom scan rules.
> > >
> > > These two new APIs (rte_vdev_add_custom_scan and
> > > rte_vdev_remove_custom_scan) are called by applications?
> > 
> > Yes, the application can use it but it can also be used by a DPDK
> > driver or library.
> > 
> > > If so, why not just constructing them in the parameters before passing
> > > to rte_eal_init?
> > 
> > It is not only for initialization because it will also work when
> > we will have some hotplug mechanism where the scan is run during run-time.
> > 
> > > > It is especially useful to create a TAP device automatically
> > > > connected to a netdevice as remote.
> > >
> > > It sounds like an use case. Without this patch, I suppose we can already
> > > do this?
> > 
> > Yes we can already update the devargs list.
> > But this hook will help to do it on hotplug events.
> 
> Do you mean, we will have the udev monitoring framework, and after monitoring the uevent (hotplug), we want to automatically add a vdev (tap) for that detected physical device which is managed by kernel?

Yes, we get the hotplug event first.
Then the kernel may manage it.
And at last, the DPDK hook decide to connect a vdev to it.
Note that it would work for other vdevs like af_packet or pcap.

> Considering the scan hook is added in bus->parse(), how this can happen automatically?

The hook is in bus->scan().
I think we should launch a bus scan when there is a new device event.
  

On 12/4/2017 5:31 PM, Thomas Monjalon wrote:
>
> The hook is in bus->scan().
> I think we should launch a bus scan when there is a new device event.

That's what I'm trying to say. We finally need to execute a handler as 
of a device event to finish the job.

Then why not just keep the vdev plug in that handler, by calling 
rte_dev_hotplug_add()?

Thanks,
Jianfeng
  

05/12/2017 09:27, Tan, Jianfeng:
> 
> On 12/4/2017 5:31 PM, Thomas Monjalon wrote:
> >
> > The hook is in bus->scan().
> > I think we should launch a bus scan when there is a new device event.
> 
> That's what I'm trying to say. We finally need to execute a handler as 
> of a device event to finish the job.

Please be more specific, I am not sure to understand.

> Then why not just keep the vdev plug in that handler, by calling 
> rte_dev_hotplug_add()?

Which handler?
  

On 12/5/2017 4:41 PM, Thomas Monjalon wrote:
>
> 05/12/2017 09:27, Tan, Jianfeng:
>
> >
>
> > On 12/4/2017 5:31 PM, Thomas Monjalon wrote:
>
> > >
>
> > > The hook is in bus->scan().
>
> > > I think we should launch a bus scan when there is a new device event.
>
> >
>
> > That's what I'm trying to say. We finally need to execute a handler as
>
> > of a device event to finish the job.
>
> Please be more specific, I am not sure to understand.
>

By the handler, I mean when we monitor the udev by select/poll/epoll and 
device uevents come, the application will execute a handler (or just a 
function) for each of such uevent. Then why not adding the vdev there?

Thanks,
Jianfeng
  

05/12/2017 14:56, Tan, Jianfeng:
> 
> On 12/5/2017 4:41 PM, Thomas Monjalon wrote:
> >
> > 05/12/2017 09:27, Tan, Jianfeng:
> >
> > >
> >
> > > On 12/4/2017 5:31 PM, Thomas Monjalon wrote:
> >
> > > >
> >
> > > > The hook is in bus->scan().
> >
> > > > I think we should launch a bus scan when there is a new device event.
> >
> > >
> >
> > > That's what I'm trying to say. We finally need to execute a handler as
> >
> > > of a device event to finish the job.
> >
> > Please be more specific, I am not sure to understand.
> >
> 
> By the handler, I mean when we monitor the udev by select/poll/epoll and 
> device uevents come, the application will execute a handler (or just a 
> function) for each of such uevent. Then why not adding the vdev there?

Because it must work for hotplug, and initial scan too.
We can also think to application requiring a manual scan.
The bus scan is the right place to have every scans called. That's simple.
  

> From: Thomas Monjalon [mailto:thomas@monjalon.net]
 > Sent: Tuesday, December 5, 2017 11:21 PM
 > To: Tan, Jianfeng
 > Cc: dev@dpdk.org
 > Subject: Re: [PATCH] bus/vdev: add custom scan hook
 >
 > 05/12/2017 14:56, Tan, Jianfeng:
 > >
 > > On 12/5/2017 4:41 PM, Thomas Monjalon wrote:
 > > >
 > > > 05/12/2017 09:27, Tan, Jianfeng:
 > > >
 > > > >
 > > >
 > > > > On 12/4/2017 5:31 PM, Thomas Monjalon wrote:
 > > >
 > > > > >
 > > >
 > > > > > The hook is in bus->scan().
 > > >
 > > > > > I think we should launch a bus scan when there is a new 
device event.
 > > >
 > > > >
 > > >
 > > > > That's what I'm trying to say. We finally need to execute a 
handler as
 > > >
 > > > > of a device event to finish the job.
 > > >
 > > > Please be more specific, I am not sure to understand.
 > > >
 > >
 > > By the handler, I mean when we monitor the udev by 
select/poll/epoll and
 > > device uevents come, the application will execute a handler (or just a
 > > function) for each of such uevent. Then why not adding the vdev there?
 >
 > Because it must work for hotplug, and initial scan too.
 > We can also think to application requiring a manual scan.
 > The bus scan is the right place to have every scans called. That's 
simple.

Yes, the logic is simpler; anything changes, appication just invokes a 
scan.Then we will have a somewhat complex hook that: (1) scan all 
devices and identify added/removed devices; (2) and add vdev for added 
device; (3) remove vdev for removed device.

Compared to that, we can separate the implementation for hotplug and 
initial scan:
   * For initial scan, iterate all devices to add vdev for each of them.
   * For hotplug, based on which device is added/removed, we just 
add/remove the corresponding vdev for that device.

Anyway, since this patch does provide a new way; and will not affect the 
other way. So I think it's OK to add this.

Thank you for your clarification.

Thanks,
Jianfeng
  

On 12/1/2017 8:36 AM, Thomas Monjalon wrote:
> The scan callback allows to spawn a vdev automatically
> given some custom scan rules.
> It is especially useful to create a TAP device automatically
> connected to a netdevice as remote.
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
> warning: to be tested
> ---
>   drivers/bus/vdev/rte_bus_vdev.h | 29 ++++++++++++++++
>   drivers/bus/vdev/vdev.c         | 75 +++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 104 insertions(+)
>
> diff --git a/drivers/bus/vdev/rte_bus_vdev.h b/drivers/bus/vdev/rte_bus_vdev.h
> index 41762b853..a90382f49 100644
> --- a/drivers/bus/vdev/rte_bus_vdev.h
> +++ b/drivers/bus/vdev/rte_bus_vdev.h
> @@ -124,6 +124,35 @@ RTE_PMD_EXPORT_NAME(nm, __COUNTER__)
>   #define RTE_PMD_REGISTER_ALIAS(nm, alias)\
>   static const char *vdrvinit_ ## nm ## _alias = RTE_STR(alias)
>   
> +typedef void (*rte_vdev_scan_callback)(void *user_arg);
> +
> +/**
> + * Add a callback to be called on vdev scan
> + * before reading the devargs list.
> + *
> + * @param callback
> + *   The function to be called which can update the devargs list.
> + * @param user_arg
> + *   An opaque pointer passed to callback.
> + * @return
> + *   0 on success, -ENOMEM otherwise
> + */
> +int
> +rte_vdev_add_custom_scan(rte_vdev_scan_callback callback, void *user_arg);
> +
> +/**
> + * Remove a registered scan callback.
> + *
> + * @param callback
> + *   The registered function to be removed.
> + * @param user_arg
> + *   The associated opaque pointer or (void*)-1 for any.
> + * @return
> + *   0 on success
> + */
> +int
> +rte_vdev_remove_custom_scan(rte_vdev_scan_callback callback, void *user_arg);
> +
>   /**
>    * Initialize a driver specified by name.
>    *
> diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
> index fd7736d63..902f1e128 100644
> --- a/drivers/bus/vdev/vdev.c
> +++ b/drivers/bus/vdev/vdev.c
> @@ -44,6 +44,8 @@
>   #include <rte_common.h>
>   #include <rte_devargs.h>
>   #include <rte_memory.h>
> +#include <rte_tailq.h>
> +#include <rte_spinlock.h>
>   #include <rte_errno.h>
>   
>   #include "rte_bus_vdev.h"
> @@ -62,6 +64,16 @@ static struct vdev_device_list vdev_device_list =
>   struct vdev_driver_list vdev_driver_list =
>   	TAILQ_HEAD_INITIALIZER(vdev_driver_list);
>   
> +struct vdev_custom_scan {
> +	TAILQ_ENTRY(vdev_custom_scan) next;
> +	rte_vdev_scan_callback callback;
> +	void *user_arg;
> +};
> +TAILQ_HEAD(vdev_custom_scans, vdev_custom_scan);
> +static struct vdev_custom_scans vdev_custom_scans =
> +	TAILQ_HEAD_INITIALIZER(vdev_custom_scans);
> +static rte_spinlock_t vdev_custom_scan_lock = RTE_SPINLOCK_INITIALIZER;
> +
>   /* register a driver */
>   void
>   rte_vdev_register(struct rte_vdev_driver *driver)
> @@ -76,6 +88,53 @@ rte_vdev_unregister(struct rte_vdev_driver *driver)
>   	TAILQ_REMOVE(&vdev_driver_list, driver, next);
>   }
>   
> +int
> +rte_vdev_add_custom_scan(rte_vdev_scan_callback callback, void *user_arg)
> +{
> +	struct vdev_custom_scan *custom_scan;
> +
> +	rte_spinlock_lock(&vdev_custom_scan_lock);
> +
> +	/* check if already registered */
> +	TAILQ_FOREACH(custom_scan, &vdev_custom_scans, next) {
> +		if (custom_scan->callback == callback &&
> +				custom_scan->user_arg == user_arg)
> +			break;
> +	}
> +
> +	if (custom_scan == NULL) {
> +		custom_scan = malloc(sizeof(struct vdev_custom_scan));
> +		if (custom_scan != NULL) {
> +			custom_scan->callback = callback;
> +			custom_scan->user_arg = user_arg;
> +			TAILQ_INSERT_TAIL(&vdev_custom_scans, custom_scan, next);
> +		}
> +	}
> +
> +	rte_spinlock_unlock(&vdev_custom_scan_lock);
> +
> +	return (custom_scan == NULL) ? -ENOMEM : 0;
> +}
> +
> +int
> +rte_vdev_remove_custom_scan(rte_vdev_scan_callback callback, void *user_arg)
> +{
> +	struct vdev_custom_scan *custom_scan, *tmp_scan;
> +
> +	rte_spinlock_lock(&vdev_custom_scan_lock);
> +	TAILQ_FOREACH_SAFE(custom_scan, &vdev_custom_scans, next, tmp_scan) {
> +		if (custom_scan->callback != callback ||
> +				(custom_scan->user_arg != (void *)-1 &&
> +				custom_scan->user_arg != user_arg))
> +			continue;
> +		TAILQ_REMOVE(&vdev_custom_scans, custom_scan, next);
> +		free(custom_scan);
> +	}
> +	rte_spinlock_unlock(&vdev_custom_scan_lock);
> +
> +	return 0;
> +}
> +
>   static int
>   vdev_parse(const char *name, void *addr)
>   {
> @@ -260,6 +319,22 @@ vdev_scan(void)
>   {
>   	struct rte_vdev_device *dev;
>   	struct rte_devargs *devargs;
> +	struct vdev_custom_scan *custom_scan;
> +
> +	/* call custom scan callbacks if any */
> +	rte_spinlock_lock(&vdev_custom_scan_lock);
> +	TAILQ_FOREACH(custom_scan, &vdev_custom_scans, next) {
> +		if (custom_scan->callback != NULL)
> +			/*
> +			 * the callback should update devargs list
> +			 * by calling rte_eal_devargs_insert() with
> +			 *     devargs.bus = rte_bus_find_by_name("vdev");
> +			 *     devargs.type = RTE_DEVTYPE_VIRTUAL;
> +			 *     devargs.policy = RTE_DEV_WHITELISTED;
> +			 */
> +			custom_scan->callback(custom_scan->user_arg);
> +	}
> +	rte_spinlock_unlock(&vdev_custom_scan_lock);

You mentioned that we want to call the hook in the scan(), but it's now 
in parse()?

Thanks,
Jianfeng

>   
>   	/* for virtual devices we scan the devargs_list populated via cmdline */
>   	TAILQ_FOREACH(devargs, &devargs_list, next) {
  

06/12/2017 03:52, Tan, Jianfeng:
> On 12/1/2017 8:36 AM, Thomas Monjalon wrote:
> > +int
> > +rte_vdev_add_custom_scan(rte_vdev_scan_callback callback, void *user_arg)
> > +{
> > +	struct vdev_custom_scan *custom_scan;
> > +
> > +	rte_spinlock_lock(&vdev_custom_scan_lock);
> > +
> > +	/* check if already registered */
> > +	TAILQ_FOREACH(custom_scan, &vdev_custom_scans, next) {
> > +		if (custom_scan->callback == callback &&
> > +				custom_scan->user_arg == user_arg)
> > +			break;
> > +	}
> > +
> > +	if (custom_scan == NULL) {
> > +		custom_scan = malloc(sizeof(struct vdev_custom_scan));
> > +		if (custom_scan != NULL) {
> > +			custom_scan->callback = callback;
> > +			custom_scan->user_arg = user_arg;
> > +			TAILQ_INSERT_TAIL(&vdev_custom_scans, custom_scan, next);
> > +		}
> > +	}
> > +
> > +	rte_spinlock_unlock(&vdev_custom_scan_lock);
> > +
> > +	return (custom_scan == NULL) ? -ENOMEM : 0;
> > +}
> > +
> > +int
> > +rte_vdev_remove_custom_scan(rte_vdev_scan_callback callback, void *user_arg)
> > +{
> > +	struct vdev_custom_scan *custom_scan, *tmp_scan;
> > +
> > +	rte_spinlock_lock(&vdev_custom_scan_lock);
> > +	TAILQ_FOREACH_SAFE(custom_scan, &vdev_custom_scans, next, tmp_scan) {
> > +		if (custom_scan->callback != callback ||
> > +				(custom_scan->user_arg != (void *)-1 &&
> > +				custom_scan->user_arg != user_arg))
> > +			continue;
> > +		TAILQ_REMOVE(&vdev_custom_scans, custom_scan, next);
> > +		free(custom_scan);
> > +	}
> > +	rte_spinlock_unlock(&vdev_custom_scan_lock);
> > +
> > +	return 0;
> > +}
> > +
> >   static int
> >   vdev_parse(const char *name, void *addr)
> >   {
> > @@ -260,6 +319,22 @@ vdev_scan(void)
> >   {
> >   	struct rte_vdev_device *dev;
> >   	struct rte_devargs *devargs;
> > +	struct vdev_custom_scan *custom_scan;
> > +
> > +	/* call custom scan callbacks if any */
> > +	rte_spinlock_lock(&vdev_custom_scan_lock);
> > +	TAILQ_FOREACH(custom_scan, &vdev_custom_scans, next) {
> > +		if (custom_scan->callback != NULL)
> > +			/*
> > +			 * the callback should update devargs list
> > +			 * by calling rte_eal_devargs_insert() with
> > +			 *     devargs.bus = rte_bus_find_by_name("vdev");
> > +			 *     devargs.type = RTE_DEVTYPE_VIRTUAL;
> > +			 *     devargs.policy = RTE_DEV_WHITELISTED;
> > +			 */
> > +			custom_scan->callback(custom_scan->user_arg);
> > +	}
> > +	rte_spinlock_unlock(&vdev_custom_scan_lock);
> 
> You mentioned that we want to call the hook in the scan(), but it's now 
> in parse()?

No, it is in vdev_scan.
Look carefully the patch, especially after @@.
  

> No, it is in vdev_scan.
> Look carefully the patch, especially after @@.

I overlooked. Sorry about that.
I don't have further comments. Please send v2 according to your own comments and document update.

Thanks,
Jianfeng