> -----Original Message-----
> From: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>
> Sent: Saturday, May 2, 2020 1:03 AM
> To: dev@dpdk.org; phil.y...@arm.com; Van Haaren, Harry
> <harry.van.haa...@intel.com>
> Cc: tho...@monjalon.net; david.march...@redhat.com; Ananyev, Konstantin
> <konstantin.anan...@intel.com>; jer...@marvell.com;
> hemant.agra...@nxp.com; Eads, Gage <gage.e...@intel.com>; Richardson,
> Bruce <bruce.richard...@intel.com>; honnappa.nagaraha...@arm.com;
> n...@arm.com; sta...@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: sta...@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.nagaraha...@arm.com>
> Reviewed-by: Phil Yang <phil.y...@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.haa...@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>
