On 2024-02-22 10:42, Morten Brørup wrote:
From: Mattias Rönnblom [mailto:mattias.ronnb...@ericsson.com]
Sent: Tuesday, 20 February 2024 09.49
Replace static array of cache-aligned structs with an lcore variable,
to slightly benefit code simplicity and performance.
Signed-off-by: Mattias Rönnblom <mattias.ronnb...@ericsson.com>
---
@@ -486,8 +489,7 @@ service_runner_func(void *arg)
{
RTE_SET_USED(arg);
uint8_t i;
- const int lcore = rte_lcore_id();
- struct core_state *cs = &lcore_states[lcore];
+ struct core_state *cs = RTE_LCORE_VAR_PTR(lcore_states);
Typo: TAB -> SPACE.
Will fix.
rte_atomic_store_explicit(&cs->thread_active, 1,
rte_memory_order_seq_cst);
@@ -533,13 +535,16 @@ service_runner_func(void *arg)
int32_t
rte_service_lcore_may_be_active(uint32_t lcore)
{
- if (lcore >= RTE_MAX_LCORE || !lcore_states[lcore].is_service_core)
+ struct core_state *cs =
+ RTE_LCORE_VAR_LCORE_PTR(lcore, lcore_states);
+
+ if (lcore >= RTE_MAX_LCORE || !cs->is_service_core)
return -EINVAL;
This comment is mostly related to patch 1 in the series...
You are setting cs = RTE_LCORE_VAR_LCORE_PTR(lcore, ...) before validating that
lcore < RTE_MAX_LCORE. I wondered if that potentially was an overrun bug.
It is obvious when looking at the RTE_LCORE_VAR_LCORE_PTR() macro implementation, but
perhaps its description could mention that it is safe to use with an "invalid"
lcore_id, but not dereferencing it.
I thought about adding something equivalent to an RTE_ASSERT() on
lcore_id in the dereferencing macros, but then I thought that maybe it
is a valid use case to pass invalid lcore ids.
Invalid ids being OK or not, I think the above code should do "cs =
/../" *after* the lcore id check. Now it looks strange and force the
reader to consider if this is valid or not, for no good reason.
The lcore variable API docs should probably explicitly allow invalid
core id in the macros.
