diff mbox series

eal: fix circular dependency in EAL proc type detection

Message ID be78bcd8da77c58dc43764e562295485f987ea71.1531911034.git.anatoly.burakov@intel.com (mailing list archive)
State Accepted, archived
Delegated to: Thomas Monjalon
Headers
Series eal: fix circular dependency in EAL proc type detection |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK

Commit Message

Burakov, Anatoly July 18, 2018, 10:53 a.m. UTC 
  Currently, we need runtime dir to put all of our runtime info in,
including the DPDK shared config. However, we use the shared
config to determine our proc type, and this happens earlier than
we actually create the config dir and thus can know where to
place the config file.

Fix this by moving runtime dir creation right after the EAL
arguments parsing, but before proc type autodetection. Also,
previously we were creating the config file unconditionally,
even if we specified no_shconf - fix it by only creating
the config file if no_shconf is not set.

Fixes: adf1d867361c ("eal: move runtime config file to new location")

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/bsdapp/eal/eal.c   | 33 ++++++++++++++++++-------------
 lib/librte_eal/linuxapp/eal/eal.c | 33 ++++++++++++++++++-------------
 2 files changed, 38 insertions(+), 28 deletions(-)

Comments

Yao, Lei A July 18, 2018, 3:58 p.m. UTC | #1 
> -----Original Message-----
> From: Burakov, Anatoly
> Sent: Wednesday, July 18, 2018 11:54 AM
> To: dev@dpdk.org
> Cc: Richardson, Bruce <bruce.richardson@intel.com>; Xu, Qian Q
> <qian.q.xu@intel.com>; Yao, Lei A <lei.a.yao@intel.com>; Lu, PeipeiX
> <peipeix.lu@intel.com>
> Subject: [PATCH] eal: fix circular dependency in EAL proc type detection
> 
> Currently, we need runtime dir to put all of our runtime info in,
> including the DPDK shared config. However, we use the shared
> config to determine our proc type, and this happens earlier than
> we actually create the config dir and thus can know where to
> place the config file.
> 
> Fix this by moving runtime dir creation right after the EAL
> arguments parsing, but before proc type autodetection. Also,
> previously we were creating the config file unconditionally,
> even if we specified no_shconf - fix it by only creating
> the config file if no_shconf is not set.
> 
> Fixes: adf1d867361c ("eal: move runtime config file to new location")
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Tested-by: Yao Lei<lei.a.yao@intel.com>
This patch pass the test with simple_mp sample. The secondary process
can recognize itself as "secondary process" even use  "--proc-type=auto"
parameter. 
> ---
>  lib/librte_eal/bsdapp/eal/eal.c   | 33 ++++++++++++++++++-------------
>  lib/librte_eal/linuxapp/eal/eal.c | 33 ++++++++++++++++++-------------
>  2 files changed, 38 insertions(+), 28 deletions(-)
> 
> diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
> index 73cdf07b8..7b399bc9d 100644
> --- a/lib/librte_eal/bsdapp/eal/eal.c
> +++ b/lib/librte_eal/bsdapp/eal/eal.c
> @@ -286,12 +286,17 @@ eal_proc_type_detect(void)
>  	enum rte_proc_type_t ptype = RTE_PROC_PRIMARY;
>  	const char *pathname = eal_runtime_config_path();
> 
> -	/* if we can open the file but not get a write-lock we are a secondary
> -	 * process. NOTE: if we get a file handle back, we keep that open
> -	 * and don't close it to prevent a race condition between multiple
> opens */
> -	if (((mem_cfg_fd = open(pathname, O_RDWR)) >= 0) &&
> -			(fcntl(mem_cfg_fd, F_SETLK, &wr_lock) < 0))
> -		ptype = RTE_PROC_SECONDARY;
> +	/* if there no shared config, there can be no secondary processes */
> +	if (!internal_config.no_shconf) {
> +		/* if we can open the file but not get a write-lock we are a
> +		 * secondary process. NOTE: if we get a file handle back, we
> +		 * keep that open and don't close it to prevent a race
> condition
> +		 * between multiple opens.
> +		 */
> +		if (((mem_cfg_fd = open(pathname, O_RDWR)) >= 0) &&
> +				(fcntl(mem_cfg_fd, F_SETLK, &wr_lock) < 0))
> +			ptype = RTE_PROC_SECONDARY;
> +	}
> 
>  	RTE_LOG(INFO, EAL, "Auto-detected process type: %s\n",
>  			ptype == RTE_PROC_PRIMARY ? "PRIMARY" :
> "SECONDARY");
> @@ -468,6 +473,14 @@ eal_parse_args(int argc, char **argv)
>  		}
>  	}
> 
> +	/* create runtime data directory */
> +	if (internal_config.no_shconf == 0 &&
> +			eal_create_runtime_dir() < 0) {
> +		RTE_LOG(ERR, EAL, "Cannot create runtime directory\n");
> +		ret = -1;
> +		goto out;
> +	}
> +
>  	if (eal_adjust_config(&internal_config) != 0) {
>  		ret = -1;
>  		goto out;
> @@ -600,14 +613,6 @@ rte_eal_init(int argc, char **argv)
>  		return -1;
>  	}
> 
> -	/* create runtime data directory */
> -	if (internal_config.no_shconf == 0 &&
> -			eal_create_runtime_dir() < 0) {
> -		rte_eal_init_alert("Cannot create runtime directory\n");
> -		rte_errno = EACCES;
> -		return -1;
> -	}
> -
>  	/* FreeBSD always uses legacy memory model */
>  	internal_config.legacy_mem = true;
> 
> diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
> index d75ae9dae..d2d5aae80 100644
> --- a/lib/librte_eal/linuxapp/eal/eal.c
> +++ b/lib/librte_eal/linuxapp/eal/eal.c
> @@ -344,12 +344,17 @@ eal_proc_type_detect(void)
>  	enum rte_proc_type_t ptype = RTE_PROC_PRIMARY;
>  	const char *pathname = eal_runtime_config_path();
> 
> -	/* if we can open the file but not get a write-lock we are a secondary
> -	 * process. NOTE: if we get a file handle back, we keep that open
> -	 * and don't close it to prevent a race condition between multiple
> opens */
> -	if (((mem_cfg_fd = open(pathname, O_RDWR)) >= 0) &&
> -			(fcntl(mem_cfg_fd, F_SETLK, &wr_lock) < 0))
> -		ptype = RTE_PROC_SECONDARY;
> +	/* if there no shared config, there can be no secondary processes */
> +	if (!internal_config.no_shconf) {
> +		/* if we can open the file but not get a write-lock we are a
> +		 * secondary process. NOTE: if we get a file handle back, we
> +		 * keep that open and don't close it to prevent a race
> condition
> +		 * between multiple opens.
> +		 */
> +		if (((mem_cfg_fd = open(pathname, O_RDWR)) >= 0) &&
> +				(fcntl(mem_cfg_fd, F_SETLK, &wr_lock) < 0))
> +			ptype = RTE_PROC_SECONDARY;
> +	}
> 
>  	RTE_LOG(INFO, EAL, "Auto-detected process type: %s\n",
>  			ptype == RTE_PROC_PRIMARY ? "PRIMARY" :
> "SECONDARY");
> @@ -692,6 +697,14 @@ eal_parse_args(int argc, char **argv)
>  		}
>  	}
> 
> +	/* create runtime data directory */
> +	if (internal_config.no_shconf == 0 &&
> +			eal_create_runtime_dir() < 0) {
> +		RTE_LOG(ERR, EAL, "Cannot create runtime directory\n");
> +		ret = -1;
> +		goto out;
> +	}
> +
>  	if (eal_adjust_config(&internal_config) != 0) {
>  		ret = -1;
>  		goto out;
> @@ -831,14 +844,6 @@ rte_eal_init(int argc, char **argv)
>  		return -1;
>  	}
> 
> -	/* create runtime data directory */
> -	if (internal_config.no_shconf == 0 &&
> -			eal_create_runtime_dir() < 0) {
> -		rte_eal_init_alert("Cannot create runtime directory\n");
> -		rte_errno = EACCES;
> -		return -1;
> -	}
> -
>  	if (eal_plugins_init() < 0) {
>  		rte_eal_init_alert("Cannot init plugins\n");
>  		rte_errno = EINVAL;
> --
> 2.17.1
Thomas Monjalon July 19, 2018, 10:17 a.m. UTC | #2 
18/07/2018 17:58, Yao, Lei A:
> From: Burakov, Anatoly
> > 
> > Currently, we need runtime dir to put all of our runtime info in,
> > including the DPDK shared config. However, we use the shared
> > config to determine our proc type, and this happens earlier than
> > we actually create the config dir and thus can know where to
> > place the config file.
> > 
> > Fix this by moving runtime dir creation right after the EAL
> > arguments parsing, but before proc type autodetection. Also,
> > previously we were creating the config file unconditionally,
> > even if we specified no_shconf - fix it by only creating
> > the config file if no_shconf is not set.
> > 
> > Fixes: adf1d867361c ("eal: move runtime config file to new location")
> > 
> > Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> Tested-by: Yao Lei<lei.a.yao@intel.com>
> This patch pass the test with simple_mp sample. The secondary process
> can recognize itself as "secondary process" even use  "--proc-type=auto"
> parameter. 

Applied, thanks
diff mbox series

Patch

diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
index 73cdf07b8..7b399bc9d 100644
--- a/lib/librte_eal/bsdapp/eal/eal.c
+++ b/lib/librte_eal/bsdapp/eal/eal.c
@@ -286,12 +286,17 @@  eal_proc_type_detect(void)
 	enum rte_proc_type_t ptype = RTE_PROC_PRIMARY;
 	const char *pathname = eal_runtime_config_path();
 
-	/* if we can open the file but not get a write-lock we are a secondary
-	 * process. NOTE: if we get a file handle back, we keep that open
-	 * and don't close it to prevent a race condition between multiple opens */
-	if (((mem_cfg_fd = open(pathname, O_RDWR)) >= 0) &&
-			(fcntl(mem_cfg_fd, F_SETLK, &wr_lock) < 0))
-		ptype = RTE_PROC_SECONDARY;
+	/* if there no shared config, there can be no secondary processes */
+	if (!internal_config.no_shconf) {
+		/* if we can open the file but not get a write-lock we are a
+		 * secondary process. NOTE: if we get a file handle back, we
+		 * keep that open and don't close it to prevent a race condition
+		 * between multiple opens.
+		 */
+		if (((mem_cfg_fd = open(pathname, O_RDWR)) >= 0) &&
+				(fcntl(mem_cfg_fd, F_SETLK, &wr_lock) < 0))
+			ptype = RTE_PROC_SECONDARY;
+	}
 
 	RTE_LOG(INFO, EAL, "Auto-detected process type: %s\n",
 			ptype == RTE_PROC_PRIMARY ? "PRIMARY" : "SECONDARY");
@@ -468,6 +473,14 @@  eal_parse_args(int argc, char **argv)
 		}
 	}
 
