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....@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
