From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id 892262BF2 for ; 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 , 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" Message-ID: 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-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 > --- > 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: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id 72821A0AC5 for ; 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 ; 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 , 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" Message-ID: 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > --- > 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