[dpdk-dev] [PATCH] timer: fix resource leak in finalize

[dpdk-dev] [PATCH] timer: fix resource leak in finalize

[Erik Gabriel Carrillo]

The finalize function should free the memzone created in the init
function, rather than freeing the allocation the memzone references,
otherwise a memzone descriptor can be leaked.

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 | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
index eb46009..fb7a87e 100644
--- a/lib/librte_timer/rte_timer.c
+++ b/lib/librte_timer/rte_timer.c
@@ -60,6 +60,7 @@ struct rte_timer_data {
 };
 
 #define RTE_MAX_DATA_ELS 64
+static const struct rte_memzone *rte_timer_data_mz;
 static struct rte_timer_data *rte_timer_data_arr;
 static const uint32_t default_data_id;
 static uint32_t rte_timer_subsystem_initialized;
@@ -164,6 +165,7 @@ rte_timer_subsystem_init_v1905(void)
 		if (mz == NULL)
 			return -EEXIST;
 
+		rte_timer_data_mz = mz;
 		rte_timer_data_arr = mz->addr;
 
 		rte_timer_data_arr[default_data_id].internal_flags |=
@@ -180,6 +182,7 @@ rte_timer_subsystem_init_v1905(void)
 	if (mz == NULL)
 		return -ENOMEM;
 
+	rte_timer_data_mz = mz;
 	rte_timer_data_arr = mz->addr;
 
 	for (i = 0; i < RTE_MAX_DATA_ELS; i++) {
@@ -205,8 +208,13 @@ 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_eal_process_type() != RTE_PROC_PRIMARY)
+		return;
+
+	if (!rte_timer_subsystem_initialized)
+		return;
+
+	rte_memzone_free(rte_timer_data_mz);
 
 	rte_timer_subsystem_initialized = 0;
 }
-- 
2.6.4

[dpdk-dev] [PATCH] timer: fix resource leak in finalize

[Erik Gabriel Carrillo]

The finalize function should free the memzone created in the init
function, rather than freeing the allocation the memzone references,
otherwise a memzone descriptor can be leaked.

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 | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
index eb46009..fb7a87e 100644
--- a/lib/librte_timer/rte_timer.c
+++ b/lib/librte_timer/rte_timer.c
@@ -60,6 +60,7 @@ struct rte_timer_data {
 };
 
 #define RTE_MAX_DATA_ELS 64
+static const struct rte_memzone *rte_timer_data_mz;
 static struct rte_timer_data *rte_timer_data_arr;
 static const uint32_t default_data_id;
 static uint32_t rte_timer_subsystem_initialized;
@@ -164,6 +165,7 @@ rte_timer_subsystem_init_v1905(void)
 		if (mz == NULL)
 			return -EEXIST;
 
+		rte_timer_data_mz = mz;
 		rte_timer_data_arr = mz->addr;
 
 		rte_timer_data_arr[default_data_id].internal_flags |=
@@ -180,6 +182,7 @@ rte_timer_subsystem_init_v1905(void)
 	if (mz == NULL)
 		return -ENOMEM;
 
+	rte_timer_data_mz = mz;
 	rte_timer_data_arr = mz->addr;
 
 	for (i = 0; i < RTE_MAX_DATA_ELS; i++) {
@@ -205,8 +208,13 @@ 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_eal_process_type() != RTE_PROC_PRIMARY)
+		return;
+
+	if (!rte_timer_subsystem_initialized)
+		return;
+
+	rte_memzone_free(rte_timer_data_mz);
 
 	rte_timer_subsystem_initialized = 0;
 }
-- 
2.6.4

Re: [dpdk-dev] [PATCH] timer: fix resource leak in finalize

[Burakov, Anatoly]

On 01-May-19 8:00 PM, Erik Gabriel Carrillo wrote:
> The finalize function should free the memzone created in the init
> function, rather than freeing the allocation the memzone references,
> otherwise a memzone descriptor can be leaked.
> 
> 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 | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
> index eb46009..fb7a87e 100644
> --- a/lib/librte_timer/rte_timer.c
> +++ b/lib/librte_timer/rte_timer.c
> @@ -60,6 +60,7 @@ struct rte_timer_data {
>   };
>   
>   #define RTE_MAX_DATA_ELS 64
> +static const struct rte_memzone *rte_timer_data_mz;
>   static struct rte_timer_data *rte_timer_data_arr;
>   static const uint32_t default_data_id;
>   static uint32_t rte_timer_subsystem_initialized;
> @@ -164,6 +165,7 @@ rte_timer_subsystem_init_v1905(void)
>   		if (mz == NULL)
>   			return -EEXIST;
>   
> +		rte_timer_data_mz = mz;
>   		rte_timer_data_arr = mz->addr;
>   
>   		rte_timer_data_arr[default_data_id].internal_flags |=
> @@ -180,6 +182,7 @@ rte_timer_subsystem_init_v1905(void)
>   	if (mz == NULL)
>   		return -ENOMEM;
>   
> +	rte_timer_data_mz = mz;
>   	rte_timer_data_arr = mz->addr;
>   
>   	for (i = 0; i < RTE_MAX_DATA_ELS; i++) {
> @@ -205,8 +208,13 @@ 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_eal_process_type() != RTE_PROC_PRIMARY)
> +		return;
> +
> +	if (!rte_timer_subsystem_initialized)
> +		return;
> +
> +	rte_memzone_free(rte_timer_data_mz);

The patch is a correct fix, but the whole idea of this looks dangerous 
to me.

If we exit the primary while secondaries are still running, wouldn't it 
basically pull out timer data from under secondaries' feet?

>   
>   	rte_timer_subsystem_initialized = 0;
>   }
> 


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH] timer: fix resource leak in finalize

[Burakov, Anatoly]

On 01-May-19 8:00 PM, Erik Gabriel Carrillo wrote:
> The finalize function should free the memzone created in the init
> function, rather than freeing the allocation the memzone references,
> otherwise a memzone descriptor can be leaked.
> 
> 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 | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
> index eb46009..fb7a87e 100644
> --- a/lib/librte_timer/rte_timer.c
> +++ b/lib/librte_timer/rte_timer.c
> @@ -60,6 +60,7 @@ struct rte_timer_data {
>   };
>   
>   #define RTE_MAX_DATA_ELS 64
> +static const struct rte_memzone *rte_timer_data_mz;
>   static struct rte_timer_data *rte_timer_data_arr;
>   static const uint32_t default_data_id;
>   static uint32_t rte_timer_subsystem_initialized;
> @@ -164,6 +165,7 @@ rte_timer_subsystem_init_v1905(void)
>   		if (mz == NULL)
>   			return -EEXIST;
>   
> +		rte_timer_data_mz = mz;
>   		rte_timer_data_arr = mz->addr;
>   
>   		rte_timer_data_arr[default_data_id].internal_flags |=
> @@ -180,6 +182,7 @@ rte_timer_subsystem_init_v1905(void)
>   	if (mz == NULL)
>   		return -ENOMEM;
>   
> +	rte_timer_data_mz = mz;
>   	rte_timer_data_arr = mz->addr;
>   
>   	for (i = 0; i < RTE_MAX_DATA_ELS; i++) {
> @@ -205,8 +208,13 @@ 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_eal_process_type() != RTE_PROC_PRIMARY)
> +		return;
> +
> +	if (!rte_timer_subsystem_initialized)
> +		return;
> +
> +	rte_memzone_free(rte_timer_data_mz);

The patch is a correct fix, but the whole idea of this looks dangerous 
to me.

If we exit the primary while secondaries are still running, wouldn't it 
basically pull out timer data from under secondaries' feet?

>   
>   	rte_timer_subsystem_initialized = 0;
>   }
> 


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH] timer: fix resource leak in finalize

[Carrillo, Erik G]

> -----Original Message-----
> From: Burakov, Anatoly
> Sent: Thursday, May 2, 2019 4:18 AM
> To: Carrillo, Erik G <erik.g.carrillo@intel.com>; rsanford@akamai.com;
> thomas@monjalon.net
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] timer: fix resource leak in finalize
> 
> On 01-May-19 8:00 PM, Erik Gabriel Carrillo wrote:
> > The finalize function should free the memzone created in the init
> > function, rather than freeing the allocation the memzone references,
> > otherwise a memzone descriptor can be leaked.
> >
> > 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 | 12 ++++++++++--
> >   1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/librte_timer/rte_timer.c
> > b/lib/librte_timer/rte_timer.c index eb46009..fb7a87e 100644
> > --- a/lib/librte_timer/rte_timer.c
> > +++ b/lib/librte_timer/rte_timer.c
> > @@ -60,6 +60,7 @@ struct rte_timer_data {
> >   };
> >
> >   #define RTE_MAX_DATA_ELS 64
> > +static const struct rte_memzone *rte_timer_data_mz;
> >   static struct rte_timer_data *rte_timer_data_arr;
> >   static const uint32_t default_data_id;
> >   static uint32_t rte_timer_subsystem_initialized; @@ -164,6 +165,7 @@
> > rte_timer_subsystem_init_v1905(void)
> >   		if (mz == NULL)
> >   			return -EEXIST;
> >
> > +		rte_timer_data_mz = mz;
> >   		rte_timer_data_arr = mz->addr;
> >
> >   		rte_timer_data_arr[default_data_id].internal_flags |= @@ -
> 180,6
> > +182,7 @@ rte_timer_subsystem_init_v1905(void)
> >   	if (mz == NULL)
> >   		return -ENOMEM;
> >
> > +	rte_timer_data_mz = mz;
> >   	rte_timer_data_arr = mz->addr;
> >
> >   	for (i = 0; i < RTE_MAX_DATA_ELS; i++) { @@ -205,8 +208,13 @@
> > 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_eal_process_type() != RTE_PROC_PRIMARY)
> > +		return;
> > +
> > +	if (!rte_timer_subsystem_initialized)
> > +		return;
> > +
> > +	rte_memzone_free(rte_timer_data_mz);
> 
> The patch is a correct fix, but the whole idea of this looks dangerous to me.
> 
> If we exit the primary while secondaries are still running, wouldn't it basically
> pull out timer data from under secondaries' feet?
> 

Ah yes - that’s right.  Perhaps it would be better to maintain a reference count of some sort such that the last process to exit could cause the memzone_free.

Thanks,
Erik

> >
> >   	rte_timer_subsystem_initialized = 0;
> >   }
> >
> 
> 
> --
> Thanks,
> Anatoly

Re: [dpdk-dev] [PATCH] timer: fix resource leak in finalize

[Carrillo, Erik G]

> -----Original Message-----
> From: Burakov, Anatoly
> Sent: Thursday, May 2, 2019 4:18 AM
> To: Carrillo, Erik G <erik.g.carrillo@intel.com>; rsanford@akamai.com;
> thomas@monjalon.net
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] timer: fix resource leak in finalize
> 
> On 01-May-19 8:00 PM, Erik Gabriel Carrillo wrote:
> > The finalize function should free the memzone created in the init
> > function, rather than freeing the allocation the memzone references,
> > otherwise a memzone descriptor can be leaked.
> >
> > 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 | 12 ++++++++++--
> >   1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/librte_timer/rte_timer.c
> > b/lib/librte_timer/rte_timer.c index eb46009..fb7a87e 100644
> > --- a/lib/librte_timer/rte_timer.c
> > +++ b/lib/librte_timer/rte_timer.c
> > @@ -60,6 +60,7 @@ struct rte_timer_data {
> >   };
> >
> >   #define RTE_MAX_DATA_ELS 64
> > +static const struct rte_memzone *rte_timer_data_mz;
> >   static struct rte_timer_data *rte_timer_data_arr;
> >   static const uint32_t default_data_id;
> >   static uint32_t rte_timer_subsystem_initialized; @@ -164,6 +165,7 @@
> > rte_timer_subsystem_init_v1905(void)
> >   		if (mz == NULL)
> >   			return -EEXIST;
> >
> > +		rte_timer_data_mz = mz;
> >   		rte_timer_data_arr = mz->addr;
> >
> >   		rte_timer_data_arr[default_data_id].internal_flags |= @@ -
> 180,6
> > +182,7 @@ rte_timer_subsystem_init_v1905(void)
> >   	if (mz == NULL)
> >   		return -ENOMEM;
> >
> > +	rte_timer_data_mz = mz;
> >   	rte_timer_data_arr = mz->addr;
> >
> >   	for (i = 0; i < RTE_MAX_DATA_ELS; i++) { @@ -205,8 +208,13 @@
> > 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_eal_process_type() != RTE_PROC_PRIMARY)
> > +		return;
> > +
> > +	if (!rte_timer_subsystem_initialized)
> > +		return;
> > +
> > +	rte_memzone_free(rte_timer_data_mz);
> 
> The patch is a correct fix, but the whole idea of this looks dangerous to me.
> 
> If we exit the primary while secondaries are still running, wouldn't it basically
> pull out timer data from under secondaries' feet?
> 

Ah yes - that’s right.  Perhaps it would be better to maintain a reference count of some sort such that the last process to exit could cause the memzone_free.

Thanks,
Erik

> >
> >   	rte_timer_subsystem_initialized = 0;
> >   }
> >
> 
> 
> --
> Thanks,
> Anatoly

Re: [dpdk-dev] [PATCH] timer: fix resource leak in finalize

[Burakov, Anatoly]

On 02-May-19 1:19 PM, Carrillo, Erik G wrote:
> 
> 
>> -----Original Message-----
>> From: Burakov, Anatoly
>> Sent: Thursday, May 2, 2019 4:18 AM
>> To: Carrillo, Erik G <erik.g.carrillo@intel.com>; rsanford@akamai.com;
>> thomas@monjalon.net
>> Cc: dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH] timer: fix resource leak in finalize
>>
>> On 01-May-19 8:00 PM, Erik Gabriel Carrillo wrote:
>>> The finalize function should free the memzone created in the init
>>> function, rather than freeing the allocation the memzone references,
>>> otherwise a memzone descriptor can be leaked.
>>>
>>> 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 | 12 ++++++++++--
>>>    1 file changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/librte_timer/rte_timer.c
>>> b/lib/librte_timer/rte_timer.c index eb46009..fb7a87e 100644
>>> --- a/lib/librte_timer/rte_timer.c
>>> +++ b/lib/librte_timer/rte_timer.c
>>> @@ -60,6 +60,7 @@ struct rte_timer_data {
>>>    };
>>>
>>>    #define RTE_MAX_DATA_ELS 64
>>> +static const struct rte_memzone *rte_timer_data_mz;
>>>    static struct rte_timer_data *rte_timer_data_arr;
>>>    static const uint32_t default_data_id;
>>>    static uint32_t rte_timer_subsystem_initialized; @@ -164,6 +165,7 @@
>>> rte_timer_subsystem_init_v1905(void)
>>>    		if (mz == NULL)
>>>    			return -EEXIST;
>>>
>>> +		rte_timer_data_mz = mz;
>>>    		rte_timer_data_arr = mz->addr;
>>>
>>>    		rte_timer_data_arr[default_data_id].internal_flags |= @@ -
>> 180,6
>>> +182,7 @@ rte_timer_subsystem_init_v1905(void)
>>>    	if (mz == NULL)
>>>    		return -ENOMEM;
>>>
>>> +	rte_timer_data_mz = mz;
>>>    	rte_timer_data_arr = mz->addr;
>>>
>>>    	for (i = 0; i < RTE_MAX_DATA_ELS; i++) { @@ -205,8 +208,13 @@
>>> 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_eal_process_type() != RTE_PROC_PRIMARY)
>>> +		return;
>>> +
>>> +	if (!rte_timer_subsystem_initialized)
>>> +		return;
>>> +
>>> +	rte_memzone_free(rte_timer_data_mz);
>>
>> The patch is a correct fix, but the whole idea of this looks dangerous to me.
>>
>> If we exit the primary while secondaries are still running, wouldn't it basically
>> pull out timer data from under secondaries' feet?
>>
> 
> Ah yes - that’s right.  Perhaps it would be better to maintain a reference count of some sort such that the last process to exit could cause the memzone_free.
> 

It feels like a hack, to be honest. A process can crash or exit without 
calling rte_eal_cleanup(), which will lead to a memory leak due to 
refcount being stuck at a value that's not representing reality. It will 
be saf-er than current approach, but still not ideal.

However, i guess it's a good compromise, if i were to choose between a 
memory leak and a segfault :D I wonder if there is a better approach.

> Thanks,
> Erik
> 
>>>
>>>    	rte_timer_subsystem_initialized = 0;
>>>    }
>>>
>>
>>
>> --
>> Thanks,
>> Anatoly


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH] timer: fix resource leak in finalize

[Burakov, Anatoly]

On 02-May-19 1:19 PM, Carrillo, Erik G wrote:
> 
> 
>> -----Original Message-----
>> From: Burakov, Anatoly
>> Sent: Thursday, May 2, 2019 4:18 AM
>> To: Carrillo, Erik G <erik.g.carrillo@intel.com>; rsanford@akamai.com;
>> thomas@monjalon.net
>> Cc: dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH] timer: fix resource leak in finalize
>>
>> On 01-May-19 8:00 PM, Erik Gabriel Carrillo wrote:
>>> The finalize function should free the memzone created in the init
>>> function, rather than freeing the allocation the memzone references,
>>> otherwise a memzone descriptor can be leaked.
>>>
>>> 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 | 12 ++++++++++--
>>>    1 file changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/librte_timer/rte_timer.c
>>> b/lib/librte_timer/rte_timer.c index eb46009..fb7a87e 100644
>>> --- a/lib/librte_timer/rte_timer.c
>>> +++ b/lib/librte_timer/rte_timer.c
>>> @@ -60,6 +60,7 @@ struct rte_timer_data {
>>>    };
>>>
>>>    #define RTE_MAX_DATA_ELS 64
>>> +static const struct rte_memzone *rte_timer_data_mz;
>>>    static struct rte_timer_data *rte_timer_data_arr;
>>>    static const uint32_t default_data_id;
>>>    static uint32_t rte_timer_subsystem_initialized; @@ -164,6 +165,7 @@
>>> rte_timer_subsystem_init_v1905(void)
>>>    		if (mz == NULL)
>>>    			return -EEXIST;
>>>
>>> +		rte_timer_data_mz = mz;
>>>    		rte_timer_data_arr = mz->addr;
>>>
>>>    		rte_timer_data_arr[default_data_id].internal_flags |= @@ -
>> 180,6
>>> +182,7 @@ rte_timer_subsystem_init_v1905(void)
>>>    	if (mz == NULL)
>>>    		return -ENOMEM;
>>>
>>> +	rte_timer_data_mz = mz;
>>>    	rte_timer_data_arr = mz->addr;
>>>
>>>    	for (i = 0; i < RTE_MAX_DATA_ELS; i++) { @@ -205,8 +208,13 @@
>>> 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_eal_process_type() != RTE_PROC_PRIMARY)
>>> +		return;
>>> +
>>> +	if (!rte_timer_subsystem_initialized)
>>> +		return;
>>> +
>>> +	rte_memzone_free(rte_timer_data_mz);
>>
>> The patch is a correct fix, but the whole idea of this looks dangerous to me.
>>
>> If we exit the primary while secondaries are still running, wouldn't it basically
>> pull out timer data from under secondaries' feet?
>>
> 
> Ah yes - that’s right.  Perhaps it would be better to maintain a reference count of some sort such that the last process to exit could cause the memzone_free.
> 

