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 <anatoly.burakov at intel.com> 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
