Re: [dpdk-dev] [PATCH v5] Restore support for virtio on FreeBSD

["Raz Amir" <razamir22@gmail.com>]

Hi,

>From both running and reading the code, the rte_virtio_pmd_init is called
only once from: rte_eal_init -> rte_eal_dev_init.
But, the uninit won't be called, since uninit it called only for PMD_VDEV
driver types, while virtio is PMD_PDEV.
Based on that, I am going to submit the original patch again, that fd won't
be closed, and without handling the close at uninit as it isn't called and
since fd will be closed anyway when the process exits.

Thanks,
Raz.

-----Original Message-----
From: Ouyang, Changchun [mailto:changchun.ouyang@intel.com] 
Sent: 16 April 2015 06:31
To: Ananyev, Konstantin; Raz Amir; dev@dpdk.org
Cc: Ouyang, Changchun
Subject: RE: [dpdk-dev] [PATCH v5] Restore support for virtio on FreeBSD



> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev, 
> Konstantin
> Sent: Wednesday, April 15, 2015 6:22 AM
> To: Raz Amir; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v5] Restore support for virtio on 
> FreeBSD
> 
> Hi,
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Raz Amir
> > Sent: Tuesday, April 14, 2015 5:23 PM
> > To: dev@dpdk.org
> > Cc: Raz Amir
> > Subject: [dpdk-dev] [PATCH v5] Restore support for virtio on FreeBSD
> >
> > Fixes: 8a312224bcde ("eal/bsd: fix fd leak")
> >
> > Closing /dev/io fd causes SIGBUS in inb/outb instructions as the 
> > process loses the IOPL privileges once the fd is closed:
> > (gdb) bt
> > 0  0x0000000000492f2c in outb (port=49170, data=0 '\000')
> >     at /usr/include/machine/cpufunc.h:244
> > 1  0x0000000000492f7a in outb_p (data=0 '\000', port=49170)
> >     at /dpdk/dpdk-2.0.0/lib/librte_pmd_virtio/virtio_pci.h:211
> > 2  0x000000000049328d in vtpci_set_status (hw=0x80331f380, status=0
> '\000')
> >     at /dpdk/dpdk-2.0.0/lib/librte_pmd_virtio/virtio_pci.c:130
> > 3  0x00000000004931fe in vtpci_reset (hw=0x80331f380)
> >     at /dpdk/dpdk-2.0.0/lib/librte_pmd_virtio/virtio_pci.c:108
> > 4  0x00000000004a175e in eth_virtio_dev_init (eth_dev=0x831b80
> <rte_eth_devices>)
> >     at /dpdk/dpdk-2.0.0/lib/librte_pmd_virtio/virtio_ethdev.c:1150
> > 5  0x0000000000462c09 in rte_eth_dev_init (pci_drv=0x79d1a0
> <rte_virtio_pmd>,
> >     pci_dev=0x802417560) at
> > /dpdk/dpdk-2.0.0/lib/librte_ether/rte_ethdev.c:326
> > 6  0x000000000046f03f in rte_eal_pci_probe_one_driver (dr=0x79d1a0
> <rte_virtio_pmd>,
> >     dev=0x802417560) at
> > /dpdk/dpdk-2.0.0/lib/librte_eal/bsdapp/eal/eal_pci.c:487
> > 7  0x0000000000475b06 in pci_probe_all_drivers (dev=0x802417560)
> >     at /dpdk/dpdk-2.0.0/lib/librte_eal/common/eal_common_pci.c:116
> > 8  0x0000000000475bb9 in rte_eal_pci_probe ()
> >     at /dpdk/dpdk-2.0.0/lib/librte_eal/common/eal_common_pci.c:246
> > 9  0x000000000046cd63 in rte_eal_init (argc=5, argv=0x7fffffffeaf0)
> >     at /dpdk/dpdk-2.0.0/lib/librte_eal/bsdapp/eal/eal.c:554
> > 10 0x0000000000404544 in main ()
> >
> > Signed-off-by: Raz Amir <razamir22@gmail.com>
> > ---
> >  lib/librte_eal/bsdapp/eal/eal.c         | 19 ++++++++++++++-----
> >  lib/librte_eal/common/include/rte_eal.h | 10 ++++++++++
> >  lib/librte_eal/linuxapp/eal/eal.c       |  5 +++++
> >  lib/librte_pmd_virtio/virtio_ethdev.c   |  9 +++++++++
> >  4 files changed, 38 insertions(+), 5 deletions(-)
> >
> > diff --git a/lib/librte_eal/bsdapp/eal/eal.c 
> > b/lib/librte_eal/bsdapp/eal/eal.c index 871d5f4..687dd83 100644
> > --- a/lib/librte_eal/bsdapp/eal/eal.c
> > +++ b/lib/librte_eal/bsdapp/eal/eal.c
> > @@ -112,6 +112,9 @@ struct internal_config internal_config;
> >  /* used by rte_rdtsc() */
> >  int rte_cycles_vmware_tsc_map;
> >
> > +/* fd to keep open for iopl */
> > +static int iopl_fd = -1;
> > +
> >  /* Return a pointer to the configuration structure */  struct 
> > rte_config *
> >  rte_eal_get_configuration(void)
> > @@ -421,15 +424,21 @@ int rte_eal_has_hugepages(void)  int
> >  rte_eal_iopl_init(void)
> >  {
> > -	int fd;
> > -
> > -	fd = open("/dev/io", O_RDWR);
> > -	if (fd < 0)
> > +	iopl_fd = open("/dev/io", O_RDWR);
> > +	if (iopl_fd < 0)
> >  		return -1;
> > -	close(fd);
> > +	/* keep fd open for iopl */
> >  	return 0;
> >  }
> >
> > +void
> > +rte_eal_iopl_uninit(void)
> > +{
> > +	if (iopl_fd != -1)
> > +		close(iopl_fd);
> > +	iopl_fd = -1;
> > +}
> 
> Did I get it right: that function would be invoked for at dev_detach()?
> And after we invoked it, we still we can have other multiple virtio 
> devices attached and active?
> If so, then I suppose you'll hit the same problem again.
> Konstantin
> 

