From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id F03D02BF1 for ; Thu, 22 Mar 2018 09:20:54 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 22 Mar 2018 01:20:52 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,343,1517904000"; d="scan'208";a="30251290" Received: from jguo15x-mobl3.ccr.corp.intel.com (HELO [10.67.68.72]) ([10.67.68.72]) by fmsmga002.fm.intel.com with ESMTP; 22 Mar 2018 01:20:49 -0700 To: "Tan, Jianfeng" , stephen@networkplumber.org, bruce.richardson@intel.com, ferruh.yigit@intel.com, konstantin.ananyev@intel.com, gaetan.rivet@6wind.com, jingjing.wu@intel.com, thomas@monjalon.net, motih@mellanox.com, harry.van.haaren@intel.com References: <1517314860-8097-3-git-send-email-jia.guo@intel.com> <1521610066-12966-1-git-send-email-jia.guo@intel.com> <1521610066-12966-2-git-send-email-jia.guo@intel.com> <6065d4b8-8069-e0ea-b948-cc1e794c5020@intel.com> Cc: jblunck@infradead.org, shreyansh.jain@nxp.com, dev@dpdk.org, helin.zhang@intel.com From: "Guo, Jia" Message-ID: Date: Thu, 22 Mar 2018 16:20:49 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <6065d4b8-8069-e0ea-b948-cc1e794c5020@intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH V15 2/5] eal: add uevent pass and process function X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 22 Mar 2018 08:20:57 -0000 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 >> --- >> 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 >> #include >> +#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 >