diff mbox

[dpdk-dev,1/2] virtio: initialize iopl when device is initialized

Message ID 1425602726-26538-2-git-send-email-stephen@networkplumber.org (mailing list archive)
State Rejected, archived
Headers

Commit Message

Stephen Hemminger March 6, 2015, 12:45 a.m. UTC 
  The virtio driver needs to use in/out instructions therefore it
must initialize using iopl(2) system call. The problem is that
virtio initialization happens very early, and any application
that uses daemon() or calls eal_init later in another context
will fail.

The fix is to move the iopl into rte_eal_init.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_eal/linuxapp/eal/eal.c     | 5 +++++
 lib/librte_pmd_virtio/virtio_ethdev.c | 5 -----
 2 files changed, 5 insertions(+), 5 deletions(-)

Comments

Ouyang Changchun March 6, 2015, 3:41 a.m. UTC | #1 
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen
> Hemminger
> Sent: Friday, March 6, 2015 8:45 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH 1/2] virtio: initialize iopl when device is initialized
> 
> The virtio driver needs to use in/out instructions therefore it must initialize
> using iopl(2) system call. The problem is that virtio initialization happens very
> early, and any application that uses daemon() or calls eal_init later in another
> context will fail.
> 
> The fix is to move the iopl into rte_eal_init.
> 

Why need move virtio specific code into rte_eal_init?
thanks
Changchun
Stephen Hemminger March 6, 2015, 4:20 p.m. UTC | #2 
On Fri, 6 Mar 2015 03:41:25 +0000
"Ouyang, Changchun" <changchun.ouyang@intel.com> wrote:

> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen
> > Hemminger
> > Sent: Friday, March 6, 2015 8:45 AM
> > To: dev@dpdk.org
> > Subject: [dpdk-dev] [PATCH 1/2] virtio: initialize iopl when device is initialized
> > 
> > The virtio driver needs to use in/out instructions therefore it must initialize
> > using iopl(2) system call. The problem is that virtio initialization happens very
> > early, and any application that uses daemon() or calls eal_init later in another
> > context will fail.
> > 
> > The fix is to move the iopl into rte_eal_init.
> > 
> 
> Why need move virtio specific code into rte_eal_init?
> thanks
> Changchun
> 

The issue is that virtio has no place it can do iopl() and have the IRQ thread
work. It only shows up on real code where application is daemon, not in a toy
demo or test application.

Right now:
    gcc start
       rte_virtio_pmd_init
          iopl
    main
        daemon
            fork
                fork 
                  Process is now child of init not original process

                  rte_eal_init
                     fork (pthread) for irq thread
                                                       irq thread
                                                        (no iopl permssion)
                  program start
                  rte_pmd_virtio_configure


So the only place where iopl() can be done in the proper context
so that the IRQ (and other helper threads in future) have the correct
permissions is in eal_init.
David Marchand March 6, 2015, 4:33 p.m. UTC | #3 
Hello Stephen,

On Fri, Mar 6, 2015 at 5:20 PM, Stephen Hemminger <
stephen@networkplumber.org> wrote:

> The issue is that virtio has no place it can do iopl() and have the IRQ
> thread
> work. It only shows up on real code where application is daemon, not in a
> toy
> demo or test application.
>
> Right now:
>     gcc start
>        rte_virtio_pmd_init
>           iopl
>     main
>         daemon
>             fork
>                 fork
>                   Process is now child of init not original process
>
>                   rte_eal_init
>                      fork (pthread) for irq thread
>                                                        irq thread
>                                                         (no iopl permssion)
>                   program start
>                   rte_pmd_virtio_configure
>
>
> So the only place where iopl() can be done in the proper context
> so that the IRQ (and other helper threads in future) have the correct
> permissions is in eal_init.
>

Is eth_virtio_dev_init() not a better place rather than eal ?

I prefer avoiding #ifdef pmd in eal.
Stephen Hemminger March 6, 2015, 4:55 p.m. UTC | #4 
On Fri, 6 Mar 2015 17:33:58 +0100
David Marchand <david.marchand@6wind.com> wrote:

> Is eth_virtio_dev_init() not a better place rather than eal ?
> 
> I prefer avoiding #ifdef pmd in eal.

No virtio_dev_init is called too late, after interrupt handling
threads are spawned.

