[dpdk-dev,V18,3/4] eal/linux: uevent parse and process

  In order to handle the uevent which has been detected from the kernel
side, add uevent parse and process function to translate the uevent into
device event, which user has subscribed to monitor.

Signed-off-by: Jeff Guo <jia.guo@intel.com>
---
v18->v17:
refine socket configuration.
---
 lib/librte_eal/linuxapp/eal/eal_dev.c | 178 +++++++++++++++++++++++++++++++++-
 1 file changed, 176 insertions(+), 2 deletions(-)
  

Hi Jeff,

Looks much better now, but still have some issues to address.

> -----Original Message-----
> From: Guo, Jia
> Sent: Tuesday, April 3, 2018 6:34 PM
> To: stephen@networkplumber.org; Richardson, Bruce; Yigit, Ferruh;
> Ananyev, Konstantin; gaetan.rivet@6wind.com; Wu, Jingjing;
> thomas@monjalon.net; motih@mellanox.com; Van Haaren, Harry; Tan,
> Jianfeng
> Cc: jblunck@infradead.org; shreyansh.jain@nxp.com; dev@dpdk.org; Guo,
> Jia; Zhang, Helin
> Subject: [PATCH V18 3/4] eal/linux: uevent parse and process
> 
> In order to handle the uevent which has been detected from the kernel
> side, add uevent parse and process function to translate the uevent into
> device event, which user has subscribed to monitor.
> 
> Signed-off-by: Jeff Guo <jia.guo@intel.com>
> ---
> v18->v17:
> refine socket configuration.
> ---
>  lib/librte_eal/linuxapp/eal/eal_dev.c | 178
> +++++++++++++++++++++++++++++++++-
>  1 file changed, 176 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_eal/linuxapp/eal/eal_dev.c
> b/lib/librte_eal/linuxapp/eal/eal_dev.c
> index 9c8d1a0..9f2ee40 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_dev.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_dev.c
> @@ -2,21 +2,195 @@
>   * Copyright(c) 2018 Intel Corporation
>   */
> 
> +#include <string.h>
> +#include <unistd.h>
> +#include <sys/socket.h>
> +#include <linux/netlink.h>
> +
>  #include <rte_log.h>
>  #include <rte_compat.h>
>  #include <rte_dev.h>
> +#include <rte_malloc.h>
> +#include <rte_interrupts.h>
> +
> +#include "eal_private.h"
> +
> +static struct rte_intr_handle intr_handle = {.fd = -1 };
> +static bool monitor_started;
> +
> +#define EAL_UEV_MSG_LEN 4096
> +#define EAL_UEV_MSG_ELEM_LEN 128
> +
> +/* identify the system layer which event exposure from */

Reword it a little bit: 
    /* identify the system layer which reports this event */

