From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <anatoly.burakov@intel.com>
Received: from mga12.intel.com (mga12.intel.com [192.55.52.136])
by dpdk.org (Postfix) with ESMTP id 892262BF2
for <dev@dpdk.org>; Thu, 2 May 2019 11:18:09 +0200 (CEST)
X-Amp-Result: SKIPPED(no attachment in message)
X-Amp-File-Uploaded: False
Received: from orsmga006.jf.intel.com ([10.7.209.51])
by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;
02 May 2019 02:18:08 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.60,421,1549958400"; d="scan'208";a="140620779"
Received: from aburakov-mobl1.ger.corp.intel.com (HELO [10.237.220.113])
([10.237.220.113])
by orsmga006.jf.intel.com with ESMTP; 02 May 2019 02:18:07 -0700
To: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>, rsanford@akamai.com,
thomas@monjalon.net
Cc: dev@dpdk.org
References: <1556737217-24338-1-git-send-email-erik.g.carrillo@intel.com>
From: "Burakov, Anatoly" <anatoly.burakov@intel.com>
Message-ID: <fcf4377c-ebbc-3778-b631-55c011431dba@intel.com>
Date: Thu, 2 May 2019 10:18:06 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
Thunderbird/60.6.1
MIME-Version: 1.0
In-Reply-To: <1556737217-24338-1-git-send-email-erik.g.carrillo@intel.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Subject: Re: [dpdk-dev] [PATCH] timer: fix resource leak in finalize
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Thu, 02 May 2019 09:18:10 -0000
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
From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from dpdk.org (dpdk.org [92.243.14.124])
by dpdk.space (Postfix) with ESMTP id 72821A0AC5
for <public@inbox.dpdk.org>; Thu, 2 May 2019 11:18:12 +0200 (CEST)
Received: from [92.243.14.124] (localhost [127.0.0.1])
by dpdk.org (Postfix) with ESMTP id 715BF2C4F;
Thu, 2 May 2019 11:18:11 +0200 (CEST)
Received: from mga12.intel.com (mga12.intel.com [192.55.52.136])
by dpdk.org (Postfix) with ESMTP id 892262BF2
for <dev@dpdk.org>; Thu, 2 May 2019 11:18:09 +0200 (CEST)
X-Amp-Result: SKIPPED(no attachment in message)
X-Amp-File-Uploaded: False
Received: from orsmga006.jf.intel.com ([10.7.209.51])
by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;
02 May 2019 02:18:08 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.60,421,1549958400"; d="scan'208";a="140620779"
Received: from aburakov-mobl1.ger.corp.intel.com (HELO [10.237.220.113])
([10.237.220.113])
by orsmga006.jf.intel.com with ESMTP; 02 May 2019 02:18:07 -0700
To: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>, rsanford@akamai.com,
thomas@monjalon.net
Cc: dev@dpdk.org
References: <1556737217-24338-1-git-send-email-erik.g.carrillo@intel.com>
From: "Burakov, Anatoly" <anatoly.burakov@intel.com>
Message-ID: <fcf4377c-ebbc-3778-b631-55c011431dba@intel.com>
Date: Thu, 2 May 2019 10:18:06 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
Thunderbird/60.6.1
MIME-Version: 1.0
In-Reply-To: <1556737217-24338-1-git-send-email-erik.g.carrillo@intel.com>
Content-Type: text/plain; charset="UTF-8"; format="flowed"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Subject: Re: [dpdk-dev] [PATCH] timer: fix resource leak in finalize
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>
Message-ID: <20190502091806.-oM-q-6zvfMXay9n5m7seHVwAoxzAjxgDBArFAX1GEM@z>
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