[dpdk-dev] [PATCH V15 2/5] eal: add uevent pass and process function

[Tan, Jianfeng]

On 3/21/2018 1:27 PM, Jeff Guo wrote:
> In order to handle the uevent which have been detected from the kernel
> side, add uevent process function, let hot plug event to be example to
> show uevent mechanism how to pass the uevent and process the uevent.

In fact, how to pass the uevent to eal/linux for processing, is already 
done by last patch, by registering a callback into interrupt thread.

In this patch, we are actually showing how to process the uevent, and 
translate it into RTE_DEV_EVENT_ADD, RTE_DEV_EVENT_DEL, etc.

So the title would be something like:

eal/linux: translate uevent to dev event


>
> About uevent passing and processing, add below functions in linux eal
> dev layer. FreeBSD not support uevent ,so let it to be void and do not
> implement in function.
> a.dev_uev_parse
> b.dev_uev_receive
> c.dev_uev_process

We already have dummy rte_dev_event_monitor_start and 
rte_dev_event_monitor_stop, we don't need to have those dummy internal 
functions any more. Actually, you did not implement those dummy functions.

>
> Signed-off-by: Jeff Guo <jia.guo at intel.com>
> ---
> v15->v14:
> remove the uevent type check and any policy from eal,
> let it check and management in user's callback.
> ---
>   lib/librte_eal/common/include/rte_dev.h | 17 ++++++

And if you agree with me in the above, we shall not touch this file. 
Move the definition into the previous patch.

