[dpdk-dev] [PATCH RFC 0/3] only call iopl when necessary

[dpdk-dev] [PATCH RFC 0/3] only call iopl when necessary

[David Marchand]

This patch series is just a little clean up to remove the unconditionnal call to
iopl on linux.
Rather than call iopl() at the eal level, let the PMD that needs it call
rte_eal_iopl_init().

-- 
David Marchand

David Marchand (3):
  eal/bsd: fix fd leak
  eal: don't call rte_eal_iopl_init unconditionnally
  eal: remove unused flags field

 lib/librte_eal/bsdapp/eal/eal.c         |    6 ++----
 lib/librte_eal/common/include/rte_eal.h |   17 +++++++++++------
 lib/librte_eal/linuxapp/eal/eal.c       |   11 ++++-------
 lib/librte_pmd_virtio/virtio_ethdev.c   |   15 ++++++++-------
 4 files changed, 25 insertions(+), 24 deletions(-)

-- 
1.7.10.4

[dpdk-dev] [PATCH RFC 1/3] eal/bsd: fix fd leak

[David Marchand]

Signed-off-by: David Marchand <david.marchand@6wind.com>
---
 lib/librte_eal/bsdapp/eal/eal.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
index a296da5..0697b05 100644
--- a/lib/librte_eal/bsdapp/eal/eal.c
+++ b/lib/librte_eal/bsdapp/eal/eal.c
@@ -810,6 +810,7 @@ rte_eal_iopl_init(void)
 	fd = open("/dev/io", O_RDWR);
 	if (fd < 0)
 		return -1;
+	close(fd);
 	return 0;
 }
 
-- 
1.7.10.4

[dpdk-dev] [PATCH RFC 2/3] eal: don't call rte_eal_iopl_init unconditionnally

[David Marchand]

There is no need for ioport access for applications that won't use virtio pmds.
Make rte_eal_iopl_init() non-static so that it is called from pmds that need it.

Signed-off-by: David Marchand <david.marchand@6wind.com>
---
 lib/librte_eal/bsdapp/eal/eal.c         |    5 +----
 lib/librte_eal/common/include/rte_eal.h |   14 +++++++++++---
 lib/librte_eal/linuxapp/eal/eal.c       |   11 ++++-------
 lib/librte_pmd_virtio/virtio_ethdev.c   |   15 ++++++++-------
 4 files changed, 24 insertions(+), 21 deletions(-)

diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
index 0697b05..dad9e0c 100644
--- a/lib/librte_eal/bsdapp/eal/eal.c
+++ b/lib/librte_eal/bsdapp/eal/eal.c
@@ -803,7 +803,7 @@ int rte_eal_has_hugepages(void)
 }
 
 /* Abstraction for port I/0 privilege */