It feels like a hack, to be honest. A process can crash or exit without 
calling rte_eal_cleanup(), which will lead to a memory leak due to 
refcount being stuck at a value that's not representing reality. It will 
be saf-er than current approach, but still not ideal.

However, i guess it's a good compromise, if i were to choose between a 
memory leak and a segfault :D I wonder if there is a better approach.

> Thanks,
> Erik
> 
>>>
>>>    	rte_timer_subsystem_initialized = 0;
>>>    }
>>>
>>
>>
>> --
>> Thanks,
>> Anatoly


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH] timer: fix resource leak in finalize

[Carrillo, Erik G]

> -----Original Message-----
> From: Burakov, Anatoly
> Sent: Thursday, May 2, 2019 8:04 AM
> To: Carrillo, Erik G <erik.g.carrillo@intel.com>; rsanford@akamai.com;
> thomas@monjalon.net
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] timer: fix resource leak in finalize
> 
> On 02-May-19 1:19 PM, Carrillo, Erik G wrote:
> >
> >
> >> -----Original Message-----
> >> From: Burakov, Anatoly
> >> Sent: Thursday, May 2, 2019 4:18 AM
> >> To: Carrillo, Erik G <erik.g.carrillo@intel.com>;
> >> rsanford@akamai.com; thomas@monjalon.net
> >> Cc: dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH] timer: fix resource leak in finalize
> >>
> >> On 01-May-19 8:00 PM, Erik Gabriel Carrillo wrote:
> >>> The finalize function should free the memzone created in the init
> >>> function, rather than freeing the allocation the memzone references,
> >>> otherwise a memzone descriptor can be leaked.
> >>>
> >>> 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 | 12 ++++++++++--
> >>>    1 file changed, 10 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/lib/librte_timer/rte_timer.c
> >>> b/lib/librte_timer/rte_timer.c index eb46009..fb7a87e 100644
> >>> --- a/lib/librte_timer/rte_timer.c
> >>> +++ b/lib/librte_timer/rte_timer.c
> >>> @@ -60,6 +60,7 @@ struct rte_timer_data {
> >>>    };
> >>>
> >>>    #define RTE_MAX_DATA_ELS 64
> >>> +static const struct rte_memzone *rte_timer_data_mz;
> >>>    static struct rte_timer_data *rte_timer_data_arr;
> >>>    static const uint32_t default_data_id;
> >>>    static uint32_t rte_timer_subsystem_initialized; @@ -164,6 +165,7
> >>> @@
> >>> rte_timer_subsystem_init_v1905(void)
> >>>    		if (mz == NULL)
> >>>    			return -EEXIST;
> >>>
> >>> +		rte_timer_data_mz = mz;
> >>>    		rte_timer_data_arr = mz->addr;
> >>>
> >>>    		rte_timer_data_arr[default_data_id].internal_flags |= @@ -
> >> 180,6
> >>> +182,7 @@ rte_timer_subsystem_init_v1905(void)
> >>>    	if (mz == NULL)
> >>>    		return -ENOMEM;
> >>>
> >>> +	rte_timer_data_mz = mz;
> >>>    	rte_timer_data_arr = mz->addr;
> >>>
> >>>    	for (i = 0; i < RTE_MAX_DATA_ELS; i++) { @@ -205,8 +208,13 @@
> >>> 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_eal_process_type() != RTE_PROC_PRIMARY)
> >>> +		return;
> >>> +
> >>> +	if (!rte_timer_subsystem_initialized)
> >>> +		return;
> >>> +
> >>> +	rte_memzone_free(rte_timer_data_mz);
> >>
> >> The patch is a correct fix, but the whole idea of this looks dangerous to
> me.
> >>
> >> If we exit the primary while secondaries are still running, wouldn't
> >> it basically pull out timer data from under secondaries' feet?
> >>
> >
> > Ah yes - that’s right.  Perhaps it would be better to maintain a reference
> count of some sort such that the last process to exit could cause the
> memzone_free.
> >
> 
> It feels like a hack, to be honest. A process can crash or exit without calling
> rte_eal_cleanup(), which will lead to a memory leak due to refcount being
> stuck at a value that's not representing reality. It will be saf-er than current
> approach, but still not ideal.
> 
> However, i guess it's a good compromise, if i were to choose between a
> memory leak and a segfault :D I wonder if there is a better approach.

Ok, I will take a look at that approach then as a first step, since a process can already crash before calling rte_eal_cleanup(), which would result in leaks anyway.

Thanks,
Erik

> 
> > Thanks,
> > Erik
> >
> >>>
> >>>    	rte_timer_subsystem_initialized = 0;
> >>>    }
> >>>
> >>
> >>
> >> --
> >> Thanks,
> >> Anatoly
> 
> 
> --
> Thanks,
> Anatoly

Re: [dpdk-dev] [PATCH] timer: fix resource leak in finalize

[Carrillo, Erik G]

> -----Original Message-----
> From: Burakov, Anatoly
> Sent: Thursday, May 2, 2019 8:04 AM
> To: Carrillo, Erik G <erik.g.carrillo@intel.com>; rsanford@akamai.com;
> thomas@monjalon.net
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] timer: fix resource leak in finalize
> 
> On 02-May-19 1:19 PM, Carrillo, Erik G wrote:
> >
> >
> >> -----Original Message-----
> >> From: Burakov, Anatoly
> >> Sent: Thursday, May 2, 2019 4:18 AM
> >> To: Carrillo, Erik G <erik.g.carrillo@intel.com>;
> >> rsanford@akamai.com; thomas@monjalon.net
> >> Cc: dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH] timer: fix resource leak in finalize
> >>
> >> On 01-May-19 8:00 PM, Erik Gabriel Carrillo wrote:
> >>> The finalize function should free the memzone created in the init
> >>> function, rather than freeing the allocation the memzone references,
> >>> otherwise a memzone descriptor can be leaked.
> >>>
> >>> 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 | 12 ++++++++++--
> >>>    1 file changed, 10 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/lib/librte_timer/rte_timer.c
> >>> b/lib/librte_timer/rte_timer.c index eb46009..fb7a87e 100644
> >>> --- a/lib/librte_timer/rte_timer.c
> >>> +++ b/lib/librte_timer/rte_timer.c
> >>> @@ -60,6 +60,7 @@ struct rte_timer_data {
> >>>    };
> >>>
> >>>    #define RTE_MAX_DATA_ELS 64
> >>> +static const struct rte_memzone *rte_timer_data_mz;
> >>>    static struct rte_timer_data *rte_timer_data_arr;
> >>>    static const uint32_t default_data_id;
> >>>    static uint32_t rte_timer_subsystem_initialized; @@ -164,6 +165,7
> >>> @@
> >>> rte_timer_subsystem_init_v1905(void)
> >>>    		if (mz == NULL)
> >>>    			return -EEXIST;
> >>>
> >>> +		rte_timer_data_mz = mz;
> >>>    		rte_timer_data_arr = mz->addr;
> >>>
> >>>    		rte_timer_data_arr[default_data_id].internal_flags |= @@ -
> >> 180,6
> >>> +182,7 @@ rte_timer_subsystem_init_v1905(void)
> >>>    	if (mz == NULL)
> >>>    		return -ENOMEM;
> >>>
> >>> +	rte_timer_data_mz = mz;
> >>>    	rte_timer_data_arr = mz->addr;
> >>>
> >>>    	for (i = 0; i < RTE_MAX_DATA_ELS; i++) { @@ -205,8 +208,13 @@
> >>> 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_eal_process_type() != RTE_PROC_PRIMARY)
> >>> +		return;
> >>> +
> >>> +	if (!rte_timer_subsystem_initialized)
> >>> +		return;
> >>> +
> >>> +	rte_memzone_free(rte_timer_data_mz);
> >>
> >> The patch is a correct fix, but the whole idea of this looks dangerous to
> me.
> >>
> >> If we exit the primary while secondaries are still running, wouldn't
> >> it basically pull out timer data from under secondaries' feet?
> >>
> >
> > Ah yes - that’s right.  Perhaps it would be better to maintain a reference
> count of some sort such that the last process to exit could cause the
> memzone_free.
> >
> 
> It feels like a hack, to be honest. A process can crash or exit without calling
> rte_eal_cleanup(), which will lead to a memory leak due to refcount being
> stuck at a value that's not representing reality. It will be saf-er than current
> approach, but still not ideal.
> 
> However, i guess it's a good compromise, if i were to choose between a
> memory leak and a segfault :D I wonder if there is a better approach.

Ok, I will take a look at that approach then as a first step, since a process can already crash before calling rte_eal_cleanup(), which would result in leaks anyway.

Thanks,
Erik

> 
> > Thanks,
> > Erik
> >
> >>>
> >>>    	rte_timer_subsystem_initialized = 0;
> >>>    }
> >>>
> >>
> >>
> >> --
> >> Thanks,
> >> Anatoly
> 
> 
> --
> Thanks,
> Anatoly

[dpdk-dev] [PATCH v2] timer: fix resource leak in finalize

[Erik Gabriel Carrillo]

The finalize function should free the memzone created in the init
function, rather than freeing the allocation the memzone references,
otherwise a memzone descriptor can be leaked.

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

Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
---
changes in v2:
 - Handle scenario where primary process exits before secondaries such
   that memzone is not freed early (Anatoly)

 lib/librte_timer/rte_timer.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
index eb46009..4771287 100644
--- a/lib/librte_timer/rte_timer.c
+++ b/lib/librte_timer/rte_timer.c
@@ -60,6 +60,8 @@ struct rte_timer_data {
 };
 
 #define RTE_MAX_DATA_ELS 64
+static const struct rte_memzone *rte_timer_data_mz;
+static rte_atomic16_t *rte_timer_mz_refcnt;
 static struct rte_timer_data *rte_timer_data_arr;
 static const uint32_t default_data_id;
 static uint32_t rte_timer_subsystem_initialized;
@@ -155,6 +157,7 @@ rte_timer_subsystem_init_v1905(void)
 	struct rte_timer_data *data;
 	int i, lcore_id;
 	static const char *mz_name = "rte_timer_mz";
+	size_t data_arr_size = RTE_MAX_DATA_ELS * sizeof(*rte_timer_data_arr);
 
 	if (rte_timer_subsystem_initialized)
 		return -EALREADY;
@@ -164,10 +167,14 @@ rte_timer_subsystem_init_v1905(void)
 		if (mz == NULL)
 			return -EEXIST;
 
+		rte_timer_data_mz = mz;
 		rte_timer_data_arr = mz->addr;
+		rte_timer_mz_refcnt =
+				(void *)((char *)mz->addr + data_arr_size);
 
 		rte_timer_data_arr[default_data_id].internal_flags |=
 			FL_ALLOCATED;
+		rte_atomic16_inc(rte_timer_mz_refcnt);
 
 		rte_timer_subsystem_initialized = 1;
 
@@ -175,12 +182,15 @@ rte_timer_subsystem_init_v1905(void)
 	}
 
 	mz = rte_memzone_reserve_aligned(mz_name,
-			RTE_MAX_DATA_ELS * sizeof(*rte_timer_data_arr),
+			data_arr_size + sizeof(*rte_timer_mz_refcnt),
 			SOCKET_ID_ANY, 0, RTE_CACHE_LINE_SIZE);
 	if (mz == NULL)
 		return -ENOMEM;
 
+	rte_timer_data_mz = mz;
 	rte_timer_data_arr = mz->addr;
+	rte_timer_mz_refcnt = (void *)((char *)mz->addr + data_arr_size);
+	rte_atomic16_init(rte_timer_mz_refcnt);
 
 	for (i = 0; i < RTE_MAX_DATA_ELS; i++) {
 		data = &rte_timer_data_arr[i];
@@ -193,6 +203,7 @@ rte_timer_subsystem_init_v1905(void)
 	}
 
 	rte_timer_data_arr[default_data_id].internal_flags |= FL_ALLOCATED;
+	rte_atomic16_inc(rte_timer_mz_refcnt);
 
 	rte_timer_subsystem_initialized = 1;
 
@@ -205,8 +216,11 @@ 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;
+
+	if (rte_atomic16_dec_and_test(rte_timer_mz_refcnt))
+		rte_memzone_free(rte_timer_data_mz);
 
 	rte_timer_subsystem_initialized = 0;
 }
-- 
2.6.4

[dpdk-dev] [PATCH v2] timer: fix resource leak in finalize

[Erik Gabriel Carrillo]

The finalize function should free the memzone created in the init
function, rather than freeing the allocation the memzone references,
otherwise a memzone descriptor can be leaked.

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

Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
---
changes in v2:
 - Handle scenario where primary process exits before secondaries such
   that memzone is not freed early (Anatoly)

 lib/librte_timer/rte_timer.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
index eb46009..4771287 100644
--- a/lib/librte_timer/rte_timer.c
+++ b/lib/librte_timer/rte_timer.c
@@ -60,6 +60,8 @@ struct rte_timer_data {
 };
 
 #define RTE_MAX_DATA_ELS 64
+static const struct rte_memzone *rte_timer_data_mz;
+static rte_atomic16_t *rte_timer_mz_refcnt;
 static struct rte_timer_data *rte_timer_data_arr;
 static const uint32_t default_data_id;
 static uint32_t rte_timer_subsystem_initialized;
@@ -155,6 +157,7 @@ rte_timer_subsystem_init_v1905(void)
 	struct rte_timer_data *data;
 	int i, lcore_id;
 	static const char *mz_name = "rte_timer_mz";
+	size_t data_arr_size = RTE_MAX_DATA_ELS * sizeof(*rte_timer_data_arr);
 
 	if (rte_timer_subsystem_initialized)
 		return -EALREADY;
@@ -164,10 +167,14 @@ rte_timer_subsystem_init_v1905(void)
 		if (mz == NULL)
 			return -EEXIST;
 
+		rte_timer_data_mz = mz;
 		rte_timer_data_arr = mz->addr;
+		rte_timer_mz_refcnt =
+				(void *)((char *)mz->addr + data_arr_size);
 
 		rte_timer_data_arr[default_data_id].internal_flags |=
 			FL_ALLOCATED;
+		rte_atomic16_inc(rte_timer_mz_refcnt);
 
 		rte_timer_subsystem_initialized = 1;
 
@@ -175,12 +182,15 @@ rte_timer_subsystem_init_v1905(void)
 	}
 
 	mz = rte_memzone_reserve_aligned(mz_name,
-			RTE_MAX_DATA_ELS * sizeof(*rte_timer_data_arr),
+			data_arr_size + sizeof(*rte_timer_mz_refcnt),
 			SOCKET_ID_ANY, 0, RTE_CACHE_LINE_SIZE);
 	if (mz == NULL)
 		return -ENOMEM;
 
+	rte_timer_data_mz = mz;
 	rte_timer_data_arr = mz->addr;
+	rte_timer_mz_refcnt = (void *)((char *)mz->addr + data_arr_size);
+	rte_atomic16_init(rte_timer_mz_refcnt);
 
 	for (i = 0; i < RTE_MAX_DATA_ELS; i++) {
 		data = &rte_timer_data_arr[i];
@@ -193,6 +203,7 @@ rte_timer_subsystem_init_v1905(void)
 	}
 
 	rte_timer_data_arr[default_data_id].internal_flags |= FL_ALLOCATED;
+	rte_atomic16_inc(rte_timer_mz_refcnt);
 
 	rte_timer_subsystem_initialized = 1;
 
@@ -205,8 +216,11 @@ 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;
+
+	if (rte_atomic16_dec_and_test(rte_timer_mz_refcnt))
+		rte_memzone_free(rte_timer_data_mz);
 
 	rte_timer_subsystem_initialized = 0;
 }
-- 
2.6.4

Re: [dpdk-dev] [PATCH v2] timer: fix resource leak in finalize

[Burakov, Anatoly]

On 03-May-19 11:54 PM, Erik Gabriel Carrillo wrote:
> The finalize function should free the memzone created in the init
> function, rather than freeing the allocation the memzone references,
> otherwise a memzone descriptor can be leaked.
> 
> Fixes: c0749f7096c7 ("timer: allow management in shared memory")
> 
> Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> ---
> changes in v2:
>   - Handle scenario where primary process exits before secondaries such
>     that memzone is not freed early (Anatoly)
> 
>   lib/librte_timer/rte_timer.c | 20 +++++++++++++++++---
>   1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
> index eb46009..4771287 100644
> --- a/lib/librte_timer/rte_timer.c
> +++ b/lib/librte_timer/rte_timer.c
> @@ -60,6 +60,8 @@ struct rte_timer_data {
>   };
>   
>   #define RTE_MAX_DATA_ELS 64
> +static const struct rte_memzone *rte_timer_data_mz;
> +static rte_atomic16_t *rte_timer_mz_refcnt;
>   static struct rte_timer_data *rte_timer_data_arr;
>   static const uint32_t default_data_id;
>   static uint32_t rte_timer_subsystem_initialized;
> @@ -155,6 +157,7 @@ rte_timer_subsystem_init_v1905(void)
>   	struct rte_timer_data *data;
>   	int i, lcore_id;
>   	static const char *mz_name = "rte_timer_mz";
> +	size_t data_arr_size = RTE_MAX_DATA_ELS * sizeof(*rte_timer_data_arr);

