From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wi0-f178.google.com (mail-wi0-f178.google.com [209.85.212.178]) by dpdk.org (Postfix) with ESMTP id 547E0593A for ; Thu, 22 May 2014 13:53:02 +0200 (CEST) Received: by mail-wi0-f178.google.com with SMTP id cc10so4164604wib.5 for ; Thu, 22 May 2014 04:53:11 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:organization :user-agent:in-reply-to:references:mime-version :content-transfer-encoding:content-type; bh=brVZ/krWOCg79v/UIz61aIBmLLhqFD8e8TMWQtR/y5Q=; b=aijFxG+r9sahbjlc0k7SFYaOVFUM95bIk+qFUaB7HM0yuz7xH8Ma1Y49ojLnlcyoIP VM9zYZqohcmBHuVRjozXzv2J+vQc0kJvE0aEMHL4wUlq6JeeMoEJVtHloo1Nw5H6ykDo ybKC9FEu8nONn1nResfYh/+a4Keync6F2M5omaVv9JquOY221s4zkxZWGmsVJsTnSQJB tGDwX7swuTWTDFGhHpm4fYY6WhUo6BwFAy+8XRtmk7IR2aSN4fP93HX4zN7ri8VMVysV beG1JVMMq/iLI0mfwfvcwDUQ1EcJ4wWA3qhTZyHTTecrX/WJbRUQO5UGSRi7RHosbSa6 EZyw== X-Gm-Message-State: ALoCoQln2oZff/9f4Ld4b7Fqp3Td1DA0r33Rrb9m4t+l3kb3WEsePHS01Ot//hjAvjPMprOsQcnZ X-Received: by 10.195.13.76 with SMTP id ew12mr1467137wjd.80.1400759591761; Thu, 22 May 2014 04:53:11 -0700 (PDT) Received: from xps13.localnet (6wind.net2.nerim.net. [213.41.180.237]) by mx.google.com with ESMTPSA id fu10sm7585087wib.11.2014.05.22.04.53.09 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 22 May 2014 04:53:11 -0700 (PDT) From: Thomas Monjalon To: Anatoly Burakov Date: Thu, 22 May 2014 13:53:06 +0200 Message-ID: <6426409.afQ7rpamsg@xps13> Organization: 6WIND User-Agent: KMail/4.13 (Linux/3.14.4-1-ARCH; KDE/4.13.0; x86_64; ; ) In-Reply-To: <1400514709-24087-9-git-send-email-anatoly.burakov@intel.com> References: <1400514709-24087-9-git-send-email-anatoly.burakov@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH v2 08/16] Add support for mapping devices through VFIO. X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 22 May 2014 11:53:02 -0000 Hi Anatoly, It seems to be the main patch, so I have many comments. 2014-05-19 16:51, Anatoly Burakov: > VFIO is kernel 3.6+ only, and so is only compiled when DPDK config > option CONFIG_RTE_EAL_VFIO is enabled, and kernel 3.6 or higher is > detected, thus preventing compile failures on older kernels if VFIO is > enabled in config (and it is, by default). > > Since VFIO cannot be used to map the same device twice, secondary > processes receive the device/group fd's by means of communicating over a > local socket. Only group and container fd's should be sent, as device > fd's can be obtained via ioctl() calls' on the group fd. > > For multiprocess, VFIO distinguishes between existing but unused groups > (e.g. grups that aren't bound to VFIO driver) and non-existing groups in > order to know if the secondary process requests a valid group, or if > secondary process requests something that doesn't exist. > > Signed-off-by: Anatoly Burakov How did you test this feature? Did you see some performance differences with igb_uio? > # workaround for a gcc bug with noreturn attribute > # http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12603 > ifeq ($(CONFIG_RTE_TOOLCHAIN_GCC),y) > CFLAGS_eal_thread.o += -Wno-return-type > -CFLAGS_eal_hpet.o += -Wno-return-type For history reason, it's better to explain in another patch that eal_hpet has been renamed eal_timer and there is no such need anymore in this file. > +++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c [...] > + * This code tries to determine if the PCI device is bound to VFIO driver, We should discuss a way to request igb_uio or VFIO binding of a device. > +++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio_socket.c This whole socket communication deserves a separated patch with protocol description. By the way, I'm not a big fan of the suffix "_socket" which can be misleading. But I have no other good naming idea. > +/* > + * socket listening thread for primary process > + */ > +__attribute__((noreturn)) void * > +pci_vfio_socket_thread(void *arg) So we have another thread to manage. I don't see where it is spawned? > --- a/lib/librte_eal/linuxapp/eal/include/eal_pci_init.h > +++ b/lib/librte_eal/linuxapp/eal/include/eal_pci_init.h [...] > +struct vfio_config vfio_cfg; > + > +pthread_t socket_thread; You are defining some variables in a .h file. I think it is a problem. Here are some other relevant errors from checkpatch.pl: ERROR: "foo * bar" should be "foo *bar" #197: FILE: lib/librte_eal/linuxapp/eal/eal_pci_vfio.c:64: +pci_vfio_get_msix_bar(int fd, int * msix_bar) ERROR: space required before the open brace '{' #216: FILE: lib/librte_eal/linuxapp/eal/eal_pci_vfio.c:83: + while (cap_offset){ ERROR: "foo * bar" should be "foo *bar" #301: FILE: lib/librte_eal/linuxapp/eal/eal_pci_vfio.c:168: + const struct rte_memseg * ms = rte_eal_get_physmem_layout(); ERROR: space required before the open parenthesis '(' #517: FILE: lib/librte_eal/linuxapp/eal/eal_pci_vfio.c:384: + switch(ret) { ERROR: "foo * bar" should be "foo *bar" #541: FILE: lib/librte_eal/linuxapp/eal/eal_pci_vfio.c:408: +pci_vfio_get_group_no(const char * pci_addr) ERROR: "foo * bar" should be "foo *bar" #545: FILE: lib/librte_eal/linuxapp/eal/eal_pci_vfio.c:412: + char * tok[16], *group_tok, *end; ERROR: else should follow close brace '}' #673: FILE: lib/librte_eal/linuxapp/eal/eal_pci_vfio.c:540: + } + else if (!(group_status.flags & VFIO_GROUP_FLAGS_VIABLE)) { WARNING: space prohibited between function name and open parenthesis '(' #751: FILE: lib/librte_eal/linuxapp/eal/eal_pci_vfio.c:618: + if ((vfio_res = rte_zmalloc("VFIO_RES", sizeof (*vfio_res), 0)) == NULL) { ERROR: "foo * bar" should be "foo *bar" #784: FILE: lib/librte_eal/linuxapp/eal/eal_pci_vfio.c:651: + void * bar_addr; ERROR: return is not a function, parentheses are not required #850: FILE: lib/librte_eal/linuxapp/eal/eal_pci_vfio.c:717: + return (0); ERROR: space required before the open parenthesis '(' #933: FILE: lib/librte_eal/linuxapp/eal/eal_pci_vfio_socket.c:75: + } while(0) WARNING: Single statement macros should not use a do {} while (0) loop #934: FILE: lib/librte_eal/linuxapp/eal/eal_pci_vfio_socket.c:76: +#define CMSGHDR_TO_FD(chdr,fd) \ + do {\ + memcpy(&(fd), (chdr).__cmsg_data, sizeof(fd));\ + } while (0) ERROR: "foo * bar" should be "foo *bar" #942: FILE: lib/librte_eal/linuxapp/eal/eal_pci_vfio_socket.c:84: +get_socket_path(char * buffer, int bufsz) ERROR: "foo * bar" should be "foo *bar" #1026: FILE: lib/librte_eal/linuxapp/eal/eal_pci_vfio_socket.c:168: + struct cmsghdr * chdr; ERROR: "foo * bar" should be "foo *bar" #1057: FILE: lib/librte_eal/linuxapp/eal/eal_pci_vfio_socket.c:199: + struct cmsghdr * chdr; ERROR: "foo * bar" should be "foo *bar" #1284: FILE: lib/librte_eal/linuxapp/eal/include/eal_pci_init.h:87: +void * pci_vfio_socket_thread(void *arg); Thanks -- Thomas