/* Launch threads, called at application init(). */
int
rte_eal_init(int argc, char **argv)
{
	int i, fctret, ret;
...
	if (rte_eal_intr_init() < 0)
		rte_panic("Cannot init interrupt-handling thread\n");
...
	
	if (rte_eal_dev_init() < 0)
		rte_panic("Cannot init pmd devices\n"
David Marchand March 6, 2015, 10:04 p.m. UTC | #5 
On Fri, Mar 6, 2015 at 5:55 PM, Stephen Hemminger <
stephen@networkplumber.org> wrote:

> No virtio_dev_init is called too late, after interrupt handling
> threads are spawned.
>
> /* Launch threads, called at application init(). */
> int
> rte_eal_init(int argc, char **argv)
> {
>         int i, fctret, ret;
> ...
>         if (rte_eal_intr_init() < 0)
>                 rte_panic("Cannot init interrupt-handling thread\n");
> ...
>
>         if (rte_eal_dev_init() < 0)
>                 rte_panic("Cannot init pmd devices\n"
>

There is no "io level" loss when going through fork and/or daemon : this
affirmation contradicts the manual and my test programs.

Now, looking at the problem from scratch, the constructor in virtio pmd
does not call iopl().
It registers a driver with an init function called from rte_eal_dev_init().
So iopl() is not called at "_start" time (in both virtio static or shared
object cases), but in rte_eal_dev_init().

In the end, the fix would be to move rte_eal_intr_init() after
rte_eal_dev_init().

Can you test this ?
Thanks.
Stephen Hemminger March 6, 2015, 11:43 p.m. UTC | #6 
On Fri, 6 Mar 2015 23:04:33 +0100
David Marchand <david.marchand@6wind.com> wrote:

> In the end, the fix would be to move rte_eal_intr_init() after rte_eal_dev_init().


That would work, but was worried it would break other devices.
David Marchand March 7, 2015, 6:53 a.m. UTC | #7 
On Sat, Mar 7, 2015 at 12:43 AM, Stephen Hemminger <
stephen@networkplumber.org> wrote:

> On Fri, 6 Mar 2015 23:04:33 +0100
> David Marchand <david.marchand@6wind.com> wrote:
>
> > In the end, the fix would be to move rte_eal_intr_init() after
> rte_eal_dev_init().
>
>
> That would work, but was worried it would break other devices.
>

I had checked the pmds before proposing.

rte_intr_* api is called only from the pdev (meaning pci) drivers at the
moment.

rte_eal_dev_init() means we are still in the "pmd init" phase for the pdev
drivers which means we are still initialising per-driver things for them
since they have no idea of the devices to handle (the devices are
discovered later with the pci scan).
In this phase, the code tells that they are not registering interrupts (and
I don't see how they could register interrupts without knowing devices).

The only problem would be for future vdev drivers (current drivers do not
use rte_intr_*).
But this problem would occur because the driver/device datamodel is not
consistent.
If we could rework this to have devices instantiated at the same phase,
then the problem won't happen.
So, we can hope we will have reworked the driver/device datamodel in dpdk,
before this problem hits us.


I only had checked the pmds, so I may have missed something else that uses
interrupts (if any) but I think this is worth trying and fixing for 2.0.
Thomas, opinion ?
David Marchand March 9, 2015, 11:05 a.m. UTC | #8 
So, a little summary.

On Fri, Mar 6, 2015 at 5:20 PM, Stephen Hemminger <
stephen@networkplumber.org> wrote:

> > > The virtio driver needs to use in/out instructions therefore it must
> initialize
> > > using iopl(2) system call. The problem is that virtio initialization
> happens very
> > > early, and any application that uses daemon() or calls eal_init later
> in another
> > > context will fail.
>

Part of this is wrong.
The manual tells that this should be inherited.
I added some fork and daemon to test programs of mine which confirmed the
manual seems to be right.

The real problem is that iopl is not called at "constructor" time for it to
be inherited by irq thread.


> The issue is that virtio has no place it can do iopl() and have the IRQ
> thread
> work. It only shows up on real code where application is daemon, not in a
> toy
> demo or test application.
>

Contrary to what you asserted, the problem does happen when using testpmd
(if this is the toy you are talking about).
So, I would say your real world application is no better than testpmd.


I will post the right fixes.
David Marchand March 9, 2015, 2:56 p.m. UTC | #9 
This patchset fixes a segfault in eal linux when using virtio pmd with link
status change interrupts in place because of an incorrect io privilege level.


Ouyang,

Fixing this has revealed some problems to be fixed (for 2.0.0 at least for the
first):
- changing link status from qemu monitor triggers this error message (even if
  link status seems to be correctly set)

testpmd> EAL: Error reading interrupts status for fd 0

- log macros in virtio pmd (especially for init) are still under a build option.
  They should be aligned on other pmds to make it easier to debug, see ixgbe pmd
  and/or thread:
  http://dpdk.org/ml/archives/dev/2014-September/005450.html

Can you look at these ?

Thanks.
Thomas Monjalon July 29, 2015, 5:26 p.m. UTC | #10 
2015-03-06 08:20, Stephen Hemminger:
> The issue is that virtio has no place it can do iopl() and have the IRQ thread
> work. It only shows up on real code where application is daemon, not in a toy
> demo or test application.
> 
> Right now:
>     gcc start
>        rte_virtio_pmd_init
>           iopl
>     main
>         daemon
>             fork
>                 fork 
>                   Process is now child of init not original process
> 
>                   rte_eal_init
>                      fork (pthread) for irq thread
>                                                        irq thread
>                                                         (no iopl permssion)
>                   program start
>                   rte_pmd_virtio_configure

This call tree is wrong.
Below is the right (and more complete) one:

gcc start
	driver constructor
		rte_eal_driver_register (if .a)
main
	rte_eal_init
		eal_parse_args
		rte_eal_pci_init
		rte_eal_memory_init
		rte_eal_intr_init
			pthread_create
				eal_intr_thread_main
					eal_intr_handle_interrupts
						eal_intr_process_interrupts
							virtio_interrupt_handler
		dlopen (if .so)
			driver constructor
				rte_eal_driver_register
		rte_eal_dev_init
			driver->init
				rte_virtio_pmd_init
					rte_eal_iopl_init
					rte_eth_driver_register
		pthread_create
		rte_eal_pci_probe
			driver->devinit
				rte_eth_dev_init
					rte_eth_dev_allocate
					eth_drv->eth_dev_init
						eth_virtio_dev_init
							virtio_resource_init

> So the only place where iopl() can be done in the proper context
> so that the IRQ (and other helper threads in future) have the correct
> permissions is in eal_init.

No there are 2 other possible solutions:
1) Move rte_eal_intr_init() after rte_eal_dev_init().
2) Move dlopen() before rte_eal_intr_init() and call iopl in the constructor.
With the second solution, we must also keep an iopl call in rte_virtio_pmd_init()
to return an error if iopl fails.

The second solution was proposed in the series sent by David:
	http://dpdk.org/ml/archives/dev/2015-March/014931.html
	http://dpdk.org/ml/archives/dev/2015-March/014932.html
diff mbox

Patch

diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index 16f9e7c..76481f7 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -764,6 +764,11 @@  rte_eal_init(int argc, char **argv)
 		rte_panic("Cannot init IVSHMEM\n");
 #endif
 
+#ifdef RTE_LIBRTE_VIRTIO_PMD
+	if (rte_eal_iopl_init() != 0)
+		rte_panic("IOPL call failed - cannot use virtio PMD");
+#endif
+
 	if (rte_eal_memory_init() < 0)
 		rte_panic("Cannot init memory\n");
 
diff --git a/lib/librte_pmd_virtio/virtio_ethdev.c b/lib/librte_pmd_virtio/virtio_ethdev.c
index d239083..e2600de 100644
--- a/lib/librte_pmd_virtio/virtio_ethdev.c
+++ b/lib/librte_pmd_virtio/virtio_ethdev.c
@@ -1247,11 +1247,6 @@  static int
 rte_virtio_pmd_init(const char *name __rte_unused,
 		    const char *param __rte_unused)
 {
-	if (rte_eal_iopl_init() != 0) {
-		PMD_INIT_LOG(ERR, "IOPL call failed - cannot use virtio PMD");
-		return -1;
-	}
-
 	rte_eth_driver_register(&rte_virtio_pmd);
 	return 0;
 }