[dpdk-stable] [PATCH v3 2/6] service: identify service running on another core correctly

[Van Haaren, Harry]

> -----Original Message-----
> From: Honnappa Nagarahalli <honnappa.nagarahalli at arm.com>
> Sent: Saturday, May 2, 2020 1:03 AM
> To: dev at dpdk.org; phil.yang at arm.com; Van Haaren, Harry
> <harry.van.haaren at intel.com>
> Cc: thomas at monjalon.net; david.marchand at redhat.com; Ananyev, Konstantin
> <konstantin.ananyev at intel.com>; jerinj at marvell.com;
> hemant.agrawal at nxp.com; Eads, Gage <gage.eads at intel.com>; Richardson,
> Bruce <bruce.richardson at intel.com>; honnappa.nagarahalli at arm.com;
> nd at arm.com; stable at dpdk.org
> Subject: [PATCH v3 2/6] service: identify service running on another core
> correctly
>
> The logic to identify if the MT unsafe service is running on another
> core can return -EBUSY spuriously. In such cases, running the service
> becomes costlier than using atomic operations. Assume that the
> application passes the right parameters and reduces the number of
> instructions for all cases.
> 
> Cc: stable at dpdk.org
> Fixes: 8d39d3e237c2 ("service: fix race in service on app lcore function")

Add "fix" to the title, suggestion:
service: fix identification of service running on other lcore

> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli at arm.com>
> Reviewed-by: Phil Yang <phil.yang at arm.com>

I believe there may be some optimizations we can apply after this patchset
as the "num_mapped_cores" variable is no longer used in a significant way 
for the atomic selection, however lets leave that optimization outside
of 20.05 scope.

With title (see above) & comment (see below) updated.
Acked-by: Harry van Haaren <harry.van.haaren at intel.com>

> ---
<snip some diff>
> @@ -412,24 +412,14 @@ rte_service_run_iter_on_app_lcore(uint32_t id,
> uint32_t serialize_mt_unsafe)
> 
>  	SERVICE_VALID_GET_OR_ERR_RET(id, s, -EINVAL);
> 
> -	/* Atomically add this core to the mapped cores first, then examine if
> -	 * we can run the service. This avoids a race condition between
> -	 * checking the value, and atomically adding to the mapped count.
> +	/* Increment num_mapped_cores to indicate that the service
> +	 * is running on a core.
>  	 */
> -	if (serialize_mt_unsafe)
> -		rte_atomic32_inc(&s->num_mapped_cores);
> +	rte_atomic32_inc(&s->num_mapped_cores);

The comment for the added lines here are a little confusing to me,
the "num_mapped_cores" does not indicate that the service "is running on a core",
it indicates the number of mapped lcores to that service. Suggestion below?

/* Increment num_mapped_cores to reflect that this core is
 * now mapped capable of running the service.
 */

> -	if (service_mt_safe(s) == 0 &&
> -			rte_atomic32_read(&s->num_mapped_cores) > 1) {
> -		if (serialize_mt_unsafe)
> -			rte_atomic32_dec(&s->num_mapped_cores);
> -		return -EBUSY;
> -	}
> -
> -	int ret = service_run(id, cs, UINT64_MAX, s);
> +	int ret = service_run(id, cs, UINT64_MAX, s, serialize_mt_unsafe);

<snip rest of diff>