nitpicking, but... const?

>   
>   	if (rte_timer_subsystem_initialized)
>   		return -EALREADY;
> @@ -164,10 +167,14 @@ rte_timer_subsystem_init_v1905(void)
>   		if (mz == NULL)
>   			return -EEXIST;
>   
> +		rte_timer_data_mz = mz;
>   		rte_timer_data_arr = mz->addr;
> +		rte_timer_mz_refcnt =
> +				(void *)((char *)mz->addr + data_arr_size);
>   
>   		rte_timer_data_arr[default_data_id].internal_flags |=
>   			FL_ALLOCATED;
> +		rte_atomic16_inc(rte_timer_mz_refcnt);
>   
>   		rte_timer_subsystem_initialized = 1;
>   
> @@ -175,12 +182,15 @@ rte_timer_subsystem_init_v1905(void)
>   	}
>   
>   	mz = rte_memzone_reserve_aligned(mz_name,
> -			RTE_MAX_DATA_ELS * sizeof(*rte_timer_data_arr),
> +			data_arr_size + sizeof(*rte_timer_mz_refcnt),
>   			SOCKET_ID_ANY, 0, RTE_CACHE_LINE_SIZE);
>   	if (mz == NULL)
>   		return -ENOMEM;
>   
> +	rte_timer_data_mz = mz;
>   	rte_timer_data_arr = mz->addr;
> +	rte_timer_mz_refcnt = (void *)((char *)mz->addr + data_arr_size);
> +	rte_atomic16_init(rte_timer_mz_refcnt);
>   
>   	for (i = 0; i < RTE_MAX_DATA_ELS; i++) {
>   		data = &rte_timer_data_arr[i];
> @@ -193,6 +203,7 @@ rte_timer_subsystem_init_v1905(void)
>   	}
>   
>   	rte_timer_data_arr[default_data_id].internal_flags |= FL_ALLOCATED;
> +	rte_atomic16_inc(rte_timer_mz_refcnt);
>   
>   	rte_timer_subsystem_initialized = 1;
>   
> @@ -205,8 +216,11 @@ 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;
> +
> +	if (rte_atomic16_dec_and_test(rte_timer_mz_refcnt))
> +		rte_memzone_free(rte_timer_data_mz);

I think there's a race here. You may get preempted after test but before 
free, where another secondary could initialize. As far as i know, we 
also support a case when secondary initializes after primary stops running.

Let's even suppose that we allow secondary processes to initialize the 
timer subsystem by reserving memzone and checking rte_errno. You would 
still have a chance of two init/deinit conflicting, because there's a 
hole between memzone allocation and atomic increment.

I don't think this race can be resolved in a safe way, so we might just 
have to settle for a memory leak.

>   
>   	rte_timer_subsystem_initialized = 0;
>   }
> 


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v2] timer: fix resource leak in finalize

[Burakov, Anatoly]

On 03-May-19 11:54 PM, Erik Gabriel Carrillo wrote:
> The finalize function should free the memzone created in the init
> function, rather than freeing the allocation the memzone references,
> otherwise a memzone descriptor can be leaked.
> 
> Fixes: c0749f7096c7 ("timer: allow management in shared memory")
> 
> Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> ---
> changes in v2:
>   - Handle scenario where primary process exits before secondaries such
>     that memzone is not freed early (Anatoly)
> 
>   lib/librte_timer/rte_timer.c | 20 +++++++++++++++++---
>   1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
> index eb46009..4771287 100644
> --- a/lib/librte_timer/rte_timer.c
> +++ b/lib/librte_timer/rte_timer.c
> @@ -60,6 +60,8 @@ struct rte_timer_data {
>   };
>   
>   #define RTE_MAX_DATA_ELS 64
> +static const struct rte_memzone *rte_timer_data_mz;
> +static rte_atomic16_t *rte_timer_mz_refcnt;
>   static struct rte_timer_data *rte_timer_data_arr;
>   static const uint32_t default_data_id;
>   static uint32_t rte_timer_subsystem_initialized;
> @@ -155,6 +157,7 @@ rte_timer_subsystem_init_v1905(void)
>   	struct rte_timer_data *data;
>   	int i, lcore_id;
>   	static const char *mz_name = "rte_timer_mz";
> +	size_t data_arr_size = RTE_MAX_DATA_ELS * sizeof(*rte_timer_data_arr);

nitpicking, but... const?

>   
>   	if (rte_timer_subsystem_initialized)
>   		return -EALREADY;
> @@ -164,10 +167,14 @@ rte_timer_subsystem_init_v1905(void)
>   		if (mz == NULL)
>   			return -EEXIST;
>   
> +		rte_timer_data_mz = mz;
>   		rte_timer_data_arr = mz->addr;
> +		rte_timer_mz_refcnt =
> +				(void *)((char *)mz->addr + data_arr_size);
>   
>   		rte_timer_data_arr[default_data_id].internal_flags |=
>   			FL_ALLOCATED;
> +		rte_atomic16_inc(rte_timer_mz_refcnt);
>   
>   		rte_timer_subsystem_initialized = 1;
>   
> @@ -175,12 +182,15 @@ rte_timer_subsystem_init_v1905(void)
>   	}
>   
>   	mz = rte_memzone_reserve_aligned(mz_name,
> -			RTE_MAX_DATA_ELS * sizeof(*rte_timer_data_arr),
> +			data_arr_size + sizeof(*rte_timer_mz_refcnt),
>   			SOCKET_ID_ANY, 0, RTE_CACHE_LINE_SIZE);
>   	if (mz == NULL)
>   		return -ENOMEM;
>   
> +	rte_timer_data_mz = mz;
>   	rte_timer_data_arr = mz->addr;
> +	rte_timer_mz_refcnt = (void *)((char *)mz->addr + data_arr_size);
> +	rte_atomic16_init(rte_timer_mz_refcnt);
>   
>   	for (i = 0; i < RTE_MAX_DATA_ELS; i++) {
>   		data = &rte_timer_data_arr[i];
> @@ -193,6 +203,7 @@ rte_timer_subsystem_init_v1905(void)
>   	}
>   
>   	rte_timer_data_arr[default_data_id].internal_flags |= FL_ALLOCATED;
> +	rte_atomic16_inc(rte_timer_mz_refcnt);
>   
>   	rte_timer_subsystem_initialized = 1;
>   
> @@ -205,8 +216,11 @@ 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;
> +
> +	if (rte_atomic16_dec_and_test(rte_timer_mz_refcnt))
> +		rte_memzone_free(rte_timer_data_mz);

I think there's a race here. You may get preempted after test but before 
free, where another secondary could initialize. As far as i know, we 
also support a case when secondary initializes after primary stops running.

Let's even suppose that we allow secondary processes to initialize the 
timer subsystem by reserving memzone and checking rte_errno. You would 
still have a chance of two init/deinit conflicting, because there's a 
hole between memzone allocation and atomic increment.

I don't think this race can be resolved in a safe way, so we might just 
have to settle for a memory leak.

>   
>   	rte_timer_subsystem_initialized = 0;
>   }
> 


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v2] timer: fix resource leak in finalize

[Carrillo, Erik G]

Hi Anatoly,

Thanks for the review.  Comments in-line:

<...snipped...>

> >   #define RTE_MAX_DATA_ELS 64
> > +static const struct rte_memzone *rte_timer_data_mz; static
> > +rte_atomic16_t *rte_timer_mz_refcnt;
> >   static struct rte_timer_data *rte_timer_data_arr;
> >   static const uint32_t default_data_id;
> >   static uint32_t rte_timer_subsystem_initialized; @@ -155,6 +157,7 @@
> > rte_timer_subsystem_init_v1905(void)
> >   	struct rte_timer_data *data;
> >   	int i, lcore_id;
> >   	static const char *mz_name = "rte_timer_mz";
> > +	size_t data_arr_size = RTE_MAX_DATA_ELS *
> > +sizeof(*rte_timer_data_arr);
> 
> nitpicking, but... const?
> 

No problem - I'll make this change if this line persists into the next version.

<...snipped...>

> >
> > @@ -205,8 +216,11 @@
> 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;
> > +
> > +	if (rte_atomic16_dec_and_test(rte_timer_mz_refcnt))
> > +		rte_memzone_free(rte_timer_data_mz);
> 
> I think there's a race here. You may get preempted after test but before
> free, where another secondary could initialize. As far as i know, we also

Indeed, thanks for catching this.

> support a case when secondary initializes after primary stops running.
> 
> Let's even suppose that we allow secondary processes to initialize the timer
> subsystem by reserving memzone and checking rte_errno. You would still
> have a chance of two init/deinit conflicting, because there's a hole between
> memzone allocation and atomic increment.
> 
> I don't think this race can be resolved in a safe way, so we might just have to
> settle for a memory leak.
> 

I don't see a solution here currently either.  I'll look at removing the memzone_free()
call and possibly the rte_timer_subsystem_finalize() API, since it seems like
there's no reason for it to exist if it can't free the allocations.

Regards,
Erik

> >
> >   	rte_timer_subsystem_initialized = 0;
> >   }
> >
> 
> 
> --
> Thanks,
> Anatoly

Re: [dpdk-dev] [PATCH v2] timer: fix resource leak in finalize

[Carrillo, Erik G]

Hi Anatoly,

Thanks for the review.  Comments in-line:

<...snipped...>

> >   #define RTE_MAX_DATA_ELS 64
> > +static const struct rte_memzone *rte_timer_data_mz; static
> > +rte_atomic16_t *rte_timer_mz_refcnt;
> >   static struct rte_timer_data *rte_timer_data_arr;
> >   static const uint32_t default_data_id;
> >   static uint32_t rte_timer_subsystem_initialized; @@ -155,6 +157,7 @@
> > rte_timer_subsystem_init_v1905(void)
> >   	struct rte_timer_data *data;
> >   	int i, lcore_id;
> >   	static const char *mz_name = "rte_timer_mz";
> > +	size_t data_arr_size = RTE_MAX_DATA_ELS *
> > +sizeof(*rte_timer_data_arr);
> 
> nitpicking, but... const?
> 

No problem - I'll make this change if this line persists into the next version.

<...snipped...>

> >
> > @@ -205,8 +216,11 @@
> 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;
> > +
> > +	if (rte_atomic16_dec_and_test(rte_timer_mz_refcnt))
> > +		rte_memzone_free(rte_timer_data_mz);
> 
> I think there's a race here. You may get preempted after test but before
> free, where another secondary could initialize. As far as i know, we also

Indeed, thanks for catching this.

> support a case when secondary initializes after primary stops running.
> 
> Let's even suppose that we allow secondary processes to initialize the timer
> subsystem by reserving memzone and checking rte_errno. You would still
> have a chance of two init/deinit conflicting, because there's a hole between
> memzone allocation and atomic increment.
> 
> I don't think this race can be resolved in a safe way, so we might just have to
> settle for a memory leak.
> 

I don't see a solution here currently either.  I'll look at removing the memzone_free()
call and possibly the rte_timer_subsystem_finalize() API, since it seems like
there's no reason for it to exist if it can't free the allocations.

Regards,
Erik

> >
> >   	rte_timer_subsystem_initialized = 0;
> >   }
> >
> 
> 
> --
> Thanks,
> Anatoly

Re: [dpdk-dev] [PATCH v2] timer: fix resource leak in finalize

[Burakov, Anatoly]

On 07-May-19 11:04 PM, Carrillo, Erik G wrote:
> Hi Anatoly,
> 
> Thanks for the review.  Comments in-line:
> 
> <...snipped...>
> 
>>>    #define RTE_MAX_DATA_ELS 64
>>> +static const struct rte_memzone *rte_timer_data_mz; static
>>> +rte_atomic16_t *rte_timer_mz_refcnt;
>>>    static struct rte_timer_data *rte_timer_data_arr;
>>>    static const uint32_t default_data_id;
>>>    static uint32_t rte_timer_subsystem_initialized; @@ -155,6 +157,7 @@
>>> rte_timer_subsystem_init_v1905(void)
>>>    	struct rte_timer_data *data;
>>>    	int i, lcore_id;
>>>    	static const char *mz_name = "rte_timer_mz";
>>> +	size_t data_arr_size = RTE_MAX_DATA_ELS *
>>> +sizeof(*rte_timer_data_arr);
>>
>> nitpicking, but... const?
>>
> 
> No problem - I'll make this change if this line persists into the next version.
> 
> <...snipped...>
> 
>>>
>>> @@ -205,8 +216,11 @@
>> 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;
>>> +
>>> +	if (rte_atomic16_dec_and_test(rte_timer_mz_refcnt))
>>> +		rte_memzone_free(rte_timer_data_mz);
>>
>> I think there's a race here. You may get preempted after test but before
>> free, where another secondary could initialize. As far as i know, we also
> 
> Indeed, thanks for catching this.
> 
>> support a case when secondary initializes after primary stops running.
>>
>> Let's even suppose that we allow secondary processes to initialize the timer
>> subsystem by reserving memzone and checking rte_errno. You would still
>> have a chance of two init/deinit conflicting, because there's a hole between
>> memzone allocation and atomic increment.
>>
>> I don't think this race can be resolved in a safe way, so we might just have to
>> settle for a memory leak.
>>
> 
> I don't see a solution here currently either.  I'll look at removing the memzone_free()
> call and possibly the rte_timer_subsystem_finalize() API, since it seems like
> there's no reason for it to exist if it can't free the allocations.

I wonder if there are other places in DPDK where this pattern is used.

Technically, this kind of thing /could/ be resolved by having something 
in our multiprocess shared memory outside of DPDK heap. I.e. store 
something in rte_eal_memconfig like some other things do. This change, 
however, would require an ABI break, so while changing this particular 
API won't need a deprecation notice, the change itself would.

> 
> Regards,
> Erik
> 
>>>
>>>    	rte_timer_subsystem_initialized = 0;
>>>    }
>>>
>>
>>
>> --
>> Thanks,
>> Anatoly


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v2] timer: fix resource leak in finalize

[Burakov, Anatoly]

On 07-May-19 11:04 PM, Carrillo, Erik G wrote:
> Hi Anatoly,
> 
> Thanks for the review.  Comments in-line:
> 
> <...snipped...>
> 
>>>    #define RTE_MAX_DATA_ELS 64
>>> +static const struct rte_memzone *rte_timer_data_mz; static
>>> +rte_atomic16_t *rte_timer_mz_refcnt;
>>>    static struct rte_timer_data *rte_timer_data_arr;
>>>    static const uint32_t default_data_id;
>>>    static uint32_t rte_timer_subsystem_initialized; @@ -155,6 +157,7 @@
>>> rte_timer_subsystem_init_v1905(void)
>>>    	struct rte_timer_data *data;
>>>    	int i, lcore_id;
>>>    	static const char *mz_name = "rte_timer_mz";
>>> +	size_t data_arr_size = RTE_MAX_DATA_ELS *
>>> +sizeof(*rte_timer_data_arr);
>>
>> nitpicking, but... const?
>>
> 
> No problem - I'll make this change if this line persists into the next version.
> 
> <...snipped...>
> 
>>>
>>> @@ -205,8 +216,11 @@
>> 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;
>>> +
>>> +	if (rte_atomic16_dec_and_test(rte_timer_mz_refcnt))
>>> +		rte_memzone_free(rte_timer_data_mz);
>>
>> I think there's a race here. You may get preempted after test but before
>> free, where another secondary could initialize. As far as i know, we also
> 
> Indeed, thanks for catching this.
> 
>> support a case when secondary initializes after primary stops running.
>>
>> Let's even suppose that we allow secondary processes to initialize the timer
>> subsystem by reserving memzone and checking rte_errno. You would still
>> have a chance of two init/deinit conflicting, because there's a hole between
>> memzone allocation and atomic increment.
>>
>> I don't think this race can be resolved in a safe way, so we might just have to
>> settle for a memory leak.
>>
> 
> I don't see a solution here currently either.  I'll look at removing the memzone_free()
> call and possibly the rte_timer_subsystem_finalize() API, since it seems like
> there's no reason for it to exist if it can't free the allocations.

I wonder if there are other places in DPDK where this pattern is used.

Technically, this kind of thing /could/ be resolved by having something 
in our multiprocess shared memory outside of DPDK heap. I.e. store 
something in rte_eal_memconfig like some other things do. This change, 
however, would require an ABI break, so while changing this particular 
API won't need a deprecation notice, the change itself would.

> 
> Regards,
> Erik
> 
>>>
>>>    	rte_timer_subsystem_initialized = 0;
>>>    }
>>>
>>
>>
>> --
>> Thanks,
>> Anatoly


-- 
Thanks,
Anatoly

[dpdk-dev] [PATCH v3] timer: fix resource leak in finalize

[Erik Gabriel Carrillo]

By using a lock added to the rte_mem_config (which lives in shared
memory), we can synchronize multiple processes in init/finalize and
safely free allocations made during init.

Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
---
changes in v3:
 - The previous version had race condition.  This version fixes the race
   by adding a lock in shared memory outside of the DPDK heap area 
   that can be used to safely free the memzone reserved in the subsystem
   init call. (Anatoly)

   This patch depends on http://patches.dpdk.org/patch/53333/.
 
