[dpdk-dev] [PATCH] timer: allow first subsystem init from secondary

[dpdk-dev] [PATCH] timer: allow first subsystem init from secondary

[Erik Gabriel Carrillo]

Since memzones can be reserved from secondary processes as well as
primary processes, if the first call to the timer subsystem init
function occurs in a secondary process, we should allow it to succeed.

Fixes: c0749f7096c7 ("timer: allow management in shared memory")

Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
---
 lib/librte_timer/rte_timer.c | 52 +++++++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 25 deletions(-)

diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
index 9f2e921..c0f5b87 100644
--- a/lib/librte_timer/rte_timer.c
+++ b/lib/librte_timer/rte_timer.c
@@ -25,6 +25,7 @@
 #include <rte_memzone.h>
 #include <rte_malloc.h>
 #include <rte_compat.h>
+#include <rte_errno.h>
 
 #include "rte_timer.h"
 
@@ -155,40 +156,41 @@ rte_timer_subsystem_init_v1905(void)
 	struct rte_timer_data *data;
 	int i, lcore_id;
 	static const char *mz_name = "rte_timer_mz";
+	const size_t data_arr_size =
+				RTE_MAX_DATA_ELS * sizeof(*rte_timer_data_arr);
+	bool do_full_init;
 
 	if (rte_timer_subsystem_initialized)
 		return -EALREADY;
 
-	if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
-		mz = rte_memzone_lookup(mz_name);
-		if (mz == NULL)
-			return -EEXIST;
+lookup:
+	mz = rte_memzone_lookup(mz_name);
+	if (mz == NULL) {
+		mz = rte_memzone_reserve_aligned(mz_name, data_arr_size,
+				SOCKET_ID_ANY, 0, RTE_CACHE_LINE_SIZE);
+		if (mz == NULL) {
+			if (rte_errno == EEXIST)
+				goto lookup;
 
-		rte_timer_data_arr = mz->addr;
-
-		rte_timer_data_arr[default_data_id].internal_flags |=
-			FL_ALLOCATED;
-
-		rte_timer_subsystem_initialized = 1;
+			return -ENOMEM;
+		}
 
-		return 0;
+		do_full_init = true;
 	}
 
-	mz = rte_memzone_reserve_aligned(mz_name,
-			RTE_MAX_DATA_ELS * sizeof(*rte_timer_data_arr),
-			SOCKET_ID_ANY, 0, RTE_CACHE_LINE_SIZE);
-	if (mz == NULL)
-		return -ENOMEM;
-
 	rte_timer_data_arr = mz->addr;
 