> +enum eal_dev_event_subsystem {
> +	EAL_DEV_EVENT_SUBSYSTEM_PCI, /* PCI bus device event */
> +	EAL_DEV_EVENT_SUBSYSTEM_UIO, /* UIO driver device event */
> +	EAL_DEV_EVENT_SUBSYSTEM_VFIO, /* VFIO driver device event */
> +	EAL_DEV_EVENT_SUBSYSTEM_MAX
> +};
> +
> +static int
> +dev_uev_socket_fd_create(void)
> +{
> +	struct sockaddr_nl addr;
> +	int ret;
> +
> +	intr_handle.fd = socket(PF_NETLINK, SOCK_RAW | SOCK_CLOEXEC |
> +			SOCK_NONBLOCK,
> +			NETLINK_KOBJECT_UEVENT);
> +	if (intr_handle.fd < 0) {
> +		RTE_LOG(ERR, EAL, "create uevent fd failed.\n");
> +		return -1;
> +	}
> +
> +	memset(&addr, 0, sizeof(addr));
> +	addr.nl_family = AF_NETLINK;
> +	addr.nl_pid = 0;
> +	addr.nl_groups = 0xffffffff;
> +
> +	ret = bind(intr_handle.fd, (struct sockaddr *) &addr, sizeof(addr));
> +	if (ret < 0) {
> +		RTE_LOG(ERR, EAL, "Failed to bind socket for netlink fd.\n");

Reword it a little bit so that we can understand it's a log related to hotplug:
    Failed to bind uevent socket

> +		goto err;
> +	}
> +
> +	return 0;
> +err:
> +	close(intr_handle.fd);

Then: intr_handle.fd = -1?

> +	return ret;
> +}
> +
> +static void
> +dev_uev_parse(const char *buf, struct rte_dev_event *event, int length)
> +{
> +	char action[EAL_UEV_MSG_ELEM_LEN];
> +	char subsystem[EAL_UEV_MSG_ELEM_LEN];
> +	char pci_slot_name[EAL_UEV_MSG_ELEM_LEN];
> +	int i = 0;
> +
> +	memset(action, 0, EAL_UEV_MSG_ELEM_LEN);
> +	memset(subsystem, 0, EAL_UEV_MSG_ELEM_LEN);
> +	memset(pci_slot_name, 0, EAL_UEV_MSG_ELEM_LEN);
> +
> +	while (i < length) {
> +		for (; i < length; i++) {
> +			if (*buf)
> +				break;
> +			buf++;
> +		}
> +		if (!strncmp(buf, "ACTION=", 7)) {
> +			buf += 7;
> +			i += 7;
> +			snprintf(action, sizeof(action), "%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 = strdup(pci_slot_name);
> +		}
> +		for (; i < length; i++) {
> +			if (*buf == '\0')
> +				break;
> +			buf++;
> +		}
> +	}
> +
> +	if (!strncmp(subsystem, "uio", 3))
> +		event->subsystem = EAL_DEV_EVENT_SUBSYSTEM_UIO;
> +	else if (!strncmp(subsystem, "pci", 3))
> +		event->subsystem = EAL_DEV_EVENT_SUBSYSTEM_PCI;
> +	else if (!strncmp(subsystem, "vfio", 4))

How can we indicate it is an event with subsystem that we will not handle?

> +		event->subsystem = EAL_DEV_EVENT_SUBSYSTEM_VFIO;
> +	if (!strncmp(action, "add", 3))
> +		event->type = RTE_DEV_EVENT_ADD;
> +	if (!strncmp(action, "remove", 6))
> +		event->type = RTE_DEV_EVENT_REMOVE;

How can we indicate it is an event with type that we will not handle?

My suggestion is to define a return value for that:
- EVENT_VALID returned for an event that we will handle later.
- EVENT_INVALID returned for any unknown events.

> +}
> +
> +static int
> +dev_uev_receive(int fd, struct rte_dev_event *uevent)
> +{
> +	int ret;
> +	char buf[EAL_UEV_MSG_LEN];
> +
> +	memset(uevent, 0, sizeof(struct rte_dev_event));
> +	memset(buf, 0, EAL_UEV_MSG_LEN);
> +
> +	ret = recv(fd, buf, EAL_UEV_MSG_LEN, 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;
> +
> +	dev_uev_parse(buf, uevent, EAL_UEV_MSG_LEN);
> +
> +	return 0;
> +}
> +
> +static void
> +dev_uev_process(__rte_unused void *param)
> +{
> +	struct rte_dev_event uevent;
> +
> +	if (dev_uev_receive(intr_handle.fd, &uevent))

If error happens, we shall start an alarm task to remove the callback of interrupt thread.

> +		return;

You may want to add a log here for debugging, showing what event comes for which device.

> 
> +	if (uevent.devname)

You can also filter this kind of events using the way I suggested above.

> +		dev_callback_process(uevent.devname, uevent.type);
> +}
> 
>  int __rte_experimental
>  rte_dev_event_monitor_start(void)
>  {
> -	/* TODO: start uevent monitor for linux */
> +	int ret;
> +
> +	if (monitor_started)
> +		return 0;
> +
> +	ret = dev_uev_socket_fd_create();
> +	if (ret) {
> +		RTE_LOG(ERR, EAL, "error create device event fd.\n");
> +		return -1;
> +	}
> +
> +	intr_handle.type = RTE_INTR_HANDLE_DEV_EVENT;
> +	ret = rte_intr_callback_register(&intr_handle, dev_uev_process,
> NULL);
> +
> +	if (ret) {
> +		RTE_LOG(ERR, EAL, "fail to register uevent callback.\n");
> +		return -1;
> +	}
> +
> +	monitor_started = true;
> +
>  	return 0;
>  }
> 
>  int __rte_experimental
>  rte_dev_event_monitor_stop(void)
>  {
> -	/* TODO: stop uevent monitor for linux */
> +	int ret;
> +
> +	if (!monitor_started)
> +		return 0;
> +
> +	ret = rte_intr_callback_unregister(&intr_handle, dev_uev_process,
> +					   (void *)-1);
> +	if (ret < 0) {
> +		RTE_LOG(ERR, EAL, "fail to unregister uevent callback.\n");
> +		return ret;
> +	}
> +
> +	close(intr_handle.fd);
> +	intr_handle.fd = -1;
> +	monitor_started = false;
>  	return 0;
>  }
> --
> 2.7.4
  

thanks for review.


On 4/4/2018 11:15 AM, Tan, Jianfeng wrote:
> Hi Jeff,
>
> Looks much better now, but still have some issues to address.
>
>> -----Original Message-----
>> From: Guo, Jia
>> Sent: Tuesday, April 3, 2018 6:34 PM
>> To: stephen@networkplumber.org; Richardson, Bruce; Yigit, Ferruh;
>> Ananyev, Konstantin; gaetan.rivet@6wind.com; Wu, Jingjing;
>> thomas@monjalon.net; motih@mellanox.com; Van Haaren, Harry; Tan,
>> Jianfeng
>> Cc: jblunck@infradead.org; shreyansh.jain@nxp.com; dev@dpdk.org; Guo,
>> Jia; Zhang, Helin
>> Subject: [PATCH V18 3/4] eal/linux: uevent parse and process
>>
>> In order to handle the uevent which has been detected from the kernel
>> side, add uevent parse and process function to translate the uevent into
>> device event, which user has subscribed to monitor.
>>
>> Signed-off-by: Jeff Guo <jia.guo@intel.com>
>> ---
>> v18->v17:
>> refine socket configuration.
>> ---
>>   lib/librte_eal/linuxapp/eal/eal_dev.c | 178
>> +++++++++++++++++++++++++++++++++-
>>   1 file changed, 176 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/librte_eal/linuxapp/eal/eal_dev.c
>> b/lib/librte_eal/linuxapp/eal/eal_dev.c
>> index 9c8d1a0..9f2ee40 100644
>> --- a/lib/librte_eal/linuxapp/eal/eal_dev.c
>> +++ b/lib/librte_eal/linuxapp/eal/eal_dev.c
>> @@ -2,21 +2,195 @@
>>    * Copyright(c) 2018 Intel Corporation
>>    */
>>
>> +#include <string.h>
>> +#include <unistd.h>
>> +#include <sys/socket.h>
>> +#include <linux/netlink.h>
>> +
>>   #include <rte_log.h>
>>   #include <rte_compat.h>
>>   #include <rte_dev.h>
>> +#include <rte_malloc.h>
>> +#include <rte_interrupts.h>
>> +
>> +#include "eal_private.h"
>> +
>> +static struct rte_intr_handle intr_handle = {.fd = -1 };
>> +static bool monitor_started;
>> +
>> +#define EAL_UEV_MSG_LEN 4096
>> +#define EAL_UEV_MSG_ELEM_LEN 128
>> +
>> +/* identify the system layer which event exposure from */
> Reword it a little bit:
>      /* identify the system layer which reports this event */
>
>> +enum eal_dev_event_subsystem {
>> +	EAL_DEV_EVENT_SUBSYSTEM_PCI, /* PCI bus device event */
>> +	EAL_DEV_EVENT_SUBSYSTEM_UIO, /* UIO driver device event */
>> +	EAL_DEV_EVENT_SUBSYSTEM_VFIO, /* VFIO driver device event */
>> +	EAL_DEV_EVENT_SUBSYSTEM_MAX
>> +};
>> +
>> +static int
>> +dev_uev_socket_fd_create(void)
>> +{
>> +	struct sockaddr_nl addr;
>> +	int ret;
>> +
>> +	intr_handle.fd = socket(PF_NETLINK, SOCK_RAW | SOCK_CLOEXEC |
>> +			SOCK_NONBLOCK,
>> +			NETLINK_KOBJECT_UEVENT);
>> +	if (intr_handle.fd < 0) {
>> +		RTE_LOG(ERR, EAL, "create uevent fd failed.\n");
>> +		return -1;
>> +	}
>> +
>> +	memset(&addr, 0, sizeof(addr));
>> +	addr.nl_family = AF_NETLINK;
>> +	addr.nl_pid = 0;
>> +	addr.nl_groups = 0xffffffff;
>> +
>> +	ret = bind(intr_handle.fd, (struct sockaddr *) &addr, sizeof(addr));
>> +	if (ret < 0) {
>> +		RTE_LOG(ERR, EAL, "Failed to bind socket for netlink fd.\n");
> Reword it a little bit so that we can understand it's a log related to hotplug:
>      Failed to bind uevent socket
>
>> +		goto err;
>> +	}
>> +
>> +	return 0;
>> +err:
>> +	close(intr_handle.fd);
> Then: intr_handle.fd = -1?
sure.
>> +	return ret;
>> +}
>> +
>> +static void
>> +dev_uev_parse(const char *buf, struct rte_dev_event *event, int length)
>> +{
>> +	char action[EAL_UEV_MSG_ELEM_LEN];
>> +	char subsystem[EAL_UEV_MSG_ELEM_LEN];
>> +	char pci_slot_name[EAL_UEV_MSG_ELEM_LEN];
>> +	int i = 0;
>> +
>> +	memset(action, 0, EAL_UEV_MSG_ELEM_LEN);
>> +	memset(subsystem, 0, EAL_UEV_MSG_ELEM_LEN);
>> +	memset(pci_slot_name, 0, EAL_UEV_MSG_ELEM_LEN);
>> +
>> +	while (i < length) {
>> +		for (; i < length; i++) {
>> +			if (*buf)
>> +				break;
>> +			buf++;
>> +		}
>> +		if (!strncmp(buf, "ACTION=", 7)) {
>> +			buf += 7;
>> +			i += 7;
>> +			snprintf(action, sizeof(action), "%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 = strdup(pci_slot_name);
>> +		}
>> +		for (; i < length; i++) {
>> +			if (*buf == '\0')
>> +				break;
>> +			buf++;
>> +		}
>> +	}
>> +
>> +	if (!strncmp(subsystem, "uio", 3))
>> +		event->subsystem = EAL_DEV_EVENT_SUBSYSTEM_UIO;
>> +	else if (!strncmp(subsystem, "pci", 3))
>> +		event->subsystem = EAL_DEV_EVENT_SUBSYSTEM_PCI;
>> +	else if (!strncmp(subsystem, "vfio", 4))
> How can we indicate it is an event with subsystem that we will not handle?
>
>> +		event->subsystem = EAL_DEV_EVENT_SUBSYSTEM_VFIO;
>> +	if (!strncmp(action, "add", 3))
>> +		event->type = RTE_DEV_EVENT_ADD;
>> +	if (!strncmp(action, "remove", 6))
>> +		event->type = RTE_DEV_EVENT_REMOVE;
> How can we indicate it is an event with type that we will not handle?
>
> My suggestion is to define a return value for that:
> - EVENT_VALID returned for an event that we will handle later.
> - EVENT_INVALID returned for any unknown events.
make sense ,just need 0 & -1 enough i think.
>> +}
>> +
>> +static int
>> +dev_uev_receive(int fd, struct rte_dev_event *uevent)
>> +{
>> +	int ret;
>> +	char buf[EAL_UEV_MSG_LEN];
>> +
>> +	memset(uevent, 0, sizeof(struct rte_dev_event));
>> +	memset(buf, 0, EAL_UEV_MSG_LEN);
>> +
>> +	ret = recv(fd, buf, EAL_UEV_MSG_LEN, 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;
>> +
>> +	dev_uev_parse(buf, uevent, EAL_UEV_MSG_LEN);
>> +
>> +	return 0;
>> +}
>> +
>> +static void
>> +dev_uev_process(__rte_unused void *param)
>> +{
>> +	struct rte_dev_event uevent;
>> +
>> +	if (dev_uev_receive(intr_handle.fd, &uevent))
> If error happens, we shall start an alarm task to remove the callback of interrupt thread.
>> +		return;
> You may want to add a log here for debugging, showing what event comes for which device.
>
>> +	if (uevent.devname)
> You can also filter this kind of events using the way I suggested above.
>
>> +		dev_callback_process(uevent.devname, uevent.type);
>> +}
>>
>>   int __rte_experimental
>>   rte_dev_event_monitor_start(void)
>>   {
>> -	/* TODO: start uevent monitor for linux */
>> +	int ret;
>> +
>> +	if (monitor_started)
>> +		return 0;
>> +
>> +	ret = dev_uev_socket_fd_create();
>> +	if (ret) {
>> +		RTE_LOG(ERR, EAL, "error create device event fd.\n");
>> +		return -1;
>> +	}
>> +
>> +	intr_handle.type = RTE_INTR_HANDLE_DEV_EVENT;
>> +	ret = rte_intr_callback_register(&intr_handle, dev_uev_process,
>> NULL);
>> +
>> +	if (ret) {
>> +		RTE_LOG(ERR, EAL, "fail to register uevent callback.\n");
>> +		return -1;
>> +	}
>> +
>> +	monitor_started = true;
>> +
>>   	return 0;
>>   }
>>
>>   int __rte_experimental
>>   rte_dev_event_monitor_stop(void)
>>   {
>> -	/* TODO: stop uevent monitor for linux */
>> +	int ret;
>> +
>> +	if (!monitor_started)
>> +		return 0;
>> +
>> +	ret = rte_intr_callback_unregister(&intr_handle, dev_uev_process,
>> +					   (void *)-1);
>> +	if (ret < 0) {
>> +		RTE_LOG(ERR, EAL, "fail to unregister uevent callback.\n");
>> +		return ret;
>> +	}
>> +
>> +	close(intr_handle.fd);
>> +	intr_handle.fd = -1;
>> +	monitor_started = false;
>>   	return 0;
>>   }
>> --
>> 2.7.4