changes in v2:
 - Handle scenario where primary process exits before secondaries such
   that memzone is not freed early (Anatoly)

 lib/librte_eal/common/include/rte_eal_memconfig.h |  3 +++
 lib/librte_timer/rte_timer.c                      | 23 ++++++++++++++++++++++-
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/include/rte_eal_memconfig.h b/lib/librte_eal/common/include/rte_eal_memconfig.h
index 84aabe3..8cbc09c 100644
--- a/lib/librte_eal/common/include/rte_eal_memconfig.h
+++ b/lib/librte_eal/common/include/rte_eal_memconfig.h
@@ -64,6 +64,9 @@ struct rte_mem_config {
 	rte_rwlock_t memory_hotplug_lock;
 	/**< indicates whether memory hotplug request is in progress. */
 
+	rte_spinlock_t timer_subsystem_lock;
+	/**< indicates whether timer subsystem init/finalize is in progress. */
+
 	/* memory segments and zones */
 	struct rte_fbarray memzones; /**< Memzone descriptors. */
 
diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
index c0f5b87..a055d6e 100644
--- a/lib/librte_timer/rte_timer.c
+++ b/lib/librte_timer/rte_timer.c
@@ -26,6 +26,7 @@
 #include <rte_malloc.h>
 #include <rte_compat.h>
 #include <rte_errno.h>
+#include <rte_eal_memconfig.h>
 
 #include "rte_timer.h"
 
@@ -60,7 +61,12 @@ struct rte_timer_data {
 	uint8_t internal_flags;
 };
 
+#define RTE_TIMER_SUBSYSTEM_LOCK \
+	(&rte_eal_get_configuration()->mem_config->timer_subsystem_lock)
+
 #define RTE_MAX_DATA_ELS 64
+static const struct rte_memzone *rte_timer_data_mz;
+static int *rte_timer_mz_refcnt;
 static struct rte_timer_data *rte_timer_data_arr;
 static const uint32_t default_data_id;
 static uint32_t rte_timer_subsystem_initialized;
@@ -163,22 +169,27 @@ rte_timer_subsystem_init_v1905(void)
 	if (rte_timer_subsystem_initialized)
 		return -EALREADY;
 
+	rte_spinlock_lock(RTE_TIMER_SUBSYSTEM_LOCK);
 lookup:
 	mz = rte_memzone_lookup(mz_name);
 	if (mz == NULL) {
-		mz = rte_memzone_reserve_aligned(mz_name, data_arr_size,
+		mz = rte_memzone_reserve_aligned(mz_name,
+				data_arr_size + sizeof(*rte_timer_mz_refcnt),
 				SOCKET_ID_ANY, 0, RTE_CACHE_LINE_SIZE);
 		if (mz == NULL) {
 			if (rte_errno == EEXIST)
 				goto lookup;
 
+			rte_spinlock_unlock(RTE_TIMER_SUBSYSTEM_LOCK);
 			return -ENOMEM;
 		}
 
 		do_full_init = true;
 	}
 
+	rte_timer_data_mz = mz;
 	rte_timer_data_arr = mz->addr;
+	rte_timer_mz_refcnt = (void *)((char *)mz->addr + data_arr_size);
 
 	if (do_full_init) {
 		for (i = 0; i < RTE_MAX_DATA_ELS; i++) {
@@ -195,6 +206,9 @@ rte_timer_subsystem_init_v1905(void)
 	}
 
 	rte_timer_data_arr[default_data_id].internal_flags |= FL_ALLOCATED;
+	(*rte_timer_mz_refcnt)++;
+
+	rte_spinlock_unlock(RTE_TIMER_SUBSYSTEM_LOCK);
 
 	rte_timer_subsystem_initialized = 1;
 
@@ -210,6 +224,13 @@ rte_timer_subsystem_finalize(void)
 	if (!rte_timer_subsystem_initialized)
 		return;
 
+	rte_spinlock_lock(RTE_TIMER_SUBSYSTEM_LOCK);
+
+	if (--(*rte_timer_mz_refcnt) == 0)
+		rte_memzone_free(rte_timer_data_mz);
+
+	rte_spinlock_unlock(RTE_TIMER_SUBSYSTEM_LOCK);
+
 	rte_timer_subsystem_initialized = 0;
 }
 
-- 
2.6.4

[dpdk-dev] [PATCH v3] timer: fix resource leak in finalize

[Erik Gabriel Carrillo]

By using a lock added to the rte_mem_config (which lives in shared
memory), we can synchronize multiple processes in init/finalize and
safely free allocations made during init.

Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
---
changes in v3:
 - The previous version had race condition.  This version fixes the race
   by adding a lock in shared memory outside of the DPDK heap area 
   that can be used to safely free the memzone reserved in the subsystem
   init call. (Anatoly)

   This patch depends on http://patches.dpdk.org/patch/53333/.
 
changes in v2:
 - Handle scenario where primary process exits before secondaries such
   that memzone is not freed early (Anatoly)

 lib/librte_eal/common/include/rte_eal_memconfig.h |  3 +++
 lib/librte_timer/rte_timer.c                      | 23 ++++++++++++++++++++++-
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/include/rte_eal_memconfig.h b/lib/librte_eal/common/include/rte_eal_memconfig.h
index 84aabe3..8cbc09c 100644
--- a/lib/librte_eal/common/include/rte_eal_memconfig.h
+++ b/lib/librte_eal/common/include/rte_eal_memconfig.h
@@ -64,6 +64,9 @@ struct rte_mem_config {
 	rte_rwlock_t memory_hotplug_lock;
 	/**< indicates whether memory hotplug request is in progress. */
 
+	rte_spinlock_t timer_subsystem_lock;
+	/**< indicates whether timer subsystem init/finalize is in progress. */
+
 	/* memory segments and zones */
 	struct rte_fbarray memzones; /**< Memzone descriptors. */
 
diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
index c0f5b87..a055d6e 100644
--- a/lib/librte_timer/rte_timer.c
+++ b/lib/librte_timer/rte_timer.c
@@ -26,6 +26,7 @@
 #include <rte_malloc.h>
 #include <rte_compat.h>
 #include <rte_errno.h>
+#include <rte_eal_memconfig.h>
 
 #include "rte_timer.h"
 
@@ -60,7 +61,12 @@ struct rte_timer_data {
 	uint8_t internal_flags;
 };
 
+#define RTE_TIMER_SUBSYSTEM_LOCK \
+	(&rte_eal_get_configuration()->mem_config->timer_subsystem_lock)
+
 #define RTE_MAX_DATA_ELS 64
+static const struct rte_memzone *rte_timer_data_mz;
+static int *rte_timer_mz_refcnt;
 static struct rte_timer_data *rte_timer_data_arr;
 static const uint32_t default_data_id;
 static uint32_t rte_timer_subsystem_initialized;
@@ -163,22 +169,27 @@ rte_timer_subsystem_init_v1905(void)
 	if (rte_timer_subsystem_initialized)
 		return -EALREADY;
 
+	rte_spinlock_lock(RTE_TIMER_SUBSYSTEM_LOCK);
 lookup:
 	mz = rte_memzone_lookup(mz_name);
 	if (mz == NULL) {
-		mz = rte_memzone_reserve_aligned(mz_name, data_arr_size,
+		mz = rte_memzone_reserve_aligned(mz_name,
+				data_arr_size + sizeof(*rte_timer_mz_refcnt),
 				SOCKET_ID_ANY, 0, RTE_CACHE_LINE_SIZE);
 		if (mz == NULL) {
 			if (rte_errno == EEXIST)
 				goto lookup;
 
+			rte_spinlock_unlock(RTE_TIMER_SUBSYSTEM_LOCK);
 			return -ENOMEM;
 		}
 
 		do_full_init = true;
 	}
 
+	rte_timer_data_mz = mz;
 	rte_timer_data_arr = mz->addr;
+	rte_timer_mz_refcnt = (void *)((char *)mz->addr + data_arr_size);
 
 	if (do_full_init) {
 		for (i = 0; i < RTE_MAX_DATA_ELS; i++) {
@@ -195,6 +206,9 @@ rte_timer_subsystem_init_v1905(void)
 	}
 
 	rte_timer_data_arr[default_data_id].internal_flags |= FL_ALLOCATED;
+	(*rte_timer_mz_refcnt)++;
+
+	rte_spinlock_unlock(RTE_TIMER_SUBSYSTEM_LOCK);
 
 	rte_timer_subsystem_initialized = 1;
 
@@ -210,6 +224,13 @@ rte_timer_subsystem_finalize(void)
 	if (!rte_timer_subsystem_initialized)
 		return;
 
+	rte_spinlock_lock(RTE_TIMER_SUBSYSTEM_LOCK);
+
+	if (--(*rte_timer_mz_refcnt) == 0)
+		rte_memzone_free(rte_timer_data_mz);
+
+	rte_spinlock_unlock(RTE_TIMER_SUBSYSTEM_LOCK);
+
 	rte_timer_subsystem_initialized = 0;
 }
 
-- 
2.6.4

Re: [dpdk-dev] [PATCH v2] timer: fix resource leak in finalize

[Carrillo, Erik G]

> -----Original Message-----
> From: Burakov, Anatoly
> Sent: Wednesday, May 8, 2019 3:50 AM
> To: Carrillo, Erik G <erik.g.carrillo@intel.com>; rsanford@akamai.com;
> thomas@monjalon.net
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v2] timer: fix resource leak in finalize
> 
> On 07-May-19 11:04 PM, Carrillo, Erik G wrote:
> > Hi Anatoly,
> >
> > Thanks for the review.  Comments in-line:
> >
> > <...snipped...>
> >
> >>>    #define RTE_MAX_DATA_ELS 64
> >>> +static const struct rte_memzone *rte_timer_data_mz; static
> >>> +rte_atomic16_t *rte_timer_mz_refcnt;
> >>>    static struct rte_timer_data *rte_timer_data_arr;
> >>>    static const uint32_t default_data_id;
> >>>    static uint32_t rte_timer_subsystem_initialized; @@ -155,6 +157,7
> >>> @@
> >>> rte_timer_subsystem_init_v1905(void)
> >>>    	struct rte_timer_data *data;
> >>>    	int i, lcore_id;
> >>>    	static const char *mz_name = "rte_timer_mz";
> >>> +	size_t data_arr_size = RTE_MAX_DATA_ELS *
> >>> +sizeof(*rte_timer_data_arr);
> >>
> >> nitpicking, but... const?
> >>
> >
> > No problem - I'll make this change if this line persists into the next version.
> >
> > <...snipped...>
> >
> >>>
> >>> @@ -205,8 +216,11 @@
> >> 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;
> >>> +
> >>> +	if (rte_atomic16_dec_and_test(rte_timer_mz_refcnt))
> >>> +		rte_memzone_free(rte_timer_data_mz);
> >>
> >> I think there's a race here. You may get preempted after test but
> >> before free, where another secondary could initialize. As far as i
> >> know, we also
> >
> > Indeed, thanks for catching this.
> >
> >> support a case when secondary initializes after primary stops running.
> >>
> >> Let's even suppose that we allow secondary processes to initialize
> >> the timer subsystem by reserving memzone and checking rte_errno. You
> >> would still have a chance of two init/deinit conflicting, because
> >> there's a hole between memzone allocation and atomic increment.
> >>
> >> I don't think this race can be resolved in a safe way, so we might
> >> just have to settle for a memory leak.
> >>
> >
> > I don't see a solution here currently either.  I'll look at removing
> > the memzone_free() call and possibly the
> > rte_timer_subsystem_finalize() API, since it seems like there's no reason
> for it to exist if it can't free the allocations.
> 
> I wonder if there are other places in DPDK where this pattern is used.
> 
> Technically, this kind of thing /could/ be resolved by having something in our
> multiprocess shared memory outside of DPDK heap. I.e. store something in
> rte_eal_memconfig like some other things do. This change, however, would
> require an ABI break, so while changing this particular API won't need a
> deprecation notice, the change itself would.

I like this idea; thanks for the suggestion.  I went ahead and submitted a new version 
of this patch with this approach.  It's dependent on one other new patch that allows
secondary processes to reserve the memzone.  I also submitted a deprecation
notice for the ABI break for the change to rte_mem_config.

Thomas, I suspect that I should mark v3 of this patch as "deferred", so I've gone ahead and
done that, but let me know if that's wrong.

Thanks,
Erik

> 
> >
> > Regards,
> > Erik
> >
> >>>
> >>>    	rte_timer_subsystem_initialized = 0;
> >>>    }
> >>>
> >>
> >>
> >> --
> >> Thanks,
> >> Anatoly
> 
> 
> --
> Thanks,
> Anatoly

Re: [dpdk-dev] [PATCH v2] timer: fix resource leak in finalize

[Carrillo, Erik G]

> -----Original Message-----
> From: Burakov, Anatoly
> Sent: Wednesday, May 8, 2019 3:50 AM
> To: Carrillo, Erik G <erik.g.carrillo@intel.com>; rsanford@akamai.com;
> thomas@monjalon.net
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v2] timer: fix resource leak in finalize
> 
> On 07-May-19 11:04 PM, Carrillo, Erik G wrote:
> > Hi Anatoly,
> >
> > Thanks for the review.  Comments in-line:
> >
> > <...snipped...>
> >
> >>>    #define RTE_MAX_DATA_ELS 64
> >>> +static const struct rte_memzone *rte_timer_data_mz; static
> >>> +rte_atomic16_t *rte_timer_mz_refcnt;
> >>>    static struct rte_timer_data *rte_timer_data_arr;
> >>>    static const uint32_t default_data_id;
> >>>    static uint32_t rte_timer_subsystem_initialized; @@ -155,6 +157,7
> >>> @@
> >>> rte_timer_subsystem_init_v1905(void)
> >>>    	struct rte_timer_data *data;
> >>>    	int i, lcore_id;
> >>>    	static const char *mz_name = "rte_timer_mz";
> >>> +	size_t data_arr_size = RTE_MAX_DATA_ELS *
> >>> +sizeof(*rte_timer_data_arr);
> >>
> >> nitpicking, but... const?
> >>
> >
> > No problem - I'll make this change if this line persists into the next version.
> >
> > <...snipped...>
> >
> >>>
> >>> @@ -205,8 +216,11 @@
> >> 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;
> >>> +
> >>> +	if (rte_atomic16_dec_and_test(rte_timer_mz_refcnt))
> >>> +		rte_memzone_free(rte_timer_data_mz);
> >>
> >> I think there's a race here. You may get preempted after test but
> >> before free, where another secondary could initialize. As far as i
> >> know, we also
> >
> > Indeed, thanks for catching this.
> >
> >> support a case when secondary initializes after primary stops running.
> >>
> >> Let's even suppose that we allow secondary processes to initialize
> >> the timer subsystem by reserving memzone and checking rte_errno. You
> >> would still have a chance of two init/deinit conflicting, because
> >> there's a hole between memzone allocation and atomic increment.
> >>
> >> I don't think this race can be resolved in a safe way, so we might
> >> just have to settle for a memory leak.
> >>
> >
> > I don't see a solution here currently either.  I'll look at removing
> > the memzone_free() call and possibly the
> > rte_timer_subsystem_finalize() API, since it seems like there's no reason
> for it to exist if it can't free the allocations.
> 
> I wonder if there are other places in DPDK where this pattern is used.
> 
> Technically, this kind of thing /could/ be resolved by having something in our
> multiprocess shared memory outside of DPDK heap. I.e. store something in
> rte_eal_memconfig like some other things do. This change, however, would
> require an ABI break, so while changing this particular API won't need a
> deprecation notice, the change itself would.

I like this idea; thanks for the suggestion.  I went ahead and submitted a new version 
of this patch with this approach.  It's dependent on one other new patch that allows
secondary processes to reserve the memzone.  I also submitted a deprecation
notice for the ABI break for the change to rte_mem_config.

Thomas, I suspect that I should mark v3 of this patch as "deferred", so I've gone ahead and
done that, but let me know if that's wrong.

Thanks,
Erik

> 
> >
> > Regards,
> > Erik
> >
> >>>
> >>>    	rte_timer_subsystem_initialized = 0;
> >>>    }
> >>>
> >>
> >>
> >> --
> >> Thanks,
> >> Anatoly
> 
> 
> --
> Thanks,
> Anatoly

Re: [dpdk-dev] [PATCH v2] timer: fix resource leak in finalize

[Thomas Monjalon]

09/05/2019 01:01, Carrillo, Erik G:
> I like this idea; thanks for the suggestion.  I went ahead and submitted a new version 
> of this patch with this approach.  It's dependent on one other new patch that allows
> secondary processes to reserve the memzone.  I also submitted a deprecation
> notice for the ABI break for the change to rte_mem_config.
> 
> Thomas, I suspect that I should mark v3 of this patch as "deferred", so I've gone ahead and
> done that, but let me know if that's wrong.

Any patch targetting 19.08 should be marked as deferred, thanks.

Re: [dpdk-dev] [PATCH v2] timer: fix resource leak in finalize

[Thomas Monjalon]

09/05/2019 01:01, Carrillo, Erik G:
> I like this idea; thanks for the suggestion.  I went ahead and submitted a new version 
> of this patch with this approach.  It's dependent on one other new patch that allows
> secondary processes to reserve the memzone.  I also submitted a deprecation
> notice for the ABI break for the change to rte_mem_config.
> 
> Thomas, I suspect that I should mark v3 of this patch as "deferred", so I've gone ahead and
> done that, but let me know if that's wrong.

Any patch targetting 19.08 should be marked as deferred, thanks.

Re: [dpdk-dev] [PATCH v3] timer: fix resource leak in finalize

[Burakov, Anatoly]

On 08-May-19 11:35 PM, Erik Gabriel Carrillo wrote:
> By using a lock added to the rte_mem_config (which lives in shared
> memory), we can synchronize multiple processes in init/finalize and
> safely free allocations made during init.
> 
> Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> ---
> changes in v3:
>   - The previous version had race condition.  This version fixes the race
>     by adding a lock in shared memory outside of the DPDK heap area
>     that can be used to safely free the memzone reserved in the subsystem
>     init call. (Anatoly)
> 
>     This patch depends on http://patches.dpdk.org/patch/53333/.
>   
> changes in v2:
>   - Handle scenario where primary process exits before secondaries such
>     that memzone is not freed early (Anatoly)
> 
>   lib/librte_eal/common/include/rte_eal_memconfig.h |  3 +++
>   lib/librte_timer/rte_timer.c                      | 23 ++++++++++++++++++++++-
>   2 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_eal/common/include/rte_eal_memconfig.h b/lib/librte_eal/common/include/rte_eal_memconfig.h
> index 84aabe3..8cbc09c 100644
> --- a/lib/librte_eal/common/include/rte_eal_memconfig.h
> +++ b/lib/librte_eal/common/include/rte_eal_memconfig.h
> @@ -64,6 +64,9 @@ struct rte_mem_config {
>   	rte_rwlock_t memory_hotplug_lock;
>   	/**< indicates whether memory hotplug request is in progress. */
>   
> +	rte_spinlock_t timer_subsystem_lock;
> +	/**< indicates whether timer subsystem init/finalize is in progress. */
> +

I believe there's an initialize function somewhere which will initialize 
all of these locks. This lock should be in there as well.

Other than that, i'm OK with this change, however i feel like /just/ 
adding this would be a missed opportunity, because next time we want to 
add something here it will be an ABI break again.

We could perhaps use this opportunity to leave some padding for future 
data. I'm not sure how would that look like, just an idea floating in my 
head :)

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v3] timer: fix resource leak in finalize