+	/* create runtime data directory */
+	if (internal_config.no_shconf == 0 &&
+			eal_create_runtime_dir() < 0) {
+		RTE_LOG(ERR, EAL, "Cannot create runtime directory\n");
+		ret = -1;
+		goto out;
+	}
+
 	if (eal_adjust_config(&internal_config) != 0) {
 		ret = -1;
 		goto out;
@@ -600,14 +613,6 @@  rte_eal_init(int argc, char **argv)
 		return -1;
 	}
 
-	/* create runtime data directory */
-	if (internal_config.no_shconf == 0 &&
-			eal_create_runtime_dir() < 0) {
-		rte_eal_init_alert("Cannot create runtime directory\n");
-		rte_errno = EACCES;
-		return -1;
-	}
-
 	/* FreeBSD always uses legacy memory model */
 	internal_config.legacy_mem = true;
 
diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index d75ae9dae..d2d5aae80 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -344,12 +344,17 @@  eal_proc_type_detect(void)
 	enum rte_proc_type_t ptype = RTE_PROC_PRIMARY;
 	const char *pathname = eal_runtime_config_path();
 
-	/* if we can open the file but not get a write-lock we are a secondary
-	 * process. NOTE: if we get a file handle back, we keep that open
-	 * and don't close it to prevent a race condition between multiple opens */
-	if (((mem_cfg_fd = open(pathname, O_RDWR)) >= 0) &&
-			(fcntl(mem_cfg_fd, F_SETLK, &wr_lock) < 0))
-		ptype = RTE_PROC_SECONDARY;
+	/* if there no shared config, there can be no secondary processes */
+	if (!internal_config.no_shconf) {
+		/* if we can open the file but not get a write-lock we are a
+		 * secondary process. NOTE: if we get a file handle back, we
+		 * keep that open and don't close it to prevent a race condition
+		 * between multiple opens.
+		 */
+		if (((mem_cfg_fd = open(pathname, O_RDWR)) >= 0) &&
+				(fcntl(mem_cfg_fd, F_SETLK, &wr_lock) < 0))
+			ptype = RTE_PROC_SECONDARY;
+	}
 
 	RTE_LOG(INFO, EAL, "Auto-detected process type: %s\n",
 			ptype == RTE_PROC_PRIMARY ? "PRIMARY" : "SECONDARY");
@@ -692,6 +697,14 @@  eal_parse_args(int argc, char **argv)
 		}
 	}
 
+	/* create runtime data directory */
+	if (internal_config.no_shconf == 0 &&
+			eal_create_runtime_dir() < 0) {
+		RTE_LOG(ERR, EAL, "Cannot create runtime directory\n");
+		ret = -1;
+		goto out;
+	}
+
 	if (eal_adjust_config(&internal_config) != 0) {
 		ret = -1;
 		goto out;
@@ -831,14 +844,6 @@  rte_eal_init(int argc, char **argv)
 		return -1;
 	}
 
-	/* create runtime data directory */
-	if (internal_config.no_shconf == 0 &&
-			eal_create_runtime_dir() < 0) {
-		rte_eal_init_alert("Cannot create runtime directory\n");
-		rte_errno = EACCES;
-		return -1;
-	}
-
 	if (eal_plugins_init() < 0) {
 		rte_eal_init_alert("Cannot init plugins\n");
 		rte_errno = EINVAL;