Yes, need verify this issue,
If it is true, may use reference counter to resolve it?
Thanks
Changchun
 
> > +
> >  /* Launch threads, called at application init(). */  int 
> > rte_eal_init(int argc, char **argv) diff --git 
> > a/lib/librte_eal/common/include/rte_eal.h
> > b/lib/librte_eal/common/include/rte_eal.h
> > index 1385a73..9151e08 100644
> > --- a/lib/librte_eal/common/include/rte_eal.h
> > +++ b/lib/librte_eal/common/include/rte_eal.h
> > @@ -127,6 +127,16 @@ enum rte_proc_type_t
> rte_eal_process_type(void);
> > int rte_eal_iopl_init(void);
> >
> >  /**
> > + * Release iopl priviledge - currently relevant only for FreeBSD.
> > + *
> > + * This function should be called by pmds which need access to ioports.
> > +
> > + * @return
> > + *   void
> > + */
> > +void rte_eal_iopl_uninit(void);
> > +
> > +/**
> >   * Initialize the Environment Abstraction Layer (EAL).
> >   *
> >   * This function is to be executed on the MASTER lcore only, as 
> > soon diff --git a/lib/librte_eal/linuxapp/eal/eal.c
> > b/lib/librte_eal/linuxapp/eal/eal.c
> > index bd770cf..687cebf 100644
> > --- a/lib/librte_eal/linuxapp/eal/eal.c
> > +++ b/lib/librte_eal/linuxapp/eal/eal.c
> > @@ -695,6 +695,11 @@ rte_eal_iopl_init(void)  #endif  }
> >
> > +void
> > +rte_eal_iopl_uninit(void)
> > +{
> > +}
> > +
> >  /* Launch threads, called at application init(). */  int 
> > rte_eal_init(int argc, char **argv) diff --git 
> > a/lib/librte_pmd_virtio/virtio_ethdev.c
> > b/lib/librte_pmd_virtio/virtio_ethdev.c
> > index 7b83d9b..5be5c27 100644
> > --- a/lib/librte_pmd_virtio/virtio_ethdev.c
> > +++ b/lib/librte_pmd_virtio/virtio_ethdev.c
> > @@ -1265,6 +1265,14 @@ rte_virtio_pmd_init(const char *name
> __rte_unused,
> >  	return 0;
> >  }
> >
> > +static int
> > +rte_virtio_pmd_uninit(const char *name) {
> > +	(void)name;
> > +	rte_eal_iopl_uninit();
> > +	return 0;
> > +}
> > +
> >  /*
> >   * Only 1 queue is supported, no queue release related operation
> >   */
> > @@ -1499,6 +1507,7 @@ __rte_unused uint8_t is_rx)  static struct 
> > rte_driver rte_virtio_driver = {
> >  	.type = PMD_PDEV,
> >  	.init = rte_virtio_pmd_init,
> > +	.uninit = rte_virtio_pmd_uninit,
> >  };
> >
> >  PMD_REGISTER_DRIVER(rte_virtio_driver);
> > --
> > 2.1.2