[Burakov, Anatoly]

On 08-May-19 11:35 PM, Erik Gabriel Carrillo wrote:
> By using a lock added to the rte_mem_config (which lives in shared
> memory), we can synchronize multiple processes in init/finalize and
> safely free allocations made during init.
> 
> Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> ---
> changes in v3:
>   - The previous version had race condition.  This version fixes the race
>     by adding a lock in shared memory outside of the DPDK heap area
>     that can be used to safely free the memzone reserved in the subsystem
>     init call. (Anatoly)
> 
>     This patch depends on http://patches.dpdk.org/patch/53333/.
>   
> changes in v2:
>   - Handle scenario where primary process exits before secondaries such
>     that memzone is not freed early (Anatoly)
> 
>   lib/librte_eal/common/include/rte_eal_memconfig.h |  3 +++
>   lib/librte_timer/rte_timer.c                      | 23 ++++++++++++++++++++++-
>   2 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_eal/common/include/rte_eal_memconfig.h b/lib/librte_eal/common/include/rte_eal_memconfig.h
> index 84aabe3..8cbc09c 100644
> --- a/lib/librte_eal/common/include/rte_eal_memconfig.h
> +++ b/lib/librte_eal/common/include/rte_eal_memconfig.h
> @@ -64,6 +64,9 @@ struct rte_mem_config {
>   	rte_rwlock_t memory_hotplug_lock;
>   	/**< indicates whether memory hotplug request is in progress. */
>   
> +	rte_spinlock_t timer_subsystem_lock;
> +	/**< indicates whether timer subsystem init/finalize is in progress. */
> +

I believe there's an initialize function somewhere which will initialize 
all of these locks. This lock should be in there as well.

Other than that, i'm OK with this change, however i feel like /just/ 
adding this would be a missed opportunity, because next time we want to 
add something here it will be an ABI break again.

We could perhaps use this opportunity to leave some padding for future 
data. I'm not sure how would that look like, just an idea floating in my 
head :)

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v3] timer: fix resource leak in finalize

[Thomas Monjalon]

09/05/2019 10:29, Burakov, Anatoly:
> On 08-May-19 11:35 PM, Erik Gabriel Carrillo wrote:
> > By using a lock added to the rte_mem_config (which lives in shared
> > memory), we can synchronize multiple processes in init/finalize and
> > safely free allocations made during init.
> > 
> > Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> > ---
> > changes in v3:
> >   - The previous version had race condition.  This version fixes the race
> >     by adding a lock in shared memory outside of the DPDK heap area
> >     that can be used to safely free the memzone reserved in the subsystem
> >     init call. (Anatoly)
> > 
> >     This patch depends on http://patches.dpdk.org/patch/53333/.
> >   
> > changes in v2:
> >   - Handle scenario where primary process exits before secondaries such
> >     that memzone is not freed early (Anatoly)
> > 
> >   lib/librte_eal/common/include/rte_eal_memconfig.h |  3 +++
> >   lib/librte_timer/rte_timer.c                      | 23 ++++++++++++++++++++++-
> >   2 files changed, 25 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/librte_eal/common/include/rte_eal_memconfig.h b/lib/librte_eal/common/include/rte_eal_memconfig.h
> > index 84aabe3..8cbc09c 100644
> > --- a/lib/librte_eal/common/include/rte_eal_memconfig.h
> > +++ b/lib/librte_eal/common/include/rte_eal_memconfig.h
> > @@ -64,6 +64,9 @@ struct rte_mem_config {
> >   	rte_rwlock_t memory_hotplug_lock;
> >   	/**< indicates whether memory hotplug request is in progress. */
> >   
> > +	rte_spinlock_t timer_subsystem_lock;
> > +	/**< indicates whether timer subsystem init/finalize is in progress. */
> > +
> 
> I believe there's an initialize function somewhere which will initialize 
> all of these locks. This lock should be in there as well.
> 
> Other than that, i'm OK with this change, however i feel like /just/ 
> adding this would be a missed opportunity, because next time we want to 
> add something here it will be an ABI break again.
> 
> We could perhaps use this opportunity to leave some padding for future 
> data. I'm not sure how would that look like, just an idea floating in my 
> head :)

Any update or other opinion?

Re: [dpdk-dev] [PATCH v3] timer: fix resource leak in finalize

[Burakov, Anatoly]

On 05-Jun-19 10:33 AM, Thomas Monjalon wrote:
> 09/05/2019 10:29, Burakov, Anatoly:
>> On 08-May-19 11:35 PM, Erik Gabriel Carrillo wrote:
>>> By using a lock added to the rte_mem_config (which lives in shared
>>> memory), we can synchronize multiple processes in init/finalize and
>>> safely free allocations made during init.
>>>
>>> Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
>>> ---
>>> changes in v3:
>>>    - The previous version had race condition.  This version fixes the race
>>>      by adding a lock in shared memory outside of the DPDK heap area
>>>      that can be used to safely free the memzone reserved in the subsystem
>>>      init call. (Anatoly)
>>>
>>>      This patch depends on http://patches.dpdk.org/patch/53333/.
>>>    
>>> changes in v2:
>>>    - Handle scenario where primary process exits before secondaries such
>>>      that memzone is not freed early (Anatoly)
>>>
>>>    lib/librte_eal/common/include/rte_eal_memconfig.h |  3 +++
>>>    lib/librte_timer/rte_timer.c                      | 23 ++++++++++++++++++++++-
>>>    2 files changed, 25 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/librte_eal/common/include/rte_eal_memconfig.h b/lib/librte_eal/common/include/rte_eal_memconfig.h
>>> index 84aabe3..8cbc09c 100644
>>> --- a/lib/librte_eal/common/include/rte_eal_memconfig.h
>>> +++ b/lib/librte_eal/common/include/rte_eal_memconfig.h
>>> @@ -64,6 +64,9 @@ struct rte_mem_config {
>>>    	rte_rwlock_t memory_hotplug_lock;
>>>    	/**< indicates whether memory hotplug request is in progress. */
>>>    
>>> +	rte_spinlock_t timer_subsystem_lock;
>>> +	/**< indicates whether timer subsystem init/finalize is in progress. */
>>> +
>>
>> I believe there's an initialize function somewhere which will initialize
>> all of these locks. This lock should be in there as well.
>>
>> Other than that, i'm OK with this change, however i feel like /just/
>> adding this would be a missed opportunity, because next time we want to
>> add something here it will be an ABI break again.
>>
>> We could perhaps use this opportunity to leave some padding for future
>> data. I'm not sure how would that look like, just an idea floating in my
>> head :)
> 
> Any update or other opinion?
> 

I have a patchset that hides mem config - if we are to add a lock, it 
would be after adding that patch, as a bugfix (since by then it would n 
longer be an ABI breakage or an API change).

-- 
Thanks,
Anatoly

[dpdk-dev] [PATCH 0/2] Fix timer resource leak

[Anatoly Burakov]

Previous attempts [1] at fixing the resource leak have been
deemed unsuccessful because of limitations around what
can be done without breaking the ABI. Now that we've
broken the EAL ABI, we can fix this issue properly.

This patchset is adding a new lock API, as well as fixes the
actual issue.

The patchset dependson the FreeBSD fixes [2], as well as the
mem config patchset [3].

[1] http://patches.dpdk.org/patch/53334/
[2] http://patches.dpdk.org/project/dpdk/list/?series=5161
[3] http://patches.dpdk.org/project/dpdk/list/?series=5162

Anatoly Burakov (2):
  eal: add internal locks for timer lib into EAL
  timer: fix resource leak in finalize

 lib/librte_eal/common/eal_common_mcfg.c       | 14 +++++++
 lib/librte_eal/common/eal_memcfg.h            |  2 +
 .../common/include/rte_eal_memconfig.h        | 18 ++++++++
 lib/librte_eal/rte_eal_version.map            |  2 +
 lib/librte_timer/rte_timer.c                  | 41 +++++++++++++------
 lib/librte_timer/rte_timer.h                  |  5 ++-
 6 files changed, 67 insertions(+), 15 deletions(-)

-- 
2.17.1

[dpdk-dev] [PATCH 1/2] eal: add internal locks for timer lib into EAL

[Anatoly Burakov]

Currently, timer library has a memory leak because there is no
way to concurrently initialize/deinitialize shared memory because
of race conditions [1].

Add a spinlock to the shared mem config to have a way to
exclusively initialize/deinitialize the timer library without
any races.

[1] See the following email thread:
https://mails.dpdk.org/archives/dev/2019-May/131498.html

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/common/eal_common_mcfg.c        | 14 ++++++++++++++
 lib/librte_eal/common/eal_memcfg.h             |  2 ++
 .../common/include/rte_eal_memconfig.h         | 18 ++++++++++++++++++
 lib/librte_eal/rte_eal_version.map             |  2 ++
 4 files changed, 36 insertions(+)

diff --git a/lib/librte_eal/common/eal_common_mcfg.c b/lib/librte_eal/common/eal_common_mcfg.c
index 1825d9083..066549432 100644
--- a/lib/librte_eal/common/eal_common_mcfg.c
+++ b/lib/librte_eal/common/eal_common_mcfg.c
@@ -147,3 +147,17 @@ rte_mcfg_mempool_write_unlock(void)
 	struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
 	rte_rwlock_write_unlock(&mcfg->mplock);
 }
+
+void
+rte_mcfg_timer_lock(void)
+{
+	struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
+	rte_spinlock_lock(&mcfg->tlock);
+}
+
+void
+rte_mcfg_timer_unlock(void)
+{
+	struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
+	rte_spinlock_unlock(&mcfg->tlock);
+}
diff --git a/lib/librte_eal/common/eal_memcfg.h b/lib/librte_eal/common/eal_memcfg.h
index e1aae32df..00370cece 100644
--- a/lib/librte_eal/common/eal_memcfg.h
+++ b/lib/librte_eal/common/eal_memcfg.h
@@ -11,6 +11,7 @@
 #include <rte_memory.h>
 #include <rte_memzone.h>
 #include <rte_pause.h>
+#include <rte_spinlock.h>
 #include <rte_rwlock.h>
 #include <rte_tailq.h>
 
@@ -38,6 +39,7 @@ struct rte_mem_config {
 	rte_rwlock_t mlock;   /**< only used by memzone LIB for thread-safe. */
 	rte_rwlock_t qlock;   /**< used for tailq operation for thread safe. */
 	rte_rwlock_t mplock;  /**< only used by mempool LIB for thread-safe. */
+	rte_spinlock_t tlock; /**< needed for timer lib thread safety. */
 
 	rte_rwlock_t memory_hotplug_lock;
 	/**< indicates whether memory hotplug request is in progress. */
diff --git a/lib/librte_eal/common/include/rte_eal_memconfig.h b/lib/librte_eal/common/include/rte_eal_memconfig.h
index 1b615c892..05a32e12a 100644
--- a/lib/librte_eal/common/include/rte_eal_memconfig.h
+++ b/lib/librte_eal/common/include/rte_eal_memconfig.h
@@ -109,6 +109,24 @@ rte_mcfg_mempool_write_lock(void);
 void
 rte_mcfg_mempool_write_unlock(void);
 
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Lock the internal EAL Timer Library lock for exclusive access.
+ */
+void __rte_experimental
+rte_mcfg_timer_lock(void);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Unlock the internal EAL Timer Library lock for exclusive access.
+ */
+void __rte_experimental
+rte_mcfg_timer_unlock(void);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
index c28951f65..bc08fc4df 100644
--- a/lib/librte_eal/rte_eal_version.map
+++ b/lib/librte_eal/rte_eal_version.map
@@ -367,6 +367,8 @@ EXPERIMENTAL {
 	rte_malloc_heap_memory_detach;
 	rte_malloc_heap_memory_remove;
 	rte_malloc_heap_socket_is_external;
+	rte_mcfg_timer_lock;
+	rte_mcfg_timer_unlock;
 	rte_mem_alloc_validator_register;
 	rte_mem_alloc_validator_unregister;
 	rte_mem_check_dma_mask;
-- 
2.17.1

[dpdk-dev] [PATCH 2/2] timer: fix resource leak in finalize

[Anatoly Burakov]

Currently, whenever timer library is initialized, the memory is leaked
because there is no telling when primary or secondary processes get
to use the state, and there is no way to initialize/deinitialize
timer library state without race conditions because the data itself
must live in shared memory.

However, there is now a timer library lock in the shared memory
config, which can be used to synchronize access to the timer
library shared memory. Use it to initialize/deinitialize timer
library shared data in a safe way. There is still a way to leak
the memory (by killing one of the processes), but we can't do
anything about that.

Also, update the API doc. Note that the behavior of the API
itself did not change - the requirement to call init in every
process was simply not documented explicitly.

Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_timer/rte_timer.c | 41 ++++++++++++++++++++++++------------
 lib/librte_timer/rte_timer.h |  5 +++--
 2 files changed, 31 insertions(+), 15 deletions(-)

diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
index dd7953922..08517c120 100644
--- a/lib/librte_timer/rte_timer.c
+++ b/lib/librte_timer/rte_timer.c
@@ -13,6 +13,7 @@
 #include <rte_atomic.h>
 #include <rte_common.h>
 #include <rte_cycles.h>
+#include <rte_eal_memconfig.h>
 #include <rte_per_lcore.h>
 #include <rte_memory.h>
 #include <rte_launch.h>
@@ -61,6 +62,8 @@ struct rte_timer_data {
 };
 
 #define RTE_MAX_DATA_ELS 64
+static const struct rte_memzone *rte_timer_data_mz;
+static int *volatile rte_timer_mz_refcnt;
 static struct rte_timer_data *rte_timer_data_arr;
 static const uint32_t default_data_id;
 static uint32_t rte_timer_subsystem_initialized;
@@ -157,28 +160,30 @@ rte_timer_subsystem_init_v1905(void)
 	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);
+			RTE_MAX_DATA_ELS * sizeof(*rte_timer_data_arr);
+	const size_t mem_size = data_arr_size + sizeof(*rte_timer_mz_refcnt);
 	bool do_full_init = true;
 
 	if (rte_timer_subsystem_initialized)
 		return -EALREADY;
 
-reserve:
-	rte_errno = 0;
-	mz = rte_memzone_reserve_aligned(mz_name, data_arr_size, SOCKET_ID_ANY,
-					 0, RTE_CACHE_LINE_SIZE);
+	rte_mcfg_timer_lock();
+
+	mz = rte_memzone_lookup(mz_name);
 	if (mz == NULL) {
-		if (rte_errno == EEXIST) {
-			mz = rte_memzone_lookup(mz_name);
-			if (mz == NULL)
-				goto reserve;
-
-			do_full_init = false;
-		} else
+		mz = rte_memzone_reserve_aligned(mz_name, mem_size,
+				SOCKET_ID_ANY, 0, RTE_CACHE_LINE_SIZE);
+		if (mz == NULL) {
+			rte_mcfg_timer_unlock();
 			return -ENOMEM;
-	}
+		}
+		do_full_init = true;
+	} else
+		do_full_init = false;
 
+	rte_timer_data_mz = mz;
 	rte_timer_data_arr = mz->addr;
+	rte_timer_mz_refcnt = (void *)((char *)mz->addr + data_arr_size);
 
 	if (do_full_init) {
 		for (i = 0; i < RTE_MAX_DATA_ELS; i++) {
@@ -195,6 +200,9 @@ rte_timer_subsystem_init_v1905(void)
 	}
 
 	rte_timer_data_arr[default_data_id].internal_flags |= FL_ALLOCATED;
+	(*rte_timer_mz_refcnt)++;
+
+	rte_mcfg_timer_unlock();
 
 	rte_timer_subsystem_initialized = 1;
 
@@ -210,6 +218,13 @@ rte_timer_subsystem_finalize(void)
 	if (!rte_timer_subsystem_initialized)
 		return;
 
+	rte_mcfg_timer_lock();
+
+	if (--(*rte_timer_mz_refcnt) == 0)
+		rte_memzone_free(rte_timer_data_mz);
+
+	rte_mcfg_timer_unlock();
+
 	rte_timer_subsystem_initialized = 0;
 }
 
diff --git a/lib/librte_timer/rte_timer.h b/lib/librte_timer/rte_timer.h
index 2196934b2..a7af10176 100644
--- a/lib/librte_timer/rte_timer.h
+++ b/lib/librte_timer/rte_timer.h
@@ -170,10 +170,11 @@ int __rte_experimental rte_timer_data_dealloc(uint32_t id);
  * Initializes internal variables (list, locks and so on) for the RTE
  * timer library.
  *
+ * @note
+ *   This function must be called in every process before using the library.
+ *
  * @return
  *   - 0: Success
- *   - -EEXIST: Returned in secondary process when primary process has not
- *      yet initialized the timer subsystem
  *   - -ENOMEM: Unable to allocate memory needed to initialize timer
  *      subsystem
  */
-- 
2.17.1

Re: [dpdk-dev] [PATCH 1/2] eal: add internal locks for timer lib into EAL

[Carrillo, Erik G]

Hi Anatoly,

> -----Original Message-----
> From: Burakov, Anatoly
> Sent: Tuesday, June 25, 2019 11:12 AM
> To: dev@dpdk.org
> Cc: Carrillo, Erik G <erik.g.carrillo@intel.com>
> Subject: [PATCH 1/2] eal: add internal locks for timer lib into EAL
> 
> Currently, timer library has a memory leak because there is no way to
> concurrently initialize/deinitialize shared memory because of race conditions
> [1].
> 
> Add a spinlock to the shared mem config to have a way to exclusively
> initialize/deinitialize the timer library without any races.
> 
> [1] See the following email thread:
> https://mails.dpdk.org/archives/dev/2019-May/131498.html
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>

