> -----Original Message----- > From: Honnappa Nagarahalli <[email protected]> > Sent: Saturday, May 2, 2020 1:03 AM > To: [email protected]; [email protected]; Van Haaren, Harry > <[email protected]> > Cc: [email protected]; [email protected]; Ananyev, Konstantin > <[email protected]>; [email protected]; > [email protected]; Eads, Gage <[email protected]>; Richardson, > Bruce <[email protected]>; [email protected]; > [email protected]; [email protected] > 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: [email protected] > 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 <[email protected]> > Reviewed-by: Phil Yang <[email protected]> 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 <[email protected]> > --- <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>
