From mboxrd@z Thu Jan  1 00:00:00 1970
From: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
Subject: Re: [PATCH 1/2] service: fix del to reset lcore role to
 rte
Date: Thu, 4 Jan 2018 20:50:57 +0530
Message-ID: <20180104152056.ltzm2hsdelpsonto@Pavan-LT>
References: <1513768907-112647-1-git-send-email-harry.van.haaren@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: dev@dpdk.org
To: Harry van Haaren <harry.van.haaren@intel.com>
Return-path: <dev-bounces@dpdk.org>
Received: from NAM03-BY2-obe.outbound.protection.outlook.com
 (mail-by2nam03on0058.outbound.protection.outlook.com [104.47.42.58])
 by dpdk.org (Postfix) with ESMTP id B1ECF7CFC
 for <dev@dpdk.org>; Thu,  4 Jan 2018 16:21:20 +0100 (CET)
Content-Disposition: inline
In-Reply-To: <1513768907-112647-1-git-send-email-harry.van.haaren@intel.com>
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

Hi Harry,

Comments inline.

On Wed, Dec 20, 2017 at 11:21:46AM +0000, Harry van Haaren wrote:
> This patch fixes the reset of the service core,
> that when rte_service_lcore_del() is called, the
> lcore_role is restored to RTE.
>
> This issue was reported as when running the unit tests, an
> error was thrown that "failed to allocate lcore". Investigating
> revealed that the state of the service-cores after del() was
> not allowing a core to be re-used at a later point in time.
>
> Fixes: 21698354c832 ("service: introduce service cores concept")
> +CC stable@dpdk.org
>
> Reported-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
>
> ---
>
> @Stable maintainers; this is an EXPERIMENTAL tagged API, so I'm
> not sure what the expectation is in terms of backporting.
> ---
>  lib/librte_eal/common/rte_service.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
>
<snip>
>  int32_t rte_service_lcore_reset_all(void)
>  {
>  	/* loop over cores, reset all to mask 0 */
>  	uint32_t i;
>  	for (i = 0; i < RTE_MAX_LCORE; i++) {
>  		lcore_states[i].service_mask = 0;
> -		lcore_states[i].is_service_core = 0;
> +		set_lcore_state(i, ROLE_RTE);

Setting ROLE_RTE for RTE_MAX_LCORE lcores is incorrect. There should be a check
to set only service lcores something like this:

        for (i = 0; i < RTE_MAX_LCORE; i++) {
	-               lcore_states[i].service_mask = 0;
	-               set_lcore_state(i, ROLE_RTE);
	-               lcore_states[i].runstate = RUNSTATE_STOPPED;
	+               if (lcore_states[i].is_service_core) {
	+                       lcore_states[i].service_mask = 0;
	+                       set_lcore_state(i, ROLE_RTE);
	+                       lcore_states[i].runstate = RUNSTATE_STOPPED;
	+               }

Cheers,
Pavan.

>  		lcore_states[i].runstate = RUNSTATE_STOPPED;
>  	}
>  	for (i = 0; i < RTE_SERVICE_NUM_MAX; i++)
> @@ -600,20 +614,6 @@ int32_t rte_service_lcore_reset_all(void)
>  	return 0;
>  }
>
>  int32_t
>  rte_service_lcore_add(uint32_t lcore)
>  {
> --
> 2.7.4
>