>   lib/librte_eal/linuxapp/eal/eal_dev.c   | 95 ++++++++++++++++++++++++++++++++-
>   2 files changed, 111 insertions(+), 1 deletion(-)
>
> diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
> index d2fcbc9..98ea12b 100644
> --- a/lib/librte_eal/common/include/rte_dev.h
> +++ b/lib/librte_eal/common/include/rte_dev.h
> @@ -24,6 +24,23 @@ extern "C" {
>   #include <rte_compat.h>
>   #include <rte_log.h>
>   
> +#define RTE_EAL_UEV_MSG_LEN 4096
> +#define RTE_EAL_UEV_MSG_ELEM_LEN 128

Such macro shall be linux uevent specific, so put them in linuxapp folder.

> +
> +enum rte_dev_state {
> +	RTE_DEV_UNDEFINED,	/**< unknown device state */
> +	RTE_DEV_FAULT,	/**< device fault or error */
> +	RTE_DEV_PARSED,	/**< device has been scanned on bus*/
> +	RTE_DEV_PROBED,	/**< device has been probed driver  */
> +};

This enum is not used in this patch series, I do see it's used in the 
other series. So put the definition there.

> +
> +enum rte_dev_event_subsystem {
> +	RTE_DEV_EVENT_SUBSYSTEM_UNKNOWN,

I don't see where we use this macro. Seems that we now only implement 
UIO, so I suppose, we shall set the other cases to this UNKNOWN.

> +	RTE_DEV_EVENT_SUBSYSTEM_UIO,
> +	RTE_DEV_EVENT_SUBSYSTEM_VFIO,

If we don't support VFIO now, I prefer not defining it now.

> +	RTE_DEV_EVENT_SUBSYSTEM_MAX
> +};

> +
>   /**
>    * The device event type.
>    */
> diff --git a/lib/librte_eal/linuxapp/eal/eal_dev.c b/lib/librte_eal/linuxapp/eal/eal_dev.c
> index 9d9e088..2b34e2c 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_dev.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_dev.c
> @@ -78,9 +78,102 @@ dev_uev_monitor_create(int netlink_fd)
>   }
>   
>   static void
> +dev_uev_parse(const char *buf, struct rte_dev_event *event)
> +{
> +	char action[RTE_EAL_UEV_MSG_ELEM_LEN];
> +	char subsystem[RTE_EAL_UEV_MSG_ELEM_LEN];
> +	char dev_path[RTE_EAL_UEV_MSG_ELEM_LEN];
> +	char pci_slot_name[RTE_EAL_UEV_MSG_ELEM_LEN];
> +	int i = 0;
> +
> +	memset(action, 0, RTE_EAL_UEV_MSG_ELEM_LEN);
> +	memset(subsystem, 0, RTE_EAL_UEV_MSG_ELEM_LEN);
> +	memset(dev_path, 0, RTE_EAL_UEV_MSG_ELEM_LEN);
> +	memset(pci_slot_name, 0, RTE_EAL_UEV_MSG_ELEM_LEN);
> +

Maybe we can put an example here for better understanding.

And if this buf can contain multiple events? If yes, the implementation 
is not correct, we will only record one event; if no, we can simplify it 
a little bit.

> +	while (i < RTE_EAL_UEV_MSG_LEN) {
> +		for (; i < RTE_EAL_UEV_MSG_LEN; i++) {
> +			if (*buf)
> +				break;
> +			buf++;
> +		}

If we pass in the length of the buf, we don't have to skip "\0"?

> +		/**
> +		 * check device uevent from kernel side, no need to check
> +		 * uevent from udev.
> +		 */
> +		if (!strncmp(buf, "libudev", 7)) {

Use strcmp is enough. And we actually need to check left length enough 
for strlen("libudev").

> +			buf += 7;
> +			i += 7;
> +			return;
> +		}
> +		if (!strncmp(buf, "ACTION=", 7)) {
> +			buf += 7;
> +			i += 7;
> +			snprintf(action, sizeof(action), "%s", buf);
> +		} else if (!strncmp(buf, "DEVPATH=", 8)) {
> +			buf += 8;
> +			i += 8;
> +			snprintf(dev_path, sizeof(dev_path), "%s", buf);
> +		} else if (!strncmp(buf, "SUBSYSTEM=", 10)) {
> +			buf += 10;
> +			i += 10;
> +			snprintf(subsystem, sizeof(subsystem), "%s", buf);
> +		} else if (!strncmp(buf, "PCI_SLOT_NAME=", 14)) {
> +			buf += 14;
> +			i += 14;
> +			snprintf(pci_slot_name, sizeof(subsystem), "%s", buf);
> +			event->devname = pci_slot_name;
> +		}
> +		for (; i < RTE_EAL_UEV_MSG_LEN; i++) {
> +			if (*buf == '\0')
> +				break;
> +			buf++;
> +		}

As we already check '\0' in the begin of the loop, we don't need it at 
the end any more.

> +	}
> +
> +	if ((!strncmp(subsystem, "uio", 3)) ||
> +		(!strncmp(subsystem, "pci", 3)))
> +		event->subsystem = RTE_DEV_EVENT_SUBSYSTEM_UIO;
> +	if (!strncmp(action, "add", 3))
> +		event->type = RTE_DEV_EVENT_ADD;
> +	if (!strncmp(action, "remove", 6))
> +		event->type = RTE_DEV_EVENT_REMOVE;
> +}
> +
> +static int
> +dev_uev_receive(int fd, struct rte_dev_event *uevent)
> +{
> +	int ret;
> +	char buf[RTE_EAL_UEV_MSG_LEN];
> +
> +	memset(uevent, 0, sizeof(struct rte_dev_event));
> +	memset(buf, 0, RTE_EAL_UEV_MSG_LEN);
> +
> +	ret = recv(fd, buf, RTE_EAL_UEV_MSG_LEN - 1, MSG_DONTWAIT);
> +	if (ret < 0) {
> +		RTE_LOG(ERR, EAL,
> +		"Socket read error(%d): %s\n",
> +		errno, strerror(errno));
> +		return -1;
> +	} else if (ret == 0)
> +		/* connection closed */
> +		return -1;

So we are sure how many bytes shall be parsed, we can pass the length 
into dev_uev_parse().

> +
> +	dev_uev_parse(buf, uevent);
> +
> +	return 0;
> +}
> +
> +static void
>   dev_uev_process(__rte_unused void *param)
>   {
> -	/* TODO: device uevent processing */
> +	struct rte_dev_event uevent;
> +
> +	if (dev_uev_receive(intr_handle.fd, &uevent))
> +		return;

We don't use uevent->subsystem below, why we have to define it in first 
place?

> +
> +	if (uevent.devname)
> +		_rte_dev_callback_process(uevent.devname, uevent.type, NULL);
>   }
>   
>   int __rte_experimental