From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by dpdk.org (Postfix) with ESMTP id 465F614E8 for ; Wed, 28 Mar 2018 18:15:43 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 28 Mar 2018 09:15:43 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,372,1517904000"; d="scan'208";a="215671528" Received: from tanjianf-mobl.ccr.corp.intel.com (HELO [10.255.29.250]) ([10.255.29.250]) by fmsmga005.fm.intel.com with ESMTP; 28 Mar 2018 09:15:40 -0700 To: Jeff Guo , 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: <1521610066-12966-3-git-send-email-jia.guo@intel.com> <1522063256-3997-1-git-send-email-jia.guo@intel.com> <1522063256-3997-4-git-send-email-jia.guo@intel.com> Cc: jblunck@infradead.org, shreyansh.jain@nxp.com, dev@dpdk.org, helin.zhang@intel.com From: "Tan, Jianfeng" Message-ID: Date: Thu, 29 Mar 2018 00:15:40 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <1522063256-3997-4-git-send-email-jia.guo@intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH V16 3/4] eal/linux: uevent parse and process 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: Wed, 28 Mar 2018 16:15:44 -0000 BTW, adding new .c file needs to update meson.build now. On 3/26/2018 7:20 PM, Jeff Guo wrote: > In order to handle the uevent which have been detected from the kernel > side, add uevent parse and process function to translate the uevent into > device event, which user has subscribe to monitor. > > Signed-off-by: Jeff Guo > --- > 1.move all linux specific together > --- > lib/librte_eal/linuxapp/eal/eal_dev.c | 214 +++++++++++++++++++++++++++++++++- > 1 file changed, 211 insertions(+), 3 deletions(-) > > diff --git a/lib/librte_eal/linuxapp/eal/eal_dev.c b/lib/librte_eal/linuxapp/eal/eal_dev.c > index 5ab5830..90094c0 100644 > --- a/lib/librte_eal/linuxapp/eal/eal_dev.c > +++ b/lib/librte_eal/linuxapp/eal/eal_dev.c > @@ -2,19 +2,227 @@ > * Copyright(c) 2018 Intel Corporation > */ > > -#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include Some header files are not necessary, the above one for example. > +#include > +#include > + > +#include > +#include > #include > +#include We don't need this one neither. > +#include > +#include > +#include > + > +#include "eal_private.h" > +#include "eal_thread.h" Ditto. > + > +static struct rte_intr_handle intr_handle = {.fd = -1 }; I don't think we need a static intr_handle, what we need is the monitor fd. > +static bool monitor_not_started = true; > + > +#define EAL_UEV_MSG_LEN 4096 > +#define EAL_UEV_MSG_ELEM_LEN 128 > + > +/* identify the system layer which event exposure from */ > +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_MAX > +}; > + > +static int > +dev_uev_monitor_fd_new(void) > +{ > + int uevent_fd; > + > + uevent_fd = socket(PF_NETLINK, SOCK_RAW | SOCK_CLOEXEC | > + SOCK_NONBLOCK, > + NETLINK_KOBJECT_UEVENT); > + if (uevent_fd < 0) { > + RTE_LOG(ERR, EAL, "create uevent fd failed\n"); > + return -1; > + } > + return uevent_fd; > +} > + > +static int > +dev_uev_monitor_create(int netlink_fd) I think we should merge this function with above function. I don't see a reason to split into two functions. > +{ > + struct sockaddr_nl addr; > + int ret; > + int size = 64 * 1024; > + int nonblock = 1; > + > + memset(&addr, 0, sizeof(addr)); > + addr.nl_family = AF_NETLINK; > + addr.nl_pid = 0; > + addr.nl_groups = 0xffffffff; > + > + if (bind(netlink_fd, (struct sockaddr *) &addr, sizeof(addr)) < 0) { > + RTE_LOG(ERR, EAL, "bind failed\n"); Please print more information here, so that we don't have to check the code if we really encounter such an error. > + goto err; > + } > + > + setsockopt(netlink_fd, SOL_SOCKET, SO_PASSCRED, &size, sizeof(size)); > + > + ret = ioctl(netlink_fd, FIONBIO, &nonblock); > + if (ret != 0) { > + RTE_LOG(ERR, EAL, "ioctl(FIONBIO) failed\n"); > + goto err; > + } > + return 0; > +err: > + close(netlink_fd); > + return -1; > +} > + > +static void > +dev_uev_parse(const char *buf, struct rte_dev_event *event, int length) We always get an event we care? If no, we need return something so that the caller can skip this event. > +{ > + char action[EAL_UEV_MSG_ELEM_LEN]; > + char subsystem[EAL_UEV_MSG_ELEM_LEN]; > + char dev_path[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(dev_path, 0, EAL_UEV_MSG_ELEM_LEN); > + memset(pci_slot_name, 0, EAL_UEV_MSG_ELEM_LEN); I did not see you need dev_path, why do we care to parse it? > + > + 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, "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; You are assigning a stack pointer for the caller to use; this is dangerous, we should never do that. > + } > + 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; > + 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[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)); The above three lines are in bad format. > + 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)) > + return; > + > + if (uevent.devname) > + dev_callback_process(uevent.devname, uevent.type, NULL); > +} > > int __rte_experimental > rte_dev_event_monitor_start(void) > { > - /* TODO: start uevent monitor for linux */ > + int ret; > + > + if (!monitor_not_started) > + return 0; > + > + intr_handle.fd = dev_uev_monitor_fd_new(); > + intr_handle.type = RTE_INTR_HANDLE_DEV_EVENT; > + > + ret = dev_uev_monitor_create(intr_handle.fd); > + > + if (ret) { > + RTE_LOG(ERR, EAL, "error create device event monitor\n"); > + return -1; > + } > + > + 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_not_started = false; > + > return 0; > } > > int __rte_experimental > rte_dev_event_monitor_stop(void) > { > - /* TODO: stop uevent monitor for linux */ > + int ret; > + > + if (monitor_not_started) > + return 0; > + > + ret = rte_intr_callback_unregister(&intr_handle, dev_uev_process, NULL); > + if (ret) { > + RTE_LOG(ERR, EAL, "fail to unregister uevent callback"); > + return ret; > + } > + > + close(intr_handle.fd); > + intr_handle.fd = -1; > + monitor_not_started = true; > return 0; > }