[dpdk-dev,V15,2/5] eal: add uevent pass and process function
Checks
Commit Message
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.
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
Signed-off-by: Jeff Guo <jia.guo@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 ++++++
lib/librte_eal/linuxapp/eal/eal_dev.c | 95 ++++++++++++++++++++++++++++++++-
2 files changed, 111 insertions(+), 1 deletion(-)
Comments
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@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
jianfeng, thanks for your review. almost make sense and comment as bellow.
On 3/21/2018 10:20 PM, Tan, Jianfeng wrote:
>
>
> 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
>
>
sorry, that what i mean should be uevent message parse but not pass, and
what you say make sense.
>>
>> 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.
>
yes, not dummy just internal function.
>>
>> Signed-off-by: Jeff Guo <jia.guo@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.
>
will check and split the definition more explicit.
>> 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.
>
agree.
>> +
>> +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.
>
yes.
>> +
>> +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.
>
ok.
>> + RTE_DEV_EVENT_SUBSYSTEM_UIO,
>> + RTE_DEV_EVENT_SUBSYSTEM_VFIO,
>
> If we don't support VFIO now, I prefer not defining it now.
>
will remove it at this stage and add later.
>> + 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.
>
the buf do not contain multiple event but will involve more string split
by several "/0" , so need check that by bellow code.
>> + 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"?
>
the reason is show as above.
>> + /**
>> + * 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.
>
the reason is show as above.
>> + }
>> +
>> + 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().
>
might be better from what you said.
>> +
>> + 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?
>
could check here and i will add that check only for uio now.
>> +
>> + if (uevent.devname)
>> + _rte_dev_callback_process(uevent.devname, uevent.type, NULL);
>> }
>> int __rte_experimental
>
@@ -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
+
+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 */
+};
+
+enum rte_dev_event_subsystem {
+ RTE_DEV_EVENT_SUBSYSTEM_UNKNOWN,
+ RTE_DEV_EVENT_SUBSYSTEM_UIO,
+ RTE_DEV_EVENT_SUBSYSTEM_VFIO,
+ RTE_DEV_EVENT_SUBSYSTEM_MAX
+};
+
/**
* The device event type.
*/
@@ -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);
+
+ while (i < RTE_EAL_UEV_MSG_LEN) {
+ for (; i < RTE_EAL_UEV_MSG_LEN; i++) {
+ if (*buf)
+ break;
+ buf++;
+ }
+ /**
+ * check device uevent from kernel side, no need to check
+ * uevent from udev.
+ */
+ if (!strncmp(buf, "libudev", 7)) {
+ 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++;
+ }
+ }
+
+ 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;
+
+ 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;
+
+ if (uevent.devname)
+ _rte_dev_callback_process(uevent.devname, uevent.type, NULL);
}
int __rte_experimental