<... snipped ...>

> --- a/lib/librte_eal/common/include/rte_eal_memconfig.h
> +++ b/lib/librte_eal/common/include/rte_eal_memconfig.h
> @@ -109,6 +109,24 @@ rte_mcfg_mempool_write_lock(void);
>  void
>  rte_mcfg_mempool_write_unlock(void);
> 
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Lock the internal EAL Timer Library lock for exclusive access.
> + */
> +void __rte_experimental

Depending on the decision made for the following thread, the "__rte_experimental" tag location may move:
https://mails.dpdk.org/archives/dev/2019-June/136050.html

> +rte_mcfg_timer_lock(void);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Unlock the internal EAL Timer Library lock for exclusive access.
> + */
> +void __rte_experimental
> +rte_mcfg_timer_unlock(void);
> +
>  #ifdef __cplusplus
>  }
>  #endif

<... snipped ...>

Other than that:

Acked-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>

Re: [dpdk-dev] [PATCH 2/2] timer: fix resource leak in finalize

[Carrillo, Erik G]

> -----Original Message-----
> From: Burakov, Anatoly
> Sent: Tuesday, June 25, 2019 11:12 AM
> To: dev@dpdk.org
> Cc: Robert Sanford <rsanford@akamai.com>; Carrillo, Erik G
> <erik.g.carrillo@intel.com>
> Subject: [PATCH 2/2] timer: fix resource leak in finalize
> 
> Currently, whenever timer library is initialized, the memory is leaked because
> there is no telling when primary or secondary processes get to use the state,
> and there is no way to initialize/deinitialize timer library state without race
> conditions because the data itself must live in shared memory.
> 
> However, there is now a timer library lock in the shared memory config,
> which can be used to synchronize access to the timer library shared memory.
> Use it to initialize/deinitialize timer library shared data in a safe way. There is
> still a way to leak the memory (by killing one of the processes), but we can't
> do anything about that.
> 
> Also, update the API doc. Note that the behavior of the API itself did not
> change - the requirement to call init in every process was simply not
> documented explicitly.
> 
> Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Acked-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>

Re: [dpdk-dev] [PATCH 1/2] eal: add internal locks for timer lib into EAL

[David Marchand]

On Tue, Jun 25, 2019 at 6:12 PM Anatoly Burakov <anatoly.burakov@intel.com>
wrote:

> Currently, timer library has a memory leak because there is no
> way to concurrently initialize/deinitialize shared memory because
> of race conditions [1].
>
> Add a spinlock to the shared mem config to have a way to
> exclusively initialize/deinitialize the timer library without
> any races.
>
> [1] See the following email thread:
> https://mails.dpdk.org/archives/dev/2019-May/131498.html
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>  lib/librte_eal/common/eal_common_mcfg.c        | 14 ++++++++++++++
>  lib/librte_eal/common/eal_memcfg.h             |  2 ++
>  .../common/include/rte_eal_memconfig.h         | 18 ++++++++++++++++++
>  lib/librte_eal/rte_eal_version.map             |  2 ++
>  4 files changed, 36 insertions(+)
>
> diff --git a/lib/librte_eal/common/eal_common_mcfg.c
> b/lib/librte_eal/common/eal_common_mcfg.c
> index 1825d9083..066549432 100644
> --- a/lib/librte_eal/common/eal_common_mcfg.c
> +++ b/lib/librte_eal/common/eal_common_mcfg.c
> @@ -147,3 +147,17 @@ rte_mcfg_mempool_write_unlock(void)
>         struct rte_mem_config *mcfg =
> rte_eal_get_configuration()->mem_config;
>         rte_rwlock_write_unlock(&mcfg->mplock);
>  }
> +
> +void
> +rte_mcfg_timer_lock(void)
> +{
> +       struct rte_mem_config *mcfg =
> rte_eal_get_configuration()->mem_config;
> +       rte_spinlock_lock(&mcfg->tlock);
> +}
> +
> +void
> +rte_mcfg_timer_unlock(void)
> +{
> +       struct rte_mem_config *mcfg =
> rte_eal_get_configuration()->mem_config;
> +       rte_spinlock_unlock(&mcfg->tlock);
> +}
> diff --git a/lib/librte_eal/common/eal_memcfg.h
> b/lib/librte_eal/common/eal_memcfg.h
> index e1aae32df..00370cece 100644
> --- a/lib/librte_eal/common/eal_memcfg.h
> +++ b/lib/librte_eal/common/eal_memcfg.h
> @@ -11,6 +11,7 @@
>  #include <rte_memory.h>
>  #include <rte_memzone.h>
>  #include <rte_pause.h>
> +#include <rte_spinlock.h>
>  #include <rte_rwlock.h>
>  #include <rte_tailq.h>
>
> @@ -38,6 +39,7 @@ struct rte_mem_config {
>         rte_rwlock_t mlock;   /**< only used by memzone LIB for
> thread-safe. */
>         rte_rwlock_t qlock;   /**< used for tailq operation for thread
> safe. */
>         rte_rwlock_t mplock;  /**< only used by mempool LIB for
> thread-safe. */
> +       rte_spinlock_t tlock; /**< needed for timer lib thread safety. */
>

>         rte_rwlock_t memory_hotplug_lock;
>         /**< indicates whether memory hotplug request is in progress. */
> diff --git a/lib/librte_eal/common/include/rte_eal_memconfig.h
> b/lib/librte_eal/common/include/rte_eal_memconfig.h
> index 1b615c892..05a32e12a 100644
> --- a/lib/librte_eal/common/include/rte_eal_memconfig.h
> +++ b/lib/librte_eal/common/include/rte_eal_memconfig.h
> @@ -109,6 +109,24 @@ rte_mcfg_mempool_write_lock(void);
>  void
>  rte_mcfg_mempool_write_unlock(void);
>
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Lock the internal EAL Timer Library lock for exclusive access.
> + */
> +void __rte_experimental
>

As mentionned by Erik,

rte_experimental
void

+rte_mcfg_timer_lock(void);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Unlock the internal EAL Timer Library lock for exclusive access.
> + */
> +void __rte_experimental
>

rte_experimental
void



> +rte_mcfg_timer_unlock(void);
> +
>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/lib/librte_eal/rte_eal_version.map
> b/lib/librte_eal/rte_eal_version.map
> index c28951f65..bc08fc4df 100644
> --- a/lib/librte_eal/rte_eal_version.map
> +++ b/lib/librte_eal/rte_eal_version.map
> @@ -367,6 +367,8 @@ EXPERIMENTAL {
>         rte_malloc_heap_memory_detach;
>         rte_malloc_heap_memory_remove;
>         rte_malloc_heap_socket_is_external;
> +       rte_mcfg_timer_lock;
> +       rte_mcfg_timer_unlock;
>         rte_mem_alloc_validator_register;
>         rte_mem_alloc_validator_unregister;
>         rte_mem_check_dma_mask;
> --
> 2.17.1
>

Can you put this at the end of the EXPERIMENTAL block under the 19.08
comment?


-- 
David Marchand

Re: [dpdk-dev] [PATCH 2/2] timer: fix resource leak in finalize

[David Marchand]

On Tue, Jun 25, 2019 at 6:12 PM Anatoly Burakov <anatoly.burakov@intel.com>
wrote:

> Currently, whenever timer library is initialized, the memory is leaked
> because there is no telling when primary or secondary processes get
> to use the state, and there is no way to initialize/deinitialize
> timer library state without race conditions because the data itself
> must live in shared memory.
>
> However, there is now a timer library lock in the shared memory
> config, which can be used to synchronize access to the timer
> library shared memory. Use it to initialize/deinitialize timer
> library shared data in a safe way. There is still a way to leak
> the memory (by killing one of the processes), but we can't do
> anything about that.
>
> Also, update the API doc. Note that the behavior of the API
> itself did not change - the requirement to call init in every
> process was simply not documented explicitly.
>
> Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>  lib/librte_timer/rte_timer.c | 41 ++++++++++++++++++++++++------------
>  lib/librte_timer/rte_timer.h |  5 +++--
>  2 files changed, 31 insertions(+), 15 deletions(-)
>
> diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
> index dd7953922..08517c120 100644
> --- a/lib/librte_timer/rte_timer.c
> +++ b/lib/librte_timer/rte_timer.c
> @@ -13,6 +13,7 @@
>  #include <rte_atomic.h>
>  #include <rte_common.h>
>  #include <rte_cycles.h>
> +#include <rte_eal_memconfig.h>
>  #include <rte_per_lcore.h>
>  #include <rte_memory.h>
>  #include <rte_launch.h>
> @@ -61,6 +62,8 @@ struct rte_timer_data {
>  };
>
>  #define RTE_MAX_DATA_ELS 64
> +static const struct rte_memzone *rte_timer_data_mz;
> +static int *volatile rte_timer_mz_refcnt;
>  static struct rte_timer_data *rte_timer_data_arr;
>  static const uint32_t default_data_id;
>  static uint32_t rte_timer_subsystem_initialized;
> @@ -157,28 +160,30 @@ rte_timer_subsystem_init_v1905(void)
>         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);
> +                       RTE_MAX_DATA_ELS * sizeof(*rte_timer_data_arr);
> +       const size_t mem_size = data_arr_size +
> sizeof(*rte_timer_mz_refcnt);
>         bool do_full_init = true;
>
>         if (rte_timer_subsystem_initialized)
>                 return -EALREADY;
>
> -reserve:
> -       rte_errno = 0;
> -       mz = rte_memzone_reserve_aligned(mz_name, data_arr_size,
> SOCKET_ID_ANY,
> -                                        0, RTE_CACHE_LINE_SIZE);
> +       rte_mcfg_timer_lock();
> +
> +       mz = rte_memzone_lookup(mz_name);
>         if (mz == NULL) {
> -               if (rte_errno == EEXIST) {
> -                       mz = rte_memzone_lookup(mz_name);
> -                       if (mz == NULL)
> -                               goto reserve;
> -
> -                       do_full_init = false;
> -               } else
> +               mz = rte_memzone_reserve_aligned(mz_name, mem_size,
> +                               SOCKET_ID_ANY, 0, RTE_CACHE_LINE_SIZE);
> +               if (mz == NULL) {
> +                       rte_mcfg_timer_unlock();
>                         return -ENOMEM;
> -       }
> +               }
> +               do_full_init = true;
> +       } else
> +               do_full_init = false;
>
> +       rte_timer_data_mz = mz;
>         rte_timer_data_arr = mz->addr;
> +       rte_timer_mz_refcnt = (void *)((char *)mz->addr + data_arr_size);
>
>         if (do_full_init) {
>                 for (i = 0; i < RTE_MAX_DATA_ELS; i++) {
> @@ -195,6 +200,9 @@ rte_timer_subsystem_init_v1905(void)
>         }
>
>         rte_timer_data_arr[default_data_id].internal_flags |= FL_ALLOCATED;
> +       (*rte_timer_mz_refcnt)++;
> +
> +       rte_mcfg_timer_unlock();
>
>         rte_timer_subsystem_initialized = 1;
>
> @@ -210,6 +218,13 @@ rte_timer_subsystem_finalize(void)
>         if (!rte_timer_subsystem_initialized)
>                 return;
>
> +       rte_mcfg_timer_lock();
> +
> +       if (--(*rte_timer_mz_refcnt) == 0)
> +               rte_memzone_free(rte_timer_data_mz);
> +
> +       rte_mcfg_timer_unlock();
> +
>         rte_timer_subsystem_initialized = 0;
>  }
>
> diff --git a/lib/librte_timer/rte_timer.h b/lib/librte_timer/rte_timer.h
> index 2196934b2..a7af10176 100644
> --- a/lib/librte_timer/rte_timer.h
> +++ b/lib/librte_timer/rte_timer.h
> @@ -170,10 +170,11 @@ int __rte_experimental
> rte_timer_data_dealloc(uint32_t id);
>   * Initializes internal variables (list, locks and so on) for the RTE
>   * timer library.
>   *
> + * @note
> + *   This function must be called in every process before using the
> library.
> + *
>   * @return
>   *   - 0: Success
> - *   - -EEXIST: Returned in secondary process when primary process has not
> - *      yet initialized the timer subsystem
>   *   - -ENOMEM: Unable to allocate memory needed to initialize timer
>   *      subsystem
>   */
> --
> 2.17.1
>

Can be squashed with the first patch.


-- 
David Marchand

Re: [dpdk-dev] [PATCH 1/2] eal: add internal locks for timer lib into EAL

[Burakov, Anatoly]

On 04-Jul-19 10:09 AM, David Marchand wrote:
> On Tue, Jun 25, 2019 at 6:12 PM Anatoly Burakov <anatoly.burakov@intel.com>
> wrote:
> 
>> Currently, timer library has a memory leak because there is no
>> way to concurrently initialize/deinitialize shared memory because
>> of race conditions [1].
>>
>> Add a spinlock to the shared mem config to have a way to
>> exclusively initialize/deinitialize the timer library without
>> any races.
>>
>> [1] See the following email thread:
>> https://mails.dpdk.org/archives/dev/2019-May/131498.html
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>> ---
>>   lib/librte_eal/common/eal_common_mcfg.c        | 14 ++++++++++++++
>>   lib/librte_eal/common/eal_memcfg.h             |  2 ++
>>   .../common/include/rte_eal_memconfig.h         | 18 ++++++++++++++++++
>>   lib/librte_eal/rte_eal_version.map             |  2 ++
>>   4 files changed, 36 insertions(+)
>>
>> diff --git a/lib/librte_eal/common/eal_common_mcfg.c
>> b/lib/librte_eal/common/eal_common_mcfg.c
>> index 1825d9083..066549432 100644
>> --- a/lib/librte_eal/common/eal_common_mcfg.c
>> +++ b/lib/librte_eal/common/eal_common_mcfg.c
>> @@ -147,3 +147,17 @@ rte_mcfg_mempool_write_unlock(void)
>>          struct rte_mem_config *mcfg =
>> rte_eal_get_configuration()->mem_config;
>>          rte_rwlock_write_unlock(&mcfg->mplock);
>>   }
>> +
>> +void
>> +rte_mcfg_timer_lock(void)
>> +{
>> +       struct rte_mem_config *mcfg =
>> rte_eal_get_configuration()->mem_config;
>> +       rte_spinlock_lock(&mcfg->tlock);
>> +}
>> +
>> +void
>> +rte_mcfg_timer_unlock(void)
>> +{
>> +       struct rte_mem_config *mcfg =
>> rte_eal_get_configuration()->mem_config;
>> +       rte_spinlock_unlock(&mcfg->tlock);
>> +}
>> diff --git a/lib/librte_eal/common/eal_memcfg.h
>> b/lib/librte_eal/common/eal_memcfg.h
>> index e1aae32df..00370cece 100644
>> --- a/lib/librte_eal/common/eal_memcfg.h
>> +++ b/lib/librte_eal/common/eal_memcfg.h
>> @@ -11,6 +11,7 @@
>>   #include <rte_memory.h>
>>   #include <rte_memzone.h>
>>   #include <rte_pause.h>
>> +#include <rte_spinlock.h>
>>   #include <rte_rwlock.h>
>>   #include <rte_tailq.h>
>>
>> @@ -38,6 +39,7 @@ struct rte_mem_config {
>>          rte_rwlock_t mlock;   /**< only used by memzone LIB for
>> thread-safe. */
>>          rte_rwlock_t qlock;   /**< used for tailq operation for thread
>> safe. */
>>          rte_rwlock_t mplock;  /**< only used by mempool LIB for
>> thread-safe. */
>> +       rte_spinlock_t tlock; /**< needed for timer lib thread safety. */
>>
> 
>>          rte_rwlock_t memory_hotplug_lock;
>>          /**< indicates whether memory hotplug request is in progress. */
>> diff --git a/lib/librte_eal/common/include/rte_eal_memconfig.h
>> b/lib/librte_eal/common/include/rte_eal_memconfig.h
>> index 1b615c892..05a32e12a 100644
>> --- a/lib/librte_eal/common/include/rte_eal_memconfig.h
>> +++ b/lib/librte_eal/common/include/rte_eal_memconfig.h
>> @@ -109,6 +109,24 @@ rte_mcfg_mempool_write_lock(void);
>>   void
>>   rte_mcfg_mempool_write_unlock(void);
>>
>> +/**
>> + * @warning
>> + * @b EXPERIMENTAL: this API may change without prior notice
>> + *
>> + * Lock the internal EAL Timer Library lock for exclusive access.
>> + */
>> +void __rte_experimental
>>
> 
> As mentionned by Erik,
> 
> rte_experimental
> void
> 
> +rte_mcfg_timer_lock(void);
>> +
>> +/**
>> + * @warning
>> + * @b EXPERIMENTAL: this API may change without prior notice
>> + *
>> + * Unlock the internal EAL Timer Library lock for exclusive access.
>> + */
>> +void __rte_experimental
>>
> 
> rte_experimental
> void
> 
> 
> 
>> +rte_mcfg_timer_unlock(void);
>> +
>>   #ifdef __cplusplus
>>   }
>>   #endif
>> diff --git a/lib/librte_eal/rte_eal_version.map
>> b/lib/librte_eal/rte_eal_version.map
>> index c28951f65..bc08fc4df 100644
>> --- a/lib/librte_eal/rte_eal_version.map
>> +++ b/lib/librte_eal/rte_eal_version.map
>> @@ -367,6 +367,8 @@ EXPERIMENTAL {
>>          rte_malloc_heap_memory_detach;
>>          rte_malloc_heap_memory_remove;
>>          rte_malloc_heap_socket_is_external;
>> +       rte_mcfg_timer_lock;
>> +       rte_mcfg_timer_unlock;
>>          rte_mem_alloc_validator_register;
>>          rte_mem_alloc_validator_unregister;
>>          rte_mem_check_dma_mask;
>> --
>> 2.17.1
>>
> 
> Can you put this at the end of the EXPERIMENTAL block under the 19.08
> comment?
> 

OK, will do both for v2.

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH 2/2] timer: fix resource leak in finalize

[Burakov, Anatoly]

