Re: [PATCH v2 08/16] Add support for mapping devices through VFIO.

[Thomas Monjalon <thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>]

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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

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