-	for (i = 0; i < RTE_MAX_DATA_ELS; i++) {
-		data = &rte_timer_data_arr[i];
+	if (do_full_init) {
+		for (i = 0; i < RTE_MAX_DATA_ELS; i++) {
+			data = &rte_timer_data_arr[i];
 
-		for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
-			rte_spinlock_init(
-				&data->priv_timer[lcore_id].list_lock);
-			data->priv_timer[lcore_id].prev_lcore = lcore_id;
+			for (lcore_id = 0; lcore_id < RTE_MAX_LCORE;
+			     lcore_id++) {
+				rte_spinlock_init(
+					&data->priv_timer[lcore_id].list_lock);
+				data->priv_timer[lcore_id].prev_lcore =
+					lcore_id;
+			}
 		}
 	}
 
@@ -205,8 +207,8 @@ BIND_DEFAULT_SYMBOL(rte_timer_subsystem_init, _v1905, 19.05);
 void __rte_experimental
 rte_timer_subsystem_finalize(void)
 {
-	if (rte_timer_data_arr)
-		rte_free(rte_timer_data_arr);
+	if (!rte_timer_subsystem_initialized)
+		return;
 
 	rte_timer_subsystem_initialized = 0;
 }
-- 
2.6.4

[dpdk-dev] [PATCH] timer: allow first subsystem init from secondary

[Erik Gabriel Carrillo]

Since memzones can be reserved from secondary processes as well as
primary processes, if the first call to the timer subsystem init
function occurs in a secondary process, we should allow it to succeed.

Fixes: c0749f7096c7 ("timer: allow management in shared memory")

Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
---
 lib/librte_timer/rte_timer.c | 52 +++++++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 25 deletions(-)

diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
index 9f2e921..c0f5b87 100644
--- a/lib/librte_timer/rte_timer.c
+++ b/lib/librte_timer/rte_timer.c
@@ -25,6 +25,7 @@
 #include <rte_memzone.h>
 #include <rte_malloc.h>
 #include <rte_compat.h>
+#include <rte_errno.h>
 
 #include "rte_timer.h"
 
@@ -155,40 +156,41 @@ rte_timer_subsystem_init_v1905(void)
 	struct rte_timer_data *data;
 	int i, lcore_id;
 	static const char *mz_name = "rte_timer_mz";
+	const size_t data_arr_size =
+				RTE_MAX_DATA_ELS * sizeof(*rte_timer_data_arr);
+	bool do_full_init;
 
 	if (rte_timer_subsystem_initialized)
 		return -EALREADY;
 
-	if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
-		mz = rte_memzone_lookup(mz_name);
-		if (mz == NULL)
-			return -EEXIST;
+lookup:
+	mz = rte_memzone_lookup(mz_name);
+	if (mz == NULL) {
+		mz = rte_memzone_reserve_aligned(mz_name, data_arr_size,
+				SOCKET_ID_ANY, 0, RTE_CACHE_LINE_SIZE);
+		if (mz == NULL) {
+			if (rte_errno == EEXIST)
+				goto lookup;
 
-		rte_timer_data_arr = mz->addr;
-
-		rte_timer_data_arr[default_data_id].internal_flags |=
-			FL_ALLOCATED;
-
-		rte_timer_subsystem_initialized = 1;
+			return -ENOMEM;
+		}
 
-		return 0;
+		do_full_init = true;
 	}
 
-	mz = rte_memzone_reserve_aligned(mz_name,
-			RTE_MAX_DATA_ELS * sizeof(*rte_timer_data_arr),
-			SOCKET_ID_ANY, 0, RTE_CACHE_LINE_SIZE);
-	if (mz == NULL)
-		return -ENOMEM;
-
 	rte_timer_data_arr = mz->addr;
 
-	for (i = 0; i < RTE_MAX_DATA_ELS; i++) {
-		data = &rte_timer_data_arr[i];
+	if (do_full_init) {
+		for (i = 0; i < RTE_MAX_DATA_ELS; i++) {
+			data = &rte_timer_data_arr[i];
 
-		for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
-			rte_spinlock_init(
-				&data->priv_timer[lcore_id].list_lock);
-			data->priv_timer[lcore_id].prev_lcore = lcore_id;
+			for (lcore_id = 0; lcore_id < RTE_MAX_LCORE;
+			     lcore_id++) {
+				rte_spinlock_init(
+					&data->priv_timer[lcore_id].list_lock);
+				data->priv_timer[lcore_id].prev_lcore =
+					lcore_id;
+			}
 		}
 	}
 
@@ -205,8 +207,8 @@ BIND_DEFAULT_SYMBOL(rte_timer_subsystem_init, _v1905, 19.05);
 void __rte_experimental
 rte_timer_subsystem_finalize(void)
 {
-	if (rte_timer_data_arr)
-		rte_free(rte_timer_data_arr);
+	if (!rte_timer_subsystem_initialized)
+		return;
 
 	rte_timer_subsystem_initialized = 0;
 }
-- 
2.6.4

Re: [dpdk-dev] [PATCH] timer: allow first subsystem init from secondary

[Burakov, Anatoly]

On 08-May-19 11:17 PM, Erik Gabriel Carrillo wrote:
> Since memzones can be reserved from secondary processes as well as
> primary processes, if the first call to the timer subsystem init
> function occurs in a secondary process, we should allow it to succeed.
> 
> Fixes: c0749f7096c7 ("timer: allow management in shared memory")
> 
> Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> ---
>   lib/librte_timer/rte_timer.c | 52 +++++++++++++++++++++++---------------------
>   1 file changed, 27 insertions(+), 25 deletions(-)
> 
> diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
> index 9f2e921..c0f5b87 100644
> --- a/lib/librte_timer/rte_timer.c
> +++ b/lib/librte_timer/rte_timer.c
> @@ -25,6 +25,7 @@
>   #include <rte_memzone.h>
>   #include <rte_malloc.h>
>   #include <rte_compat.h>
> +#include <rte_errno.h>
>   
>   #include "rte_timer.h"
>   
> @@ -155,40 +156,41 @@ rte_timer_subsystem_init_v1905(void)
>   	struct rte_timer_data *data;
>   	int i, lcore_id;
>   	static const char *mz_name = "rte_timer_mz";
> +	const size_t data_arr_size =
> +				RTE_MAX_DATA_ELS * sizeof(*rte_timer_data_arr);
> +	bool do_full_init;
>   
>   	if (rte_timer_subsystem_initialized)
>   		return -EALREADY;
>   
> -	if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
> -		mz = rte_memzone_lookup(mz_name);
> -		if (mz == NULL)
> -			return -EEXIST;
> +lookup:
> +	mz = rte_memzone_lookup(mz_name);

Wouldn't it be better to attempt to reserve outright, and then do a 
lookup on EEXIST?

> +	if (mz == NULL) {
> +		mz = rte_memzone_reserve_aligned(mz_name, data_arr_size,
> +				SOCKET_ID_ANY, 0, RTE_CACHE_LINE_SIZE);
> +		if (mz == NULL) {
> +			if (rte_errno == EEXIST)
> +				goto lookup;
>   


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH] timer: allow first subsystem init from secondary

[Burakov, Anatoly]

On 08-May-19 11:17 PM, Erik Gabriel Carrillo wrote:
> Since memzones can be reserved from secondary processes as well as
> primary processes, if the first call to the timer subsystem init
> function occurs in a secondary process, we should allow it to succeed.
> 
> Fixes: c0749f7096c7 ("timer: allow management in shared memory")
> 
> Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> ---
>   lib/librte_timer/rte_timer.c | 52 +++++++++++++++++++++++---------------------
>   1 file changed, 27 insertions(+), 25 deletions(-)
> 
> diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
> index 9f2e921..c0f5b87 100644
> --- a/lib/librte_timer/rte_timer.c
> +++ b/lib/librte_timer/rte_timer.c
> @@ -25,6 +25,7 @@
>   #include <rte_memzone.h>
>   #include <rte_malloc.h>
>   #include <rte_compat.h>
> +#include <rte_errno.h>
>   
>   #include "rte_timer.h"
>   
> @@ -155,40 +156,41 @@ rte_timer_subsystem_init_v1905(void)
>   	struct rte_timer_data *data;
>   	int i, lcore_id;
>   	static const char *mz_name = "rte_timer_mz";
> +	const size_t data_arr_size =
> +				RTE_MAX_DATA_ELS * sizeof(*rte_timer_data_arr);
> +	bool do_full_init;
>   
>   	if (rte_timer_subsystem_initialized)
>   		return -EALREADY;
>   
> -	if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
> -		mz = rte_memzone_lookup(mz_name);
> -		if (mz == NULL)
> -			return -EEXIST;
> +lookup:
> +	mz = rte_memzone_lookup(mz_name);

Wouldn't it be better to attempt to reserve outright, and then do a 
lookup on EEXIST?

> +	if (mz == NULL) {
> +		mz = rte_memzone_reserve_aligned(mz_name, data_arr_size,
> +				SOCKET_ID_ANY, 0, RTE_CACHE_LINE_SIZE);
> +		if (mz == NULL) {
> +			if (rte_errno == EEXIST)
> +				goto lookup;
>   


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH] timer: allow first subsystem init from secondary

[Carrillo, Erik G]

<...snipped...>

> >
> > @@ -155,40 +156,41 @@ rte_timer_subsystem_init_v1905(void)
> >   	struct rte_timer_data *data;
> >   	int i, lcore_id;
> >   	static const char *mz_name = "rte_timer_mz";
> > +	const size_t data_arr_size =
> > +				RTE_MAX_DATA_ELS *
> sizeof(*rte_timer_data_arr);
> > +	bool do_full_init;
> >
> >   	if (rte_timer_subsystem_initialized)
> >   		return -EALREADY;
> >
> > -	if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
> > -		mz = rte_memzone_lookup(mz_name);
> > -		if (mz == NULL)
> > -			return -EEXIST;
> > +lookup:
> > +	mz = rte_memzone_lookup(mz_name);
> 
> Wouldn't it be better to attempt to reserve outright, and then do a lookup on
> EEXIST?

It's probably the expected order;  I've made the change and submitted v2.

Thanks for the review,
Erik

> 
> > +	if (mz == NULL) {
> > +		mz = rte_memzone_reserve_aligned(mz_name,
> data_arr_size,
> > +				SOCKET_ID_ANY, 0, RTE_CACHE_LINE_SIZE);
> > +		if (mz == NULL) {
> > +			if (rte_errno == EEXIST)
> > +				goto lookup;
> >
> 
> 
> --
> Thanks,
> Anatoly

Re: [dpdk-dev] [PATCH] timer: allow first subsystem init from secondary

[Carrillo, Erik G]

<...snipped...>

> >
> > @@ -155,40 +156,41 @@ rte_timer_subsystem_init_v1905(void)
> >   	struct rte_timer_data *data;
> >   	int i, lcore_id;
> >   	static const char *mz_name = "rte_timer_mz";
> > +	const size_t data_arr_size =
> > +				RTE_MAX_DATA_ELS *
> sizeof(*rte_timer_data_arr);
> > +	bool do_full_init;
> >
> >   	if (rte_timer_subsystem_initialized)
> >   		return -EALREADY;
> >
> > -	if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
> > -		mz = rte_memzone_lookup(mz_name);
> > -		if (mz == NULL)
> > -			return -EEXIST;
> > +lookup:
> > +	mz = rte_memzone_lookup(mz_name);
> 
> Wouldn't it be better to attempt to reserve outright, and then do a lookup on
> EEXIST?

It's probably the expected order;  I've made the change and submitted v2.

Thanks for the review,
Erik

> 
> > +	if (mz == NULL) {
> > +		mz = rte_memzone_reserve_aligned(mz_name,
> data_arr_size,
> > +				SOCKET_ID_ANY, 0, RTE_CACHE_LINE_SIZE);
> > +		if (mz == NULL) {
> > +			if (rte_errno == EEXIST)
> > +				goto lookup;
> >
> 
> 
> --
> Thanks,
> Anatoly

[dpdk-dev] [PATCH v2] timer: allow first subsystem init from secondary

[Erik Gabriel Carrillo]

Since memzones can be reserved from secondary processes as well as
primary processes, if the first call to the timer subsystem init
function occurs in a secondary process, we should allow it to succeed.

Fixes: c0749f7096c7 ("timer: allow management in shared memory")

Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
---
changes in v2:
 - Try to do reserve first, then do lookup on EEXIST (Anatoly)
 - Fix uninitialized variable from first version

 lib/librte_timer/rte_timer.c | 56 +++++++++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 27 deletions(-)

diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
index 9f2e921..dd79539 100644
--- a/lib/librte_timer/rte_timer.c
+++ b/lib/librte_timer/rte_timer.c
@@ -25,6 +25,7 @@
 #include <rte_memzone.h>
 #include <rte_malloc.h>
 #include <rte_compat.h>
+#include <rte_errno.h>
 
 #include "rte_timer.h"
 
@@ -155,40 +156,41 @@ rte_timer_subsystem_init_v1905(void)
 	struct rte_timer_data *data;
 	int i, lcore_id;
 	static const char *mz_name = "rte_timer_mz";
+	const size_t data_arr_size =
+				RTE_MAX_DATA_ELS * sizeof(*rte_timer_data_arr);
+	bool do_full_init = true;
 
 	if (rte_timer_subsystem_initialized)
 		return -EALREADY;
 
-	if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
-		mz = rte_memzone_lookup(mz_name);
-		if (mz == NULL)
-			return -EEXIST;
-
-		rte_timer_data_arr = mz->addr;
-
-		rte_timer_data_arr[default_data_id].internal_flags |=
-			FL_ALLOCATED;
-
-		rte_timer_subsystem_initialized = 1;
-
-		return 0;
+reserve:
+	rte_errno = 0;
+	mz = rte_memzone_reserve_aligned(mz_name, data_arr_size, SOCKET_ID_ANY,
+					 0, RTE_CACHE_LINE_SIZE);
+	if (mz == NULL) {
+		if (rte_errno == EEXIST) {
+			mz = rte_memzone_lookup(mz_name);
+			if (mz == NULL)
+				goto reserve;
+
+			do_full_init = false;
+		} else
+			return -ENOMEM;
 	}
 
-	mz = rte_memzone_reserve_aligned(mz_name,
-			RTE_MAX_DATA_ELS * sizeof(*rte_timer_data_arr),
-			SOCKET_ID_ANY, 0, RTE_CACHE_LINE_SIZE);
-	if (mz == NULL)
-		return -ENOMEM;
-
 	rte_timer_data_arr = mz->addr;
 
-	for (i = 0; i < RTE_MAX_DATA_ELS; i++) {
-		data = &rte_timer_data_arr[i];
+	if (do_full_init) {
+		for (i = 0; i < RTE_MAX_DATA_ELS; i++) {
+			data = &rte_timer_data_arr[i];
 
-		for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
-			rte_spinlock_init(
-				&data->priv_timer[lcore_id].list_lock);
-			data->priv_timer[lcore_id].prev_lcore = lcore_id;
+			for (lcore_id = 0; lcore_id < RTE_MAX_LCORE;
+			     lcore_id++) {
+				rte_spinlock_init(
+					&data->priv_timer[lcore_id].list_lock);
+				data->priv_timer[lcore_id].prev_lcore =
+					lcore_id;
+			}
 		}
 	}
 
@@ -205,8 +207,8 @@ BIND_DEFAULT_SYMBOL(rte_timer_subsystem_init, _v1905, 19.05);
 void __rte_experimental
 rte_timer_subsystem_finalize(void)
 {
-	if (rte_timer_data_arr)
-		rte_free(rte_timer_data_arr);
+	if (!rte_timer_subsystem_initialized)
+		return;
 
 	rte_timer_subsystem_initialized = 0;
 }
-- 
2.6.4

[dpdk-dev] [PATCH v2] timer: allow first subsystem init from secondary

[Erik Gabriel Carrillo]

Since memzones can be reserved from secondary processes as well as
primary processes, if the first call to the timer subsystem init
function occurs in a secondary process, we should allow it to succeed.

Fixes: c0749f7096c7 ("timer: allow management in shared memory")

Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
---
changes in v2:
 - Try to do reserve first, then do lookup on EEXIST (Anatoly)
 - Fix uninitialized variable from first version

 lib/librte_timer/rte_timer.c | 56 +++++++++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 27 deletions(-)

diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
index 9f2e921..dd79539 100644
--- a/lib/librte_timer/rte_timer.c
+++ b/lib/librte_timer/rte_timer.c
@@ -25,6 +25,7 @@
 #include <rte_memzone.h>
 #include <rte_malloc.h>
 #include <rte_compat.h>
+#include <rte_errno.h>
 
 #include "rte_timer.h"
 
@@ -155,40 +156,41 @@ rte_timer_subsystem_init_v1905(void)
 	struct rte_timer_data *data;
 	int i, lcore_id;
 	static const char *mz_name = "rte_timer_mz";
+	const size_t data_arr_size =
+				RTE_MAX_DATA_ELS * sizeof(*rte_timer_data_arr);
+	bool do_full_init = true;
 
 	if (rte_timer_subsystem_initialized)
 		return -EALREADY;
 
-	if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
-		mz = rte_memzone_lookup(mz_name);
-		if (mz == NULL)
-			return -EEXIST;
-
-		rte_timer_data_arr = mz->addr;
-
-		rte_timer_data_arr[default_data_id].internal_flags |=
-			FL_ALLOCATED;
-
-		rte_timer_subsystem_initialized = 1;
-
-		return 0;
+reserve:
+	rte_errno = 0;
+	mz = rte_memzone_reserve_aligned(mz_name, data_arr_size, SOCKET_ID_ANY,
+					 0, RTE_CACHE_LINE_SIZE);
+	if (mz == NULL) {
+		if (rte_errno == EEXIST) {
+			mz = rte_memzone_lookup(mz_name);
+			if (mz == NULL)
+				goto reserve;
+
+			do_full_init = false;
+		} else
+			return -ENOMEM;
 	}
 
-	mz = rte_memzone_reserve_aligned(mz_name,
-			RTE_MAX_DATA_ELS * sizeof(*rte_timer_data_arr),
-			SOCKET_ID_ANY, 0, RTE_CACHE_LINE_SIZE);
-	if (mz == NULL)
-		return -ENOMEM;
-
 	rte_timer_data_arr = mz->addr;
 
-	for (i = 0; i < RTE_MAX_DATA_ELS; i++) {
-		data = &rte_timer_data_arr[i];
+	if (do_full_init) {
+		for (i = 0; i < RTE_MAX_DATA_ELS; i++) {
+			data = &rte_timer_data_arr[i];
 
-		for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
-			rte_spinlock_init(
-				&data->priv_timer[lcore_id].list_lock);
-			data->priv_timer[lcore_id].prev_lcore = lcore_id;
+			for (lcore_id = 0; lcore_id < RTE_MAX_LCORE;
+			     lcore_id++) {
+				rte_spinlock_init(
+					&data->priv_timer[lcore_id].list_lock);
+				data->priv_timer[lcore_id].prev_lcore =
+					lcore_id;
+			}
 		}
 	}
 
@@ -205,8 +207,8 @@ BIND_DEFAULT_SYMBOL(rte_timer_subsystem_init, _v1905, 19.05);
 void __rte_experimental
 rte_timer_subsystem_finalize(void)
 {
-	if (rte_timer_data_arr)
-		rte_free(rte_timer_data_arr);
+	if (!rte_timer_subsystem_initialized)
+		return;
 
 	rte_timer_subsystem_initialized = 0;
 }
-- 
2.6.4

Re: [dpdk-dev] [PATCH v2] timer: allow first subsystem init from secondary

[Thomas Monjalon]

09/05/2019 21:39, Erik Gabriel Carrillo:
> Since memzones can be reserved from secondary processes as well as
> primary processes, if the first call to the timer subsystem init
> function occurs in a secondary process, we should allow it to succeed.
> 
> Fixes: c0749f7096c7 ("timer: allow management in shared memory")
> 
> Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>

I think this patch is too big for -rc4.
And it doesn't look so critical.
Do you agree to wait 19.08?

Re: [dpdk-dev] [PATCH v2] timer: allow first subsystem init from secondary

[Thomas Monjalon]

09/05/2019 21:39, Erik Gabriel Carrillo:
> Since memzones can be reserved from secondary processes as well as
> primary processes, if the first call to the timer subsystem init
> function occurs in a secondary process, we should allow it to succeed.
> 
> Fixes: c0749f7096c7 ("timer: allow management in shared memory")
> 
> Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>

I think this patch is too big for -rc4.
And it doesn't look so critical.
Do you agree to wait 19.08?

Re: [dpdk-dev] [PATCH v2] timer: allow first subsystem init from secondary

[Carrillo, Erik G]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, May 9, 2019 2:52 PM
> To: Carrillo, Erik G <erik.g.carrillo@intel.com>
> Cc: Burakov, Anatoly <anatoly.burakov@intel.com>; rsanford@akamai.com;
> dev@dpdk.org
> Subject: Re: [PATCH v2] timer: allow first subsystem init from secondary
> 
> 09/05/2019 21:39, Erik Gabriel Carrillo:
> > Since memzones can be reserved from secondary processes as well as
> > primary processes, if the first call to the timer subsystem init
> > function occurs in a secondary process, we should allow it to succeed.
> >
> > Fixes: c0749f7096c7 ("timer: allow management in shared memory")
> >
> > Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> 
> I think this patch is too big for -rc4.
> And it doesn't look so critical.
> Do you agree to wait 19.08?
> 

The very last hunk of the patch should at least be applied, as it fixes an issue in the finalize() function.  The rest of it is just to make sure the behavior is the same as the prior release with respect to the secondary.

I'd prefer if the whole patch was applied, but I can break out the last hunk for a very small patch if that's what you think we should do.

Regards,
Erik

Re: [dpdk-dev] [PATCH v2] timer: allow first subsystem init from secondary

[Thomas Monjalon]

09/05/2019 22:08, Carrillo, Erik G:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 09/05/2019 21:39, Erik Gabriel Carrillo:
> > > Since memzones can be reserved from secondary processes as well as
> > > primary processes, if the first call to the timer subsystem init
> > > function occurs in a secondary process, we should allow it to succeed.
> > >
> > > Fixes: c0749f7096c7 ("timer: allow management in shared memory")
> > >
> > > Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> > 
> > I think this patch is too big for -rc4.
> > And it doesn't look so critical.
> > Do you agree to wait 19.08?
> > 
> 
> The very last hunk of the patch should at least be applied, as it fixes an issue in the finalize() function.  The rest of it is just to make sure the behavior is the same as the prior release with respect to the secondary.
> 
> I'd prefer if the whole patch was applied, but I can break out the last hunk for a very small patch if that's what you think we should do.

It's a pity it comes so late.

Can you tell how much you think it won't bring any regression?
Are you available everyday until Monday to fix it quickly
if something goes wrong?

Re: [dpdk-dev] [PATCH v2] timer: allow first subsystem init from secondary

[Thomas Monjalon]

09/05/2019 22:08, Carrillo, Erik G:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 09/05/2019 21:39, Erik Gabriel Carrillo:
> > > Since memzones can be reserved from secondary processes as well as
> > > primary processes, if the first call to the timer subsystem init
> > > function occurs in a secondary process, we should allow it to succeed.
> > >
> > > Fixes: c0749f7096c7 ("timer: allow management in shared memory")
> > >
> > > Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> > 
> > I think this patch is too big for -rc4.
> > And it doesn't look so critical.
> > Do you agree to wait 19.08?
> > 
> 
> The very last hunk of the patch should at least be applied, as it fixes an issue in the finalize() function.  The rest of it is just to make sure the behavior is the same as the prior release with respect to the secondary.
> 
> I'd prefer if the whole patch was applied, but I can break out the last hunk for a very small patch if that's what you think we should do.

It's a pity it comes so late.

Can you tell how much you think it won't bring any regression?
Are you available everyday until Monday to fix it quickly
if something goes wrong?

Re: [dpdk-dev] [PATCH v2] timer: allow first subsystem init from secondary

[Carrillo, Erik G]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, May 9, 2019 3:13 PM
> To: Carrillo, Erik G <erik.g.carrillo@intel.com>
> Cc: Burakov, Anatoly <anatoly.burakov@intel.com>; rsanford@akamai.com;
> dev@dpdk.org
> Subject: Re: [PATCH v2] timer: allow first subsystem init from secondary
> 
> 09/05/2019 22:08, Carrillo, Erik G:
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > 09/05/2019 21:39, Erik Gabriel Carrillo:
> > > > Since memzones can be reserved from secondary processes as well as
> > > > primary processes, if the first call to the timer subsystem init
> > > > function occurs in a secondary process, we should allow it to succeed.
> > > >
> > > > Fixes: c0749f7096c7 ("timer: allow management in shared memory")
> > > >
> > > > Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> > >
> > > I think this patch is too big for -rc4.
> > > And it doesn't look so critical.
> > > Do you agree to wait 19.08?
> > >
> >
> > The very last hunk of the patch should at least be applied, as it fixes an
> issue in the finalize() function.  The rest of it is just to make sure the behavior
> is the same as the prior release with respect to the secondary.
> >
> > I'd prefer if the whole patch was applied, but I can break out the last hunk
> for a very small patch if that's what you think we should do.
> 
> It's a pity it comes so late.
> 
> Can you tell how much you think it won't bring any regression?
> Are you available everyday until Monday to fix it quickly if something goes
> wrong?
> 
I'm fairly sure there won't be a regression; I just did some more testing on the secondary side.  But I will be available to fix it if something does go wrong.

Thanks,
Erik

Re: [dpdk-dev] [PATCH v2] timer: allow first subsystem init from secondary

[Carrillo, Erik G]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, May 9, 2019 3:13 PM
> To: Carrillo, Erik G <erik.g.carrillo@intel.com>
> Cc: Burakov, Anatoly <anatoly.burakov@intel.com>; rsanford@akamai.com;
> dev@dpdk.org
> Subject: Re: [PATCH v2] timer: allow first subsystem init from secondary
> 
> 09/05/2019 22:08, Carrillo, Erik G:
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > 09/05/2019 21:39, Erik Gabriel Carrillo:
> > > > Since memzones can be reserved from secondary processes as well as
> > > > primary processes, if the first call to the timer subsystem init
> > > > function occurs in a secondary process, we should allow it to succeed.
> > > >
> > > > Fixes: c0749f7096c7 ("timer: allow management in shared memory")
> > > >
> > > > Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> > >
> > > I think this patch is too big for -rc4.
> > > And it doesn't look so critical.
> > > Do you agree to wait 19.08?
> > >
> >
> > The very last hunk of the patch should at least be applied, as it fixes an
> issue in the finalize() function.  The rest of it is just to make sure the behavior
> is the same as the prior release with respect to the secondary.
> >
> > I'd prefer if the whole patch was applied, but I can break out the last hunk
> for a very small patch if that's what you think we should do.
> 
> It's a pity it comes so late.
> 
> Can you tell how much you think it won't bring any regression?
> Are you available everyday until Monday to fix it quickly if something goes
> wrong?
> 
I'm fairly sure there won't be a regression; I just did some more testing on the secondary side.  But I will be available to fix it if something does go wrong.

Thanks,
Erik

Re: [dpdk-dev] [PATCH v2] timer: allow first subsystem init from secondary

[Thomas Monjalon]

09/05/2019 23:19, Carrillo, Erik G:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 09/05/2019 22:08, Carrillo, Erik G:
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > 09/05/2019 21:39, Erik Gabriel Carrillo:
> > > > > Since memzones can be reserved from secondary processes as well as
> > > > > primary processes, if the first call to the timer subsystem init
> > > > > function occurs in a secondary process, we should allow it to succeed.
> > > > >
> > > > > Fixes: c0749f7096c7 ("timer: allow management in shared memory")
> > > > >
> > > > > Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> > > >
> > > > I think this patch is too big for -rc4.
> > > > And it doesn't look so critical.
> > > > Do you agree to wait 19.08?
> > > >
> > >
> > > The very last hunk of the patch should at least be applied, as it fixes an
> > issue in the finalize() function.  The rest of it is just to make sure the behavior
> > is the same as the prior release with respect to the secondary.
> > >
> > > I'd prefer if the whole patch was applied, but I can break out the last hunk
> > for a very small patch if that's what you think we should do.
> > 
> > It's a pity it comes so late.
> > 
> > Can you tell how much you think it won't bring any regression?
> > Are you available everyday until Monday to fix it quickly if something goes
> > wrong?
> > 
> I'm fairly sure there won't be a regression; I just did some more testing on the secondary side.  But I will be available to fix it if something does go wrong.

Applied, thanks

Re: [dpdk-dev] [PATCH v2] timer: allow first subsystem init from secondary

[Thomas Monjalon]

09/05/2019 23:19, Carrillo, Erik G:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 09/05/2019 22:08, Carrillo, Erik G:
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > 09/05/2019 21:39, Erik Gabriel Carrillo:
> > > > > Since memzones can be reserved from secondary processes as well as
> > > > > primary processes, if the first call to the timer subsystem init
> > > > > function occurs in a secondary process, we should allow it to succeed.
> > > > >
> > > > > Fixes: c0749f7096c7 ("timer: allow management in shared memory")
> > > > >
> > > > > Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> > > >
> > > > I think this patch is too big for -rc4.
> > > > And it doesn't look so critical.
> > > > Do you agree to wait 19.08?
> > > >
> > >
> > > The very last hunk of the patch should at least be applied, as it fixes an
> > issue in the finalize() function.  The rest of it is just to make sure the behavior
> > is the same as the prior release with respect to the secondary.
> > >
> > > I'd prefer if the whole patch was applied, but I can break out the last hunk
> > for a very small patch if that's what you think we should do.
> > 
> > It's a pity it comes so late.
> > 
> > Can you tell how much you think it won't bring any regression?
> > Are you available everyday until Monday to fix it quickly if something goes
> > wrong?
> > 
> I'm fairly sure there won't be a regression; I just did some more testing on the secondary side.  But I will be available to fix it if something does go wrong.

Applied, thanks