-static int
+int
 rte_eal_iopl_init(void)
 {
 	int fd = -1;
@@ -864,9 +864,6 @@ rte_eal_init(int argc, char **argv)
 
 	rte_config_init();
 
-	if (rte_eal_iopl_init() == 0)
-		rte_config.flags |= EAL_FLG_HIGH_IOPL;
-
 	if (rte_eal_cpu_init() < 0)
 		rte_panic("Cannot detect lcores\n");
 
diff --git a/lib/librte_eal/common/include/rte_eal.h b/lib/librte_eal/common/include/rte_eal.h
index 273da9a..8d39cba 100644
--- a/lib/librte_eal/common/include/rte_eal.h
+++ b/lib/librte_eal/common/include/rte_eal.h
@@ -88,9 +88,6 @@ struct rte_config {
 	struct rte_mem_config *mem_config;
 } __attribute__((__packed__));
 
-/* Flag definitions for rte_config flags */
-#define EAL_FLG_HIGH_IOPL 1 /**< indicates high IO privilege in a linux env */
-
 /**
  * Get the global configuration structure.
  *
@@ -119,6 +116,17 @@ enum rte_lcore_role_t rte_eal_lcore_role(unsigned lcore_id);
 enum rte_proc_type_t rte_eal_process_type(void);
 
 /**
+ * Request iopl privilege for all RPL.
+ *
+ * This function should be called by pmds which need access to ioports.
+
+ * @return
+ *   - On success, returns 0.
+ *   - On failure, returns -1.
+ */
+int rte_eal_iopl_init(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 2940e69..884aa9b 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -107,8 +107,6 @@
 
 #define SOCKET_MEM_STRLEN (RTE_MAX_NUMA_NODES * 10)
 
-#define HIGHEST_RPL 3
-
 #define BITS_PER_HEX 4
 
 /* Allow the application to print its usage message too if set */
@@ -1060,10 +1058,12 @@ rte_eal_mcfg_complete(void)
 /*
  * Request iopl privilege for all RPL, returns 0 on success
  */
-static int
+int
 rte_eal_iopl_init(void)
 {
-	return iopl(HIGHEST_RPL);
+	if (iopl(3) != 0)
+		return -1;
+	return 0;
 }
 
 /* Launch threads, called at application init(). */
@@ -1125,9 +1125,6 @@ rte_eal_init(int argc, char **argv)
 
 	rte_config_init();
 
-	if (rte_eal_iopl_init() == 0)
-		rte_config.flags |= EAL_FLG_HIGH_IOPL;
-
 	if (rte_eal_pci_init() < 0)
 		rte_panic("Cannot init PCI\n");
 
diff --git a/lib/librte_pmd_virtio/virtio_ethdev.c b/lib/librte_pmd_virtio/virtio_ethdev.c
index 3344ffb..587b746 100644
--- a/lib/librte_pmd_virtio/virtio_ethdev.c
+++ b/lib/librte_pmd_virtio/virtio_ethdev.c
@@ -736,12 +736,6 @@ eth_virtio_dev_init(__rte_unused struct eth_driver *eth_drv,
 		return -1;
 	}
 
-	if (!(rte_eal_get_configuration()->flags & EAL_FLG_HIGH_IOPL)) {
-		PMD_INIT_LOG(ERR,
-			"IOPL call failed in EAL init - cannot use virtio PMD driver\n");
-		return -1;
-	}
-
 	eth_dev->dev_ops = &virtio_eth_dev_ops;
 	eth_dev->tx_pkt_burst = &virtio_xmit_pkts;
 
@@ -896,8 +890,15 @@ static struct eth_driver rte_virtio_pmd = {
  * Returns 0 on success.
  */
 static int
-rte_virtio_pmd_init(const char *name __rte_unused, const char *param __rte_unused)
+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 driver\n");
+		return -1;
+	}
+
 	rte_eth_driver_register(&rte_virtio_pmd);
 	return 0;
 }
-- 
1.7.10.4

[dpdk-dev] [PATCH RFC 3/3] eal: remove unused flags field

[David Marchand]

This field is not used anymore, remove it.

Signed-off-by: David Marchand <david.marchand@6wind.com>
---
 lib/librte_eal/common/include/rte_eal.h |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/lib/librte_eal/common/include/rte_eal.h b/lib/librte_eal/common/include/rte_eal.h
index 8d39cba..f2c4b7d 100644
--- a/lib/librte_eal/common/include/rte_eal.h
+++ b/lib/librte_eal/common/include/rte_eal.h
@@ -78,9 +78,6 @@ struct rte_config {
 	/** Primary or secondary configuration */
 	enum rte_proc_type_t process_type;
 
-	/** A set of general status flags */
-	unsigned flags;
-
 	/**
 	 * Pointer to memory configuration, which may be shared across multiple
 	 * Intel DPDK instances
-- 
1.7.10.4

Re: [dpdk-dev] [PATCH RFC 0/3] only call iopl when necessary

[Xie, Huawei]

Hi David:
The reason iopl is put in rte_eal_init is that we want all later created DPDK processes/threads inherit the iopl permission.
If you only call iopl in pmd_init, RX/TX and other threads which needs io permission will segmentation fault.

-huawei

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of David Marchand
> Sent: Tuesday, August 26, 2014 10:12 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH RFC 0/3] only call iopl when necessary
> 
> This patch series is just a little clean up to remove the unconditionnal call to
> iopl on linux.
> Rather than call iopl() at the eal level, let the PMD that needs it call
> rte_eal_iopl_init().
> 
> --
> David Marchand
> 
> David Marchand (3):
>   eal/bsd: fix fd leak
>   eal: don't call rte_eal_iopl_init unconditionnally
>   eal: remove unused flags field
> 
>  lib/librte_eal/bsdapp/eal/eal.c         |    6 ++----
>  lib/librte_eal/common/include/rte_eal.h |   17 +++++++++++------
>  lib/librte_eal/linuxapp/eal/eal.c       |   11 ++++-------
>  lib/librte_pmd_virtio/virtio_ethdev.c   |   15 ++++++++-------
>  4 files changed, 25 insertions(+), 24 deletions(-)
> 
> --
> 1.7.10.4

Re: [dpdk-dev] [PATCH RFC 0/3] only call iopl when necessary

[David Marchand]

Hello,

On Wed, Aug 27, 2014 at 11:22 AM, Xie, Huawei <huawei.xie@intel.com> wrote:

> Hi David:
> The reason iopl is put in rte_eal_init is that we want all later created
> DPDK processes/threads inherit the iopl permission.
> If you only call iopl in pmd_init, RX/TX and other threads which needs io
> permission will segmentation fault.
>

rte_virtio_pmd_init() is the constructor that registers the virtio pmd in
the driver list.
It is called from rte_eal_dev_init() in rte_eal_init().
So it means my patch only moved the call to iopl() a little later in
rte_eal_init() and this is still before any thread has been created.

I can't see why this would cause a problem.


-- 
David Marchand

Re: [dpdk-dev] [PATCH RFC 0/3] only call iopl when necessary

[Xie, Huawei]

That is ok. If virtio PMD is a dynamic linked library, is it possible that virtio PMD is loaded later?

From: David Marchand [mailto:david.marchand@6wind.com]
Sent: Wednesday, August 27, 2014 5:34 PM
To: Xie, Huawei
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH RFC 0/3] only call iopl when necessary

Hello,

On Wed, Aug 27, 2014 at 11:22 AM, Xie, Huawei <huawei.xie@intel.com<mailto:huawei.xie@intel.com>> wrote:
Hi David:
The reason iopl is put in rte_eal_init is that we want all later created DPDK processes/threads inherit the iopl permission.
If you only call iopl in pmd_init, RX/TX and other threads which needs io permission will segmentation fault.

rte_virtio_pmd_init() is the constructor that registers the virtio pmd in the driver list.
It is called from rte_eal_dev_init() in rte_eal_init().
So it means my patch only moved the call to iopl() a little later in rte_eal_init() and this is still before any thread has been created.

I can't see why this would cause a problem.


--
David Marchand

Re: [dpdk-dev] [PATCH RFC 0/3] only call iopl when necessary

[David Marchand]

On Wed, Aug 27, 2014 at 11:57 AM, Xie, Huawei <huawei.xie@intel.com> wrote:

>  That is ok. If virtio PMD is a dynamic linked library, is it possible
> that virtio PMD is loaded later?
>

The shared library are loaded through -d option, which are loaded just
before the call to rte_eal_dev_init().

-- 
David Marchand

>

Re: [dpdk-dev] [PATCH RFC 0/3] only call iopl when necessary

[Xie, Huawei]

ok,  as long as we ensure with virtio PMD, the parent DPDK process has the iopl  permission so that all child threads inherits the permission.

From: David Marchand [mailto:david.marchand@6wind.com]
Sent: Wednesday, August 27, 2014 6:05 PM
To: Xie, Huawei
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH RFC 0/3] only call iopl when necessary


On Wed, Aug 27, 2014 at 11:57 AM, Xie, Huawei <huawei.xie@intel.com<mailto:huawei.xie@intel.com>> wrote:
That is ok. If virtio PMD is a dynamic linked library, is it possible that virtio PMD is loaded later?

The shared library are loaded through -d option, which are loaded just before the call to rte_eal_dev_init().

--
David Marchand

Re: [dpdk-dev] [PATCH RFC 0/3] only call iopl when necessary

[Thomas Monjalon]

Hi Huawei,

2014-08-27 10:09, Xie, Huawei:
> ok, as long as we ensure with virtio PMD, the parent DPDK process has the
> iopl permission so that all child threads inherits the permission.

If you think the patch serie is ok, a "Reviewed-by" would be appreciated.

Also, please could you check your email settings to have cite marks
(">") in text version of your email?
Reminder 1: only text emails are forwared to the list
Reminder 2: we should answer below the question (no top posting)

Thanks
-- 
Thomas


> From: David Marchand [mailto:david.marchand@6wind.com]
> Sent: Wednesday, August 27, 2014 6:05 PM
> To: Xie, Huawei
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH RFC 0/3] only call iopl when necessary
> 
> 
> On Wed, Aug 27, 2014 at 11:57 AM, Xie, Huawei <huawei.xie@intel.com<mailto:huawei.xie@intel.com>> wrote:
> That is ok. If virtio PMD is a dynamic linked library, is it possible that virtio PMD is loaded later?
> 
> The shared library are loaded through -d option, which are loaded just before the call to rte_eal_dev_init().
> 
> --
> David Marchand

Re: [dpdk-dev] [PATCH RFC 2/3] eal: don't call rte_eal_iopl_init unconditionnally

[Xie, Huawei]

Acked-by: Huawei Xie <huawei.xie@intel.com>

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of David Marchand
> Sent: Tuesday, August 26, 2014 10:12 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH RFC 2/3] eal: don't call rte_eal_iopl_init
> unconditionnally
> 
> There is no need for ioport access for applications that won't use virtio pmds.
> Make rte_eal_iopl_init() non-static so that it is called from pmds that need it.
> 
> Signed-off-by: David Marchand <david.marchand@6wind.com>
> ---
>  lib/librte_eal/bsdapp/eal/eal.c         |    5 +----
>  lib/librte_eal/common/include/rte_eal.h |   14 +++++++++++---
>  lib/librte_eal/linuxapp/eal/eal.c       |   11 ++++-------
>  lib/librte_pmd_virtio/virtio_ethdev.c   |   15 ++++++++-------
>  4 files changed, 24 insertions(+), 21 deletions(-)
> 
> diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
> index 0697b05..dad9e0c 100644
> --- a/lib/librte_eal/bsdapp/eal/eal.c
> +++ b/lib/librte_eal/bsdapp/eal/eal.c
> @@ -803,7 +803,7 @@ int rte_eal_has_hugepages(void)
>  }
> 
>  /* Abstraction for port I/0 privilege */
> -static int
> +int
>  rte_eal_iopl_init(void)
>  {
>  	int fd = -1;
> @@ -864,9 +864,6 @@ rte_eal_init(int argc, char **argv)
> 
>  	rte_config_init();
> 
> -	if (rte_eal_iopl_init() == 0)
> -		rte_config.flags |= EAL_FLG_HIGH_IOPL;
> -
>  	if (rte_eal_cpu_init() < 0)
>  		rte_panic("Cannot detect lcores\n");
> 
> diff --git a/lib/librte_eal/common/include/rte_eal.h
> b/lib/librte_eal/common/include/rte_eal.h
> index 273da9a..8d39cba 100644
> --- a/lib/librte_eal/common/include/rte_eal.h
> +++ b/lib/librte_eal/common/include/rte_eal.h
> @@ -88,9 +88,6 @@ struct rte_config {
>  	struct rte_mem_config *mem_config;
>  } __attribute__((__packed__));
> 
> -/* Flag definitions for rte_config flags */
> -#define EAL_FLG_HIGH_IOPL 1 /**< indicates high IO privilege in a linux env */
> -
>  /**
>   * Get the global configuration structure.
>   *
> @@ -119,6 +116,17 @@ enum rte_lcore_role_t rte_eal_lcore_role(unsigned
> lcore_id);
>  enum rte_proc_type_t rte_eal_process_type(void);
> 
>  /**
> + * Request iopl privilege for all RPL.
> + *
> + * This function should be called by pmds which need access to ioports.
> +
> + * @return
> + *   - On success, returns 0.
> + *   - On failure, returns -1.
> + */
> +int rte_eal_iopl_init(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 2940e69..884aa9b 100644
> --- a/lib/librte_eal/linuxapp/eal/eal.c
> +++ b/lib/librte_eal/linuxapp/eal/eal.c
> @@ -107,8 +107,6 @@
> 
>  #define SOCKET_MEM_STRLEN (RTE_MAX_NUMA_NODES * 10)
> 
> -#define HIGHEST_RPL 3
> -
>  #define BITS_PER_HEX 4
> 
>  /* Allow the application to print its usage message too if set */
> @@ -1060,10 +1058,12 @@ rte_eal_mcfg_complete(void)
>  /*
>   * Request iopl privilege for all RPL, returns 0 on success
>   */
> -static int
> +int
>  rte_eal_iopl_init(void)
>  {
> -	return iopl(HIGHEST_RPL);
> +	if (iopl(3) != 0)
> +		return -1;
> +	return 0;
>  }
> 
>  /* Launch threads, called at application init(). */
> @@ -1125,9 +1125,6 @@ rte_eal_init(int argc, char **argv)
> 
>  	rte_config_init();
> 
> -	if (rte_eal_iopl_init() == 0)
> -		rte_config.flags |= EAL_FLG_HIGH_IOPL;
> -
>  	if (rte_eal_pci_init() < 0)
>  		rte_panic("Cannot init PCI\n");
> 
> diff --git a/lib/librte_pmd_virtio/virtio_ethdev.c
> b/lib/librte_pmd_virtio/virtio_ethdev.c
> index 3344ffb..587b746 100644
> --- a/lib/librte_pmd_virtio/virtio_ethdev.c
> +++ b/lib/librte_pmd_virtio/virtio_ethdev.c
> @@ -736,12 +736,6 @@ eth_virtio_dev_init(__rte_unused struct eth_driver
> *eth_drv,
>  		return -1;
>  	}
> 
> -	if (!(rte_eal_get_configuration()->flags & EAL_FLG_HIGH_IOPL)) {
> -		PMD_INIT_LOG(ERR,
> -			"IOPL call failed in EAL init - cannot use virtio PMD
> driver\n");
> -		return -1;
> -	}
> -
>  	eth_dev->dev_ops = &virtio_eth_dev_ops;
>  	eth_dev->tx_pkt_burst = &virtio_xmit_pkts;
> 
> @@ -896,8 +890,15 @@ static struct eth_driver rte_virtio_pmd = {
>   * Returns 0 on success.
>   */
>  static int
> -rte_virtio_pmd_init(const char *name __rte_unused, const char *param
> __rte_unused)
> +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 driver\n");
> +		return -1;
> +	}
> +
>  	rte_eth_driver_register(&rte_virtio_pmd);
>  	return 0;
>  }
> --
> 1.7.10.4

Re: [dpdk-dev] [PATCH RFC 0/3] only call iopl when necessary

[Patel, Rashmin N]

I think iopl() was added to have permission to do Port I/O for Virtio device handling.

-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Xie, Huawei
Sent: Wednesday, August 27, 2014 2:22 AM
To: David Marchand; dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH RFC 0/3] only call iopl when necessary

Hi David:
The reason iopl is put in rte_eal_init is that we want all later created DPDK processes/threads inherit the iopl permission.
If you only call iopl in pmd_init, RX/TX and other threads which needs io permission will segmentation fault.

-huawei

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of David Marchand
> Sent: Tuesday, August 26, 2014 10:12 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH RFC 0/3] only call iopl when necessary
> 
> This patch series is just a little clean up to remove the 
> unconditionnal call to iopl on linux.
> Rather than call iopl() at the eal level, let the PMD that needs it 
> call rte_eal_iopl_init().
> 
> --
> David Marchand
> 
> David Marchand (3):
>   eal/bsd: fix fd leak
>   eal: don't call rte_eal_iopl_init unconditionnally
>   eal: remove unused flags field
> 
>  lib/librte_eal/bsdapp/eal/eal.c         |    6 ++----
>  lib/librte_eal/common/include/rte_eal.h |   17 +++++++++++------
>  lib/librte_eal/linuxapp/eal/eal.c       |   11 ++++-------
>  lib/librte_pmd_virtio/virtio_ethdev.c   |   15 ++++++++-------
>  4 files changed, 25 insertions(+), 24 deletions(-)
> 
> --
> 1.7.10.4

Re: [dpdk-dev] [PATCH RFC 0/3] only call iopl when necessary

[Stephen Hemminger]

Yes. Iopl is to allow out from user space. I played with ioperm instead but
that is per thread and was a nuisance.

Re: [dpdk-dev] [PATCH RFC 1/3] eal/bsd: fix fd leak

[Thomas Monjalon]

Hi,

Is there any BSD users available to test this patch?

I'd like to add this comment in the log:
----
"The initial implementation simply raised the IOPL of the current thread
when open(2) was called on the device. This behaviour is retained in the
current implementation as legacy support for both i386 and amd64."
    http://www.freebsd.org/cgi/man.cgi?query=io&sektion=4

Nothing prevents from closing it just after.
----

But it should be tested with virtio to check iopl permissions.

Thanks
-- 
Thomas


2014-08-26 16:11, David Marchand:
> Signed-off-by: David Marchand <david.marchand@6wind.com>
> ---
>  lib/librte_eal/bsdapp/eal/eal.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
> index a296da5..0697b05 100644
> --- a/lib/librte_eal/bsdapp/eal/eal.c
> +++ b/lib/librte_eal/bsdapp/eal/eal.c
> @@ -810,6 +810,7 @@ rte_eal_iopl_init(void)
>  	fd = open("/dev/io", O_RDWR);
>  	if (fd < 0)
>  		return -1;
> +	close(fd);
>  	return 0;
>  }

Re: [dpdk-dev] [PATCH RFC 0/3] only call iopl when necessary

[Thomas Monjalon]

2014-08-26 16:11, David Marchand:
> This patch series is just a little clean up to remove the unconditionnal call to
> iopl on linux.
> Rather than call iopl() at the eal level, let the PMD that needs it call
> rte_eal_iopl_init().

Acked and applied

Thanks for the cleanup
-- 
Thomas