On 04-Jul-19 10:10 AM, David Marchand wrote:
> On Tue, Jun 25, 2019 at 6:12 PM Anatoly Burakov <anatoly.burakov@intel.com>
> wrote:
> 
>> Currently, whenever timer library is initialized, the memory is leaked
>> because there is no telling when primary or secondary processes get
>> to use the state, and there is no way to initialize/deinitialize
>> timer library state without race conditions because the data itself
>> must live in shared memory.
>>
>> However, there is now a timer library lock in the shared memory
>> config, which can be used to synchronize access to the timer
>> library shared memory. Use it to initialize/deinitialize timer
>> library shared data in a safe way. There is still a way to leak
>> the memory (by killing one of the processes), but we can't do
>> anything about that.
>>
>> Also, update the API doc. Note that the behavior of the API
>> itself did not change - the requirement to call init in every
>> process was simply not documented explicitly.
>>
>> Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>> ---
>>   lib/librte_timer/rte_timer.c | 41 ++++++++++++++++++++++++------------
>>   lib/librte_timer/rte_timer.h |  5 +++--
>>   2 files changed, 31 insertions(+), 15 deletions(-)
>>
>> diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
>> index dd7953922..08517c120 100644
>> --- a/lib/librte_timer/rte_timer.c
>> +++ b/lib/librte_timer/rte_timer.c
>> @@ -13,6 +13,7 @@
>>   #include <rte_atomic.h>
>>   #include <rte_common.h>
>>   #include <rte_cycles.h>
>> +#include <rte_eal_memconfig.h>
>>   #include <rte_per_lcore.h>
>>   #include <rte_memory.h>
>>   #include <rte_launch.h>
>> @@ -61,6 +62,8 @@ struct rte_timer_data {
>>   };
>>
>>   #define RTE_MAX_DATA_ELS 64
>> +static const struct rte_memzone *rte_timer_data_mz;
>> +static int *volatile rte_timer_mz_refcnt;
>>   static struct rte_timer_data *rte_timer_data_arr;
>>   static const uint32_t default_data_id;
>>   static uint32_t rte_timer_subsystem_initialized;
>> @@ -157,28 +160,30 @@ rte_timer_subsystem_init_v1905(void)
>>          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);
>> +                       RTE_MAX_DATA_ELS * sizeof(*rte_timer_data_arr);
>> +       const size_t mem_size = data_arr_size +
>> sizeof(*rte_timer_mz_refcnt);
>>          bool do_full_init = true;
>>
>>          if (rte_timer_subsystem_initialized)
>>                  return -EALREADY;
>>
>> -reserve:
>> -       rte_errno = 0;
>> -       mz = rte_memzone_reserve_aligned(mz_name, data_arr_size,
>> SOCKET_ID_ANY,
>> -                                        0, RTE_CACHE_LINE_SIZE);
>> +       rte_mcfg_timer_lock();
>> +
>> +       mz = rte_memzone_lookup(mz_name);
>>          if (mz == NULL) {
>> -               if (rte_errno == EEXIST) {
>> -                       mz = rte_memzone_lookup(mz_name);
>> -                       if (mz == NULL)
>> -                               goto reserve;
>> -
>> -                       do_full_init = false;
>> -               } else
>> +               mz = rte_memzone_reserve_aligned(mz_name, mem_size,
>> +                               SOCKET_ID_ANY, 0, RTE_CACHE_LINE_SIZE);
>> +               if (mz == NULL) {
>> +                       rte_mcfg_timer_unlock();
>>                          return -ENOMEM;
>> -       }
>> +               }
>> +               do_full_init = true;
>> +       } else
>> +               do_full_init = false;
>>
>> +       rte_timer_data_mz = mz;
>>          rte_timer_data_arr = mz->addr;
>> +       rte_timer_mz_refcnt = (void *)((char *)mz->addr + data_arr_size);
>>
>>          if (do_full_init) {
>>                  for (i = 0; i < RTE_MAX_DATA_ELS; i++) {
>> @@ -195,6 +200,9 @@ rte_timer_subsystem_init_v1905(void)
>>          }
>>
>>          rte_timer_data_arr[default_data_id].internal_flags |= FL_ALLOCATED;
>> +       (*rte_timer_mz_refcnt)++;
>> +
>> +       rte_mcfg_timer_unlock();
>>
>>          rte_timer_subsystem_initialized = 1;
>>
>> @@ -210,6 +218,13 @@ rte_timer_subsystem_finalize(void)
>>          if (!rte_timer_subsystem_initialized)
>>                  return;
>>
>> +       rte_mcfg_timer_lock();
>> +
>> +       if (--(*rte_timer_mz_refcnt) == 0)
>> +               rte_memzone_free(rte_timer_data_mz);
>> +
>> +       rte_mcfg_timer_unlock();
>> +
>>          rte_timer_subsystem_initialized = 0;
>>   }
>>
>> diff --git a/lib/librte_timer/rte_timer.h b/lib/librte_timer/rte_timer.h
>> index 2196934b2..a7af10176 100644
>> --- a/lib/librte_timer/rte_timer.h
>> +++ b/lib/librte_timer/rte_timer.h
>> @@ -170,10 +170,11 @@ int __rte_experimental
>> rte_timer_data_dealloc(uint32_t id);
>>    * Initializes internal variables (list, locks and so on) for the RTE
>>    * timer library.
>>    *
>> + * @note
>> + *   This function must be called in every process before using the
>> library.
>> + *
>>    * @return
>>    *   - 0: Success
>> - *   - -EEXIST: Returned in secondary process when primary process has not
>> - *      yet initialized the timer subsystem
>>    *   - -ENOMEM: Unable to allocate memory needed to initialize timer
>>    *      subsystem
>>    */
>> --
>> 2.17.1
>>
> 
> Can be squashed with the first patch.
> 

This is not just search-and-replace - this is also a bugfix. I can 
squash the search-and-replace part with the first patch, but this commit 
will have to stay IMO.

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH 2/2] timer: fix resource leak in finalize

[David Marchand]

On Thu, Jul 4, 2019 at 12:45 PM Burakov, Anatoly <anatoly.burakov@intel.com>
wrote:

> On 04-Jul-19 10:10 AM, David Marchand wrote:
> > On Tue, Jun 25, 2019 at 6:12 PM Anatoly Burakov <
> anatoly.burakov@intel.com>
> > wrote:
> >
> >> Currently, whenever timer library is initialized, the memory is leaked
> >> because there is no telling when primary or secondary processes get
> >> to use the state, and there is no way to initialize/deinitialize
> >> timer library state without race conditions because the data itself
> >> must live in shared memory.
> >>
> >> However, there is now a timer library lock in the shared memory
> >> config, which can be used to synchronize access to the timer
> >> library shared memory. Use it to initialize/deinitialize timer
> >> library shared data in a safe way. There is still a way to leak
> >> the memory (by killing one of the processes), but we can't do
> >> anything about that.
> >>
> >> Also, update the API doc. Note that the behavior of the API
> >> itself did not change - the requirement to call init in every
> >> process was simply not documented explicitly.
> >>
> >> Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> >> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> >> ---
> >>   lib/librte_timer/rte_timer.c | 41 ++++++++++++++++++++++++------------
> >>   lib/librte_timer/rte_timer.h |  5 +++--
> >>   2 files changed, 31 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
> >> index dd7953922..08517c120 100644
> >> --- a/lib/librte_timer/rte_timer.c
> >> +++ b/lib/librte_timer/rte_timer.c
> >> @@ -13,6 +13,7 @@
> >>   #include <rte_atomic.h>
> >>   #include <rte_common.h>
> >>   #include <rte_cycles.h>
> >> +#include <rte_eal_memconfig.h>
> >>   #include <rte_per_lcore.h>
> >>   #include <rte_memory.h>
> >>   #include <rte_launch.h>
> >> @@ -61,6 +62,8 @@ struct rte_timer_data {
> >>   };
> >>
> >>   #define RTE_MAX_DATA_ELS 64
> >> +static const struct rte_memzone *rte_timer_data_mz;
> >> +static int *volatile rte_timer_mz_refcnt;
> >>   static struct rte_timer_data *rte_timer_data_arr;
> >>   static const uint32_t default_data_id;
> >>   static uint32_t rte_timer_subsystem_initialized;
> >> @@ -157,28 +160,30 @@ rte_timer_subsystem_init_v1905(void)
> >>          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);
> >> +                       RTE_MAX_DATA_ELS * sizeof(*rte_timer_data_arr);
> >> +       const size_t mem_size = data_arr_size +
> >> sizeof(*rte_timer_mz_refcnt);
> >>          bool do_full_init = true;
> >>
> >>          if (rte_timer_subsystem_initialized)
> >>                  return -EALREADY;
> >>
> >> -reserve:
> >> -       rte_errno = 0;
> >> -       mz = rte_memzone_reserve_aligned(mz_name, data_arr_size,
> >> SOCKET_ID_ANY,
> >> -                                        0, RTE_CACHE_LINE_SIZE);
> >> +       rte_mcfg_timer_lock();
> >> +
> >> +       mz = rte_memzone_lookup(mz_name);
> >>          if (mz == NULL) {
> >> -               if (rte_errno == EEXIST) {
> >> -                       mz = rte_memzone_lookup(mz_name);
> >> -                       if (mz == NULL)
> >> -                               goto reserve;
> >> -
> >> -                       do_full_init = false;
> >> -               } else
> >> +               mz = rte_memzone_reserve_aligned(mz_name, mem_size,
> >> +                               SOCKET_ID_ANY, 0, RTE_CACHE_LINE_SIZE);
> >> +               if (mz == NULL) {
> >> +                       rte_mcfg_timer_unlock();
> >>                          return -ENOMEM;
> >> -       }
> >> +               }
> >> +               do_full_init = true;
> >> +       } else
> >> +               do_full_init = false;
> >>
> >> +       rte_timer_data_mz = mz;
> >>          rte_timer_data_arr = mz->addr;
> >> +       rte_timer_mz_refcnt = (void *)((char *)mz->addr +
> data_arr_size);
> >>
> >>          if (do_full_init) {
> >>                  for (i = 0; i < RTE_MAX_DATA_ELS; i++) {
> >> @@ -195,6 +200,9 @@ rte_timer_subsystem_init_v1905(void)
> >>          }
> >>
> >>          rte_timer_data_arr[default_data_id].internal_flags |=
> FL_ALLOCATED;
> >> +       (*rte_timer_mz_refcnt)++;
> >> +
> >> +       rte_mcfg_timer_unlock();
> >>
> >>          rte_timer_subsystem_initialized = 1;
> >>
> >> @@ -210,6 +218,13 @@ rte_timer_subsystem_finalize(void)
> >>          if (!rte_timer_subsystem_initialized)
> >>                  return;
> >>
> >> +       rte_mcfg_timer_lock();
> >> +
> >> +       if (--(*rte_timer_mz_refcnt) == 0)
> >> +               rte_memzone_free(rte_timer_data_mz);
> >> +
> >> +       rte_mcfg_timer_unlock();
> >> +
> >>          rte_timer_subsystem_initialized = 0;
> >>   }
> >>
> >> diff --git a/lib/librte_timer/rte_timer.h b/lib/librte_timer/rte_timer.h
> >> index 2196934b2..a7af10176 100644
> >> --- a/lib/librte_timer/rte_timer.h
> >> +++ b/lib/librte_timer/rte_timer.h
> >> @@ -170,10 +170,11 @@ int __rte_experimental
> >> rte_timer_data_dealloc(uint32_t id);
> >>    * Initializes internal variables (list, locks and so on) for the RTE
> >>    * timer library.
> >>    *
> >> + * @note
> >> + *   This function must be called in every process before using the
> >> library.
> >> + *
> >>    * @return
> >>    *   - 0: Success
> >> - *   - -EEXIST: Returned in secondary process when primary process has
> not
> >> - *      yet initialized the timer subsystem
> >>    *   - -ENOMEM: Unable to allocate memory needed to initialize timer
> >>    *      subsystem
> >>    */
> >> --
> >> 2.17.1
> >>
> >
> > Can be squashed with the first patch.
> >
>
> This is not just search-and-replace - this is also a bugfix. I can
> squash the search-and-replace part with the first patch, but this commit
> will have to stay IMO.
>

Yes this is not search and replace, but the first patch is there for the
bugfix.
There are no other uses of the newly introduced API, so the whole is a
single change to me.

Apart from that, I am ok with the changes, you can add my review tag.


-- 
David Marchand

[dpdk-dev] [PATCH v2 0/1] Fix timer resource leak

[Anatoly Burakov]

Previous attempts [1] at fixing the resource leak have been
deemed unsuccessful because of limitations around what
can be done without breaking the ABI. Now that we've
broken the EAL ABI, we can fix this issue properly.

This patchset is adding a new lock API, as well as fixes the
actual issue.

The patchset depends on mem config patchset [2].

[1] http://patches.dpdk.org/patch/53334/
[2] http://patches.dpdk.org/project/dpdk/list/?series=5369

Anatoly Burakov (1):
  timer: fix resource leak in finalize

 lib/librte_eal/common/eal_common_mcfg.c       | 29 +++++++++++++
 lib/librte_eal/common/eal_memcfg.h            |  8 ++++
 .../common/include/rte_eal_memconfig.h        | 22 ++++++++++
 lib/librte_eal/freebsd/eal/eal.c              |  5 ++-
 lib/librte_eal/linux/eal/eal.c                |  5 ++-
 lib/librte_eal/rte_eal_version.map            |  2 +
 lib/librte_timer/rte_timer.c                  | 41 +++++++++++++------
 lib/librte_timer/rte_timer.h                  |  5 ++-
 8 files changed, 100 insertions(+), 17 deletions(-)

-- 
2.17.1

[dpdk-dev] [PATCH v2 1/1] timer: fix resource leak in finalize

[Anatoly Burakov]

Currently, whenever timer library is initialized, the memory
is leaked because there is no telling when primary or secondary
processes get to use the state, and there is no way to
initialize/deinitialize timer library state without race
conditions [1] because the data itself must live in shared memory.

Add a spinlock to the shared mem config to have a way to
exclusively initialize/deinitialize the timer library without
any races, and implement the synchronization mechanism based
on this lock in the timer library.

Also, update the API doc. Note that the behavior of the API
itself did not change - the requirement to call init in every
process was simply not documented explicitly.

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

[1] See the following email thread:
https://mails.dpdk.org/archives/dev/2019-May/131498.html

Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Acked-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
Reviewed-by: David Marchand <david.marchand@redhat.com>
---
 lib/librte_eal/common/eal_common_mcfg.c       | 29 +++++++++++++
 lib/librte_eal/common/eal_memcfg.h            |  8 ++++
 .../common/include/rte_eal_memconfig.h        | 22 ++++++++++
 lib/librte_eal/freebsd/eal/eal.c              |  5 ++-
 lib/librte_eal/linux/eal/eal.c                |  5 ++-
 lib/librte_eal/rte_eal_version.map            |  2 +
 lib/librte_timer/rte_timer.c                  | 41 +++++++++++++------
 lib/librte_timer/rte_timer.h                  |  5 ++-
 8 files changed, 100 insertions(+), 17 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_mcfg.c b/lib/librte_eal/common/eal_common_mcfg.c
index fe8d2b726..066549432 100644
--- a/lib/librte_eal/common/eal_common_mcfg.c
+++ b/lib/librte_eal/common/eal_common_mcfg.c
@@ -4,6 +4,7 @@
 
 #include <rte_config.h>
 #include <rte_eal_memconfig.h>
+#include <rte_version.h>
 
 #include "eal_internal_cfg.h"
 #include "eal_memcfg.h"
@@ -31,6 +32,18 @@ eal_mcfg_wait_complete(void)
 		rte_pause();
 }
 
+int
+eal_mcfg_check_version(void)
+{
+	struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
+
+	/* check if version from memconfig matches compiled in macro */
+	if (mcfg->version != RTE_VERSION)
+		return -1;
+
+	return 0;
+}
+
 void
 eal_mcfg_update_internal(void)
 {
@@ -47,6 +60,8 @@ eal_mcfg_update_from_internal(void)
 
 	mcfg->legacy_mem = internal_config.legacy_mem;
 	mcfg->single_file_segments = internal_config.single_file_segments;
+	/* record current DPDK version */
+	mcfg->version = RTE_VERSION;
 }
 
 void
@@ -132,3 +147,17 @@ rte_mcfg_mempool_write_unlock(void)
 	struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
 	rte_rwlock_write_unlock(&mcfg->mplock);
 }
+
+void
+rte_mcfg_timer_lock(void)
+{
+	struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
+	rte_spinlock_lock(&mcfg->tlock);
+}
+
+void
+rte_mcfg_timer_unlock(void)
+{
+	struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
+	rte_spinlock_unlock(&mcfg->tlock);
+}
diff --git a/lib/librte_eal/common/eal_memcfg.h b/lib/librte_eal/common/eal_memcfg.h
index 573233896..359beb216 100644
--- a/lib/librte_eal/common/eal_memcfg.h
+++ b/lib/librte_eal/common/eal_memcfg.h
@@ -11,6 +11,7 @@
 #include <rte_memory.h>
 #include <rte_memzone.h>
 #include <rte_pause.h>
+#include <rte_spinlock.h>
 #include <rte_rwlock.h>
 #include <rte_tailq.h>
 
