[v2] net/virtio: avoid annoying IOPL call related errors

Message ID 20181123143620.10480-1-i.maximets@samsung.com (mailing list archive)
State Superseded, archived
Headers
Series [v2] net/virtio: avoid annoying IOPL call related errors |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS

Commit Message

Ilya Maximets Nov. 23, 2018, 2:36 p.m. UTC
  In case of running with not enough capabilities, i.e. running as
non-root user any application linked with DPDK prints the message
about IOPL call failure even if it was just called like
'./testpmd --help'. For example, this beaks most of the OVS unit
tests if it built with DPDK support.

Let's register the virtio driver unconditionally and print error
message while probing the device. Silent iopl() call left in the
constructor to have privileges as early as possible as it was before.

Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---

Version 2:
	* Fixed possible fd leak on BSD.

We can avoid test failures in OVS by filtering the output
like this:
  https://patchwork.ozlabs.org/project/openvswitch/list/?series=77706

But it still looks very inconvenient for me to have this
message in the output of every command for the DPDK linked app.

 drivers/net/virtio/virtio_ethdev.c | 11 ++++++-----
 lib/librte_eal/bsdapp/eal/eal.c    |  6 ++++--
 2 files changed, 10 insertions(+), 7 deletions(-)
  

Comments

David Marchand Nov. 23, 2018, 3:02 p.m. UTC | #1
On Fri, Nov 23, 2018 at 3:36 PM Ilya Maximets <i.maximets@samsung.com>
wrote:

> In case of running with not enough capabilities, i.e. running as
> non-root user any application linked with DPDK prints the message
> about IOPL call failure even if it was just called like
> './testpmd --help'. For example, this beaks most of the OVS unit
> tests if it built with DPDK support.
>

breaks


>
> Let's register the virtio driver unconditionally and print error
> message while probing the device. Silent iopl() call left in the
> constructor to have privileges as early as possible as it was before.
>

Yes, that's the important part to avoid issues with the interrupt thread
which is spawned later and inherits the capa from the thread running
rte_eal_init, iirc.


> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>

Reviewed-by: David Marchand <david.marchand@redhat.com>

I wonder if we could move the "new" iopl check even later when probing a
device since it only makes sense in legacy mode when using uio.
But this patch keeps previous behaviour.

diff --git a/lib/librte_eal/bsdapp/eal/eal.c
> b/lib/librte_eal/bsdapp/eal/eal.c
> index 508cbc46f..b8152a75c 100644
> --- a/lib/librte_eal/bsdapp/eal/eal.c
> +++ b/lib/librte_eal/bsdapp/eal/eal.c
> @@ -556,9 +556,11 @@ int rte_eal_has_hugepages(void)
>  int
>  rte_eal_iopl_init(void)
>  {
> -       static int fd;
> +       static int fd = -1;
> +
> +       if (fd < 0)
> +               fd = open("/dev/io", O_RDWR);
>
> -       fd = open("/dev/io", O_RDWR);
>         if (fd < 0)
>                 return -1;
>         /* keep fd open for iopl */
>

Good catch.
Should be a separate fix.
  
Timothy Redaelli Nov. 23, 2018, 5:15 p.m. UTC | #2
On Fri, 23 Nov 2018 17:36:20 +0300
Ilya Maximets <i.maximets@samsung.com> wrote:

> In case of running with not enough capabilities, i.e. running as
> non-root user any application linked with DPDK prints the message
> about IOPL call failure even if it was just called like
> './testpmd --help'. For example, this beaks most of the OVS unit
> tests if it built with DPDK support.
> 
> Let's register the virtio driver unconditionally and print error
> message while probing the device. Silent iopl() call left in the
> constructor to have privileges as early as possible as it was before.
> 
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> ---
> 
> Version 2:
> 	* Fixed possible fd leak on BSD.
> 
> We can avoid test failures in OVS by filtering the output
> like this:
>   https://patchwork.ozlabs.org/project/openvswitch/list/?series=77706
> 
> But it still looks very inconvenient for me to have this
> message in the output of every command for the DPDK linked app.
> 
>  drivers/net/virtio/virtio_ethdev.c | 11 ++++++-----
>  lib/librte_eal/bsdapp/eal/eal.c    |  6 ++++--
>  2 files changed, 10 insertions(+), 7 deletions(-)

Without this commit, if you link OVS as shared library
(--enable-shared), you'll also have this annoying message every time
you open a new (bash) shell, as user, due to OVS bash-completion:

[tredaell@aldebaran ~]$ bash
rte_virtio_pmd_init(): IOPL call failed - cannot use virtio PMD
rte_virtio_pmd_init(): IOPL call failed - cannot use virtio PMD
[tredaell@aldebaran ~]$

Acked-By: Timothy Redaelli <tredaelli@redhat.com>
  

Patch

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index e1fe36a23..2ba66d291 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1760,6 +1760,11 @@  vdpa_mode_selected(struct rte_devargs *devargs)
 static int eth_virtio_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 	struct rte_pci_device *pci_dev)
 {
+	if (rte_eal_iopl_init() != 0) {
+		PMD_INIT_LOG(ERR, "IOPL call failed - cannot use virtio PMD");
+		return 1;
+	}
+
 	/* virtio pmd skips probe if device needs to work in vdpa mode */
 	if (vdpa_mode_selected(pci_dev->device.devargs))
 		return 1;
@@ -1785,11 +1790,7 @@  static struct rte_pci_driver rte_virtio_pmd = {
 
 RTE_INIT(rte_virtio_pmd_init)
 {
-	if (rte_eal_iopl_init() != 0) {
-		PMD_INIT_LOG(ERR, "IOPL call failed - cannot use virtio PMD");
-		return;
-	}
-
+	rte_eal_iopl_init();
 	rte_pci_register(&rte_virtio_pmd);
 }
 
diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
index 508cbc46f..b8152a75c 100644
--- a/lib/librte_eal/bsdapp/eal/eal.c
+++ b/lib/librte_eal/bsdapp/eal/eal.c
@@ -556,9 +556,11 @@  int rte_eal_has_hugepages(void)
 int
 rte_eal_iopl_init(void)
 {
-	static int fd;
+	static int fd = -1;
+
+	if (fd < 0)
+		fd = open("/dev/io", O_RDWR);
 
-	fd = open("/dev/io", O_RDWR);
 	if (fd < 0)
 		return -1;
 	/* keep fd open for iopl */