@@ -19,6 +20,8 @@
  */
 struct rte_mem_config {
 	volatile uint32_t magic;   /**< Magic number - sanity check. */
+	uint32_t version;
+	/**< Prevent secondary processes using different DPDK versions. */
 
 	/* memory topology */
 	uint32_t nchannel;    /**< Number of channels (0 if unknown). */
@@ -34,6 +37,7 @@ struct rte_mem_config {
 	rte_rwlock_t mlock;   /**< used by memzones for thread safety. */
 	rte_rwlock_t qlock;   /**< used by tailqs for thread safety. */
 	rte_rwlock_t mplock;  /**< used by mempool library for thread safety. */
+	rte_spinlock_t tlock; /**< used by timer library for thread safety. */
 
 	rte_rwlock_t memory_hotplug_lock;
 	/**< Indicates whether memory hotplug request is in progress. */
@@ -81,6 +85,10 @@ eal_mcfg_update_from_internal(void);
 void
 eal_mcfg_wait_complete(void);
 
+/* check if DPDK version of current process matches one stored in the config */
+int
+eal_mcfg_check_version(void);
+
 /* set mem config as complete */
 void
 eal_mcfg_complete(void);
diff --git a/lib/librte_eal/common/include/rte_eal_memconfig.h b/lib/librte_eal/common/include/rte_eal_memconfig.h
index dc61a6fed..34b0e44a0 100644
--- a/lib/librte_eal/common/include/rte_eal_memconfig.h
+++ b/lib/librte_eal/common/include/rte_eal_memconfig.h
@@ -5,6 +5,8 @@
 #ifndef _RTE_EAL_MEMCONFIG_H_
 #define _RTE_EAL_MEMCONFIG_H_
 
+#include <rte_compat.h>
+
 /**
  * @file
  *
@@ -87,6 +89,26 @@ rte_mcfg_mempool_write_lock(void);
 void
 rte_mcfg_mempool_write_unlock(void);
 
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Lock the internal EAL Timer Library lock for exclusive access.
+ */
+__rte_experimental
+void
+rte_mcfg_timer_lock(void);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Unlock the internal EAL Timer Library lock for exclusive access.
+ */
+__rte_experimental
+void
+rte_mcfg_timer_unlock(void);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_eal/freebsd/eal/eal.c b/lib/librte_eal/freebsd/eal/eal.c
index ec1650c43..1d3d7e80f 100644
--- a/lib/librte_eal/freebsd/eal/eal.c
+++ b/lib/librte_eal/freebsd/eal/eal.c
@@ -385,6 +385,10 @@ rte_config_init(void)
 		if (rte_eal_config_attach() < 0)
 			return -1;
 		eal_mcfg_wait_complete();
+		if (eal_mcfg_check_version() < 0) {
+			RTE_LOG(ERR, EAL, "Primary and secondary process DPDK version mismatch\n");
+			return -1;
+		}
 		if (rte_eal_config_reattach() < 0)
 			return -1;
 		eal_mcfg_update_internal();
@@ -395,7 +399,6 @@ rte_config_init(void)
 			rte_config.process_type);
 		return -1;
 	}
-
 	return 0;
 }
 
diff --git a/lib/librte_eal/linux/eal/eal.c b/lib/librte_eal/linux/eal/eal.c
index 445d72f0c..a5e0d1e75 100644
--- a/lib/librte_eal/linux/eal/eal.c
+++ b/lib/librte_eal/linux/eal/eal.c
@@ -489,6 +489,10 @@ rte_config_init(void)
 		if (rte_eal_config_attach() < 0)
 			return -1;
 		eal_mcfg_wait_complete();
+		if (eal_mcfg_check_version() < 0) {
+			RTE_LOG(ERR, EAL, "Primary and secondary process DPDK version mismatch\n");
+			return -1;
+		}
 		if (rte_eal_config_reattach() < 0)
 			return -1;
 		eal_mcfg_update_internal();
@@ -499,7 +503,6 @@ rte_config_init(void)
 			rte_config.process_type);
 		return -1;
 	}
-
 	return 0;
 }
 
diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
index cc4ef04a0..d0ee67dbf 100644
--- a/lib/librte_eal/rte_eal_version.map
+++ b/lib/librte_eal/rte_eal_version.map
@@ -405,4 +405,6 @@ EXPERIMENTAL {
 	# added in 19.08
 	rte_lcore_cpuset;
 	rte_lcore_to_cpu_id;
+	rte_mcfg_timer_lock;
+	rte_mcfg_timer_unlock;
 };
diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
index eaeafd74f..71dffd23b 100644
--- a/lib/librte_timer/rte_timer.c
+++ b/lib/librte_timer/rte_timer.c
@@ -13,6 +13,7 @@
 #include <rte_atomic.h>
 #include <rte_common.h>
 #include <rte_cycles.h>
+#include <rte_eal_memconfig.h>
 #include <rte_per_lcore.h>
 #include <rte_memory.h>
 #include <rte_launch.h>
@@ -61,6 +62,8 @@ struct rte_timer_data {
 };
 
 #define RTE_MAX_DATA_ELS 64
+static const struct rte_memzone *rte_timer_data_mz;
+static int *volatile rte_timer_mz_refcnt;
 static struct rte_timer_data *rte_timer_data_arr;
 static const uint32_t default_data_id;
 static uint32_t rte_timer_subsystem_initialized;
@@ -157,28 +160,30 @@ rte_timer_subsystem_init_v1905(void)
 	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);
+			RTE_MAX_DATA_ELS * sizeof(*rte_timer_data_arr);
+	const size_t mem_size = data_arr_size + sizeof(*rte_timer_mz_refcnt);
 	bool do_full_init = true;
 
 	if (rte_timer_subsystem_initialized)
 		return -EALREADY;
 
-reserve:
-	rte_errno = 0;
-	mz = rte_memzone_reserve_aligned(mz_name, data_arr_size, SOCKET_ID_ANY,
-					 0, RTE_CACHE_LINE_SIZE);
+	rte_mcfg_timer_lock();
+
+	mz = rte_memzone_lookup(mz_name);
 	if (mz == NULL) {
-		if (rte_errno == EEXIST) {
-			mz = rte_memzone_lookup(mz_name);
-			if (mz == NULL)
-				goto reserve;
-
-			do_full_init = false;
-		} else
+		mz = rte_memzone_reserve_aligned(mz_name, mem_size,
+				SOCKET_ID_ANY, 0, RTE_CACHE_LINE_SIZE);
+		if (mz == NULL) {
+			rte_mcfg_timer_unlock();
 			return -ENOMEM;
-	}
+		}
+		do_full_init = true;
+	} else
+		do_full_init = false;
 
+	rte_timer_data_mz = mz;
 	rte_timer_data_arr = mz->addr;
+	rte_timer_mz_refcnt = (void *)((char *)mz->addr + data_arr_size);
 
 	if (do_full_init) {
 		for (i = 0; i < RTE_MAX_DATA_ELS; i++) {
@@ -195,6 +200,9 @@ rte_timer_subsystem_init_v1905(void)
 	}
 
 	rte_timer_data_arr[default_data_id].internal_flags |= FL_ALLOCATED;
+	(*rte_timer_mz_refcnt)++;
+
+	rte_mcfg_timer_unlock();
 
 	rte_timer_subsystem_initialized = 1;
 
@@ -210,6 +218,13 @@ rte_timer_subsystem_finalize(void)
 	if (!rte_timer_subsystem_initialized)
 		return;
 
+	rte_mcfg_timer_lock();
+
+	if (--(*rte_timer_mz_refcnt) == 0)
+		rte_memzone_free(rte_timer_data_mz);
+
+	rte_mcfg_timer_unlock();
+
 	rte_timer_subsystem_initialized = 0;
 }
 
diff --git a/lib/librte_timer/rte_timer.h b/lib/librte_timer/rte_timer.h
index 1b0d5f6a5..05d287d8f 100644
--- a/lib/librte_timer/rte_timer.h
+++ b/lib/librte_timer/rte_timer.h
@@ -172,10 +172,11 @@ int rte_timer_data_dealloc(uint32_t id);
  * Initializes internal variables (list, locks and so on) for the RTE
  * timer library.
  *
+ * @note
+ *   This function must be called in every process before using the library.
+ *
  * @return
  *   - 0: Success
- *   - -EEXIST: Returned in secondary process when primary process has not
- *      yet initialized the timer subsystem
  *   - -ENOMEM: Unable to allocate memory needed to initialize timer
  *      subsystem
  */
-- 
2.17.1

[dpdk-dev] [PATCH v3 0/1] Fix timer resource leak

[Anatoly Burakov]

Previous attempts [1] at fixing the resource leak have been
deemed unsuccessful because of limitations around what
can be done without breaking the ABI. Now that we've
broken the EAL ABI, we can fix this issue properly.

This patchset is adding a new lock API, as well as fixes the
actual issue.

The patchset depends on mem config patchset [2].

v3:
- Accidentally squashed with previous patch, reverted

v2:
- Addressed review comments and squashed patches

[1] http://patches.dpdk.org/patch/53334/
[2] http://patches.dpdk.org/project/dpdk/list/?series=5369

Anatoly Burakov (1):
  timer: fix resource leak in finalize

 lib/librte_eal/common/eal_common_mcfg.c       | 14 +++++++
 lib/librte_eal/common/eal_memcfg.h            |  2 +
 .../common/include/rte_eal_memconfig.h        | 22 ++++++++++
 lib/librte_eal/rte_eal_version.map            |  2 +
 lib/librte_timer/rte_timer.c                  | 41 +++++++++++++------
 lib/librte_timer/rte_timer.h                  |  5 ++-
 6 files changed, 71 insertions(+), 15 deletions(-)

-- 
2.17.1

[dpdk-dev] [PATCH v3 1/1] timer: fix resource leak in finalize

[Anatoly Burakov]

Currently, whenever timer library is initialized, the memory
is leaked because there is no telling when primary or secondary
processes get to use the state, and there is no way to
initialize/deinitialize timer library state without race
conditions [1] because the data itself must live in shared memory.

Add a spinlock to the shared mem config to have a way to
exclusively initialize/deinitialize the timer library without
any races, and implement the synchronization mechanism based
on this lock in the timer library.

Also, update the API doc. Note that the behavior of the API
itself did not change - the requirement to call init in every
process was simply not documented explicitly.

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

[1] See the following email thread:
https://mails.dpdk.org/archives/dev/2019-May/131498.html

Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Acked-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
Reviewed-by: David Marchand <david.marchand@redhat.com>
---
 lib/librte_eal/common/eal_common_mcfg.c       | 14 +++++++
 lib/librte_eal/common/eal_memcfg.h            |  2 +
 .../common/include/rte_eal_memconfig.h        | 22 ++++++++++
 lib/librte_eal/rte_eal_version.map            |  2 +
 lib/librte_timer/rte_timer.c                  | 41 +++++++++++++------
 lib/librte_timer/rte_timer.h                  |  5 ++-
 6 files changed, 71 insertions(+), 15 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_mcfg.c b/lib/librte_eal/common/eal_common_mcfg.c
index 1825d9083..066549432 100644
--- a/lib/librte_eal/common/eal_common_mcfg.c
+++ b/lib/librte_eal/common/eal_common_mcfg.c
@@ -147,3 +147,17 @@ rte_mcfg_mempool_write_unlock(void)
 	struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
 	rte_rwlock_write_unlock(&mcfg->mplock);
 }
+
+void
+rte_mcfg_timer_lock(void)
+{
+	struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
+	rte_spinlock_lock(&mcfg->tlock);
+}
+
+void
+rte_mcfg_timer_unlock(void)
+{
+	struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
+	rte_spinlock_unlock(&mcfg->tlock);
+}
diff --git a/lib/librte_eal/common/eal_memcfg.h b/lib/librte_eal/common/eal_memcfg.h
index 030f903ad..359beb216 100644
--- a/lib/librte_eal/common/eal_memcfg.h
+++ b/lib/librte_eal/common/eal_memcfg.h
@@ -11,6 +11,7 @@
 #include <rte_memory.h>
 #include <rte_memzone.h>
 #include <rte_pause.h>
+#include <rte_spinlock.h>
 #include <rte_rwlock.h>
 #include <rte_tailq.h>
 
@@ -36,6 +37,7 @@ struct rte_mem_config {
 	rte_rwlock_t mlock;   /**< used by memzones for thread safety. */
 	rte_rwlock_t qlock;   /**< used by tailqs for thread safety. */
 	rte_rwlock_t mplock;  /**< used by mempool library for thread safety. */
+	rte_spinlock_t tlock; /**< used by timer library for thread safety. */
 
 	rte_rwlock_t memory_hotplug_lock;
 	/**< Indicates whether memory hotplug request is in progress. */
diff --git a/lib/librte_eal/common/include/rte_eal_memconfig.h b/lib/librte_eal/common/include/rte_eal_memconfig.h
index dc61a6fed..34b0e44a0 100644
--- a/lib/librte_eal/common/include/rte_eal_memconfig.h
+++ b/lib/librte_eal/common/include/rte_eal_memconfig.h
@@ -5,6 +5,8 @@
 #ifndef _RTE_EAL_MEMCONFIG_H_
 #define _RTE_EAL_MEMCONFIG_H_
 
+#include <rte_compat.h>
+
 /**
  * @file
  *
@@ -87,6 +89,26 @@ rte_mcfg_mempool_write_lock(void);
 void
 rte_mcfg_mempool_write_unlock(void);
 
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Lock the internal EAL Timer Library lock for exclusive access.
+ */
+__rte_experimental
+void
+rte_mcfg_timer_lock(void);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Unlock the internal EAL Timer Library lock for exclusive access.
+ */
+__rte_experimental
+void
+rte_mcfg_timer_unlock(void);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
index cc4ef04a0..d0ee67dbf 100644
--- a/lib/librte_eal/rte_eal_version.map
+++ b/lib/librte_eal/rte_eal_version.map
@@ -405,4 +405,6 @@ EXPERIMENTAL {
 	# added in 19.08
 	rte_lcore_cpuset;
 	rte_lcore_to_cpu_id;
+	rte_mcfg_timer_lock;
+	rte_mcfg_timer_unlock;
 };
diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
index eaeafd74f..71dffd23b 100644
--- a/lib/librte_timer/rte_timer.c
+++ b/lib/librte_timer/rte_timer.c
@@ -13,6 +13,7 @@
 #include <rte_atomic.h>
 #include <rte_common.h>
 #include <rte_cycles.h>
+#include <rte_eal_memconfig.h>
 #include <rte_per_lcore.h>
 #include <rte_memory.h>
 #include <rte_launch.h>
@@ -61,6 +62,8 @@ struct rte_timer_data {
 };
 
 #define RTE_MAX_DATA_ELS 64
+static const struct rte_memzone *rte_timer_data_mz;
+static int *volatile rte_timer_mz_refcnt;
 static struct rte_timer_data *rte_timer_data_arr;
 static const uint32_t default_data_id;
 static uint32_t rte_timer_subsystem_initialized;
@@ -157,28 +160,30 @@ rte_timer_subsystem_init_v1905(void)
 	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);
+			RTE_MAX_DATA_ELS * sizeof(*rte_timer_data_arr);
+	const size_t mem_size = data_arr_size + sizeof(*rte_timer_mz_refcnt);
 	bool do_full_init = true;
 
 	if (rte_timer_subsystem_initialized)
 		return -EALREADY;
 
-reserve:
-	rte_errno = 0;
-	mz = rte_memzone_reserve_aligned(mz_name, data_arr_size, SOCKET_ID_ANY,
-					 0, RTE_CACHE_LINE_SIZE);
+	rte_mcfg_timer_lock();
+
+	mz = rte_memzone_lookup(mz_name);
 	if (mz == NULL) {
-		if (rte_errno == EEXIST) {
-			mz = rte_memzone_lookup(mz_name);
-			if (mz == NULL)
-				goto reserve;
-
-			do_full_init = false;
-		} else
+		mz = rte_memzone_reserve_aligned(mz_name, mem_size,
+				SOCKET_ID_ANY, 0, RTE_CACHE_LINE_SIZE);
+		if (mz == NULL) {
+			rte_mcfg_timer_unlock();
 			return -ENOMEM;
-	}
+		}
+		do_full_init = true;
+	} else
+		do_full_init = false;
 
+	rte_timer_data_mz = mz;
 	rte_timer_data_arr = mz->addr;
+	rte_timer_mz_refcnt = (void *)((char *)mz->addr + data_arr_size);
 
 	if (do_full_init) {
 		for (i = 0; i < RTE_MAX_DATA_ELS; i++) {
@@ -195,6 +200,9 @@ rte_timer_subsystem_init_v1905(void)
 	}
 
 	rte_timer_data_arr[default_data_id].internal_flags |= FL_ALLOCATED;
+	(*rte_timer_mz_refcnt)++;
+
+	rte_mcfg_timer_unlock();
 
 	rte_timer_subsystem_initialized = 1;
 
@@ -210,6 +218,13 @@ rte_timer_subsystem_finalize(void)
 	if (!rte_timer_subsystem_initialized)
 		return;
 
+	rte_mcfg_timer_lock();
+
+	if (--(*rte_timer_mz_refcnt) == 0)
+		rte_memzone_free(rte_timer_data_mz);
+
+	rte_mcfg_timer_unlock();
+
 	rte_timer_subsystem_initialized = 0;
 }
 
diff --git a/lib/librte_timer/rte_timer.h b/lib/librte_timer/rte_timer.h
index 1b0d5f6a5..05d287d8f 100644
--- a/lib/librte_timer/rte_timer.h
+++ b/lib/librte_timer/rte_timer.h
@@ -172,10 +172,11 @@ int rte_timer_data_dealloc(uint32_t id);
  * Initializes internal variables (list, locks and so on) for the RTE
  * timer library.
  *
+ * @note
+ *   This function must be called in every process before using the library.
+ *
  * @return
  *   - 0: Success
- *   - -EEXIST: Returned in secondary process when primary process has not
- *      yet initialized the timer subsystem
  *   - -ENOMEM: Unable to allocate memory needed to initialize timer
  *      subsystem
  */
-- 
2.17.1

Re: [dpdk-dev] [PATCH v3 1/1] timer: fix resource leak in finalize

[Thomas Monjalon]

05/07/2019 19:22, Anatoly Burakov:
> Currently, whenever timer library is initialized, the memory
> is leaked because there is no telling when primary or secondary
> processes get to use the state, and there is no way to
> initialize/deinitialize timer library state without race
> conditions [1] because the data itself must live in shared memory.
> 
> Add a spinlock to the shared mem config to have a way to
> exclusively initialize/deinitialize the timer library without
> any races, and implement the synchronization mechanism based
> on this lock in the timer library.
> 
> Also, update the API doc. Note that the behavior of the API
> itself did not change - the requirement to call init in every
> process was simply not documented explicitly.
> 
> Fixes: c0749f7096c7 ("timer: allow management in shared memory")
> 
> [1] See the following email thread:
> https://mails.dpdk.org/archives/dev/2019-May/131498.html
> 
> Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> Acked-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> Reviewed-by: David Marchand <david.marchand@redhat.com>

Applied, thanks