From: "Mattias Rönnblom" <hofors@lysator.liu.se>
To: "Morten Brørup" <mb@smartsharesystems.com>,
"Mattias Rönnblom" <mattias.ronnblom@ericsson.com>,
dev@dpdk.org
Cc: Stephen Hemminger <stephen@networkplumber.org>
Subject: Re: [RFC v3 5/6] service: keep per-lcore state in lcore variable
Date: Fri, 23 Feb 2024 11:19:05 +0100 [thread overview]
Message-ID: <cbb680ec-be3f-4ab1-a325-cdd5fffb8420@lysator.liu.se> (raw)
In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35E9F24A@smartserver.smartshare.dk>
On 2024-02-22 10:42, Morten Brørup wrote:
>> From: Mattias Rönnblom [mailto:mattias.ronnblom@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.ronnblom@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.
next prev parent reply other threads:[~2024-02-23 10:19 UTC|newest]
Thread overview: 72+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-08 18:16 [RFC 0/5] Lcore variables Mattias Rönnblom
2024-02-08 18:16 ` [RFC 1/5] eal: add static per-lcore memory allocation facility Mattias Rönnblom
2024-02-09 8:25 ` Morten Brørup
2024-02-09 11:46 ` Mattias Rönnblom
2024-02-09 13:04 ` Morten Brørup
2024-02-19 7:49 ` Mattias Rönnblom
2024-02-19 11:10 ` Morten Brørup
2024-02-19 14:31 ` Mattias Rönnblom
2024-02-19 15:04 ` Morten Brørup
2024-02-19 9:40 ` [RFC v2 0/5] Lcore variables Mattias Rönnblom
2024-02-19 9:40 ` [RFC v2 1/5] eal: add static per-lcore memory allocation facility Mattias Rönnblom
2024-02-20 8:49 ` [RFC v3 0/6] Lcore variables Mattias Rönnblom
2024-02-20 8:49 ` [RFC v3 1/6] eal: add static per-lcore memory allocation facility Mattias Rönnblom
2024-02-20 9:11 ` Bruce Richardson
2024-02-20 10:47 ` Mattias Rönnblom
2024-02-20 11:39 ` Bruce Richardson
2024-02-20 13:37 ` Morten Brørup
2024-02-20 16:26 ` Mattias Rönnblom
2024-02-21 9:43 ` Jerin Jacob
2024-02-21 10:31 ` Morten Brørup
2024-02-21 14:26 ` Mattias Rönnblom
2024-02-22 9:22 ` Morten Brørup
2024-02-23 10:12 ` Mattias Rönnblom
2024-02-25 15:03 ` [RFC v4 0/6] Lcore variables Mattias Rönnblom
2024-02-25 15:03 ` [RFC v4 1/6] eal: add static per-lcore memory allocation facility Mattias Rönnblom
2024-02-27 9:58 ` Morten Brørup
2024-02-27 13:44 ` Mattias Rönnblom
2024-02-27 15:05 ` Morten Brørup
2024-02-27 16:27 ` Mattias Rönnblom
2024-02-27 16:51 ` Morten Brørup
2024-02-28 10:09 ` [RFC v5 0/6] Lcore variables Mattias Rönnblom
2024-02-28 10:09 ` [RFC v5 1/6] eal: add static per-lcore memory allocation facility Mattias Rönnblom
2024-03-19 12:52 ` Konstantin Ananyev
2024-03-20 10:24 ` Mattias Rönnblom
2024-03-20 14:18 ` Konstantin Ananyev
2024-05-06 8:27 ` [RFC v6 0/6] Lcore variables Mattias Rönnblom
2024-05-06 8:27 ` [RFC v6 1/6] eal: add static per-lcore memory allocation facility Mattias Rönnblom
2024-05-06 8:27 ` [RFC v6 2/6] eal: add lcore variable test suite Mattias Rönnblom
2024-05-06 8:27 ` [RFC v6 3/6] random: keep PRNG state in lcore variable Mattias Rönnblom
2024-05-06 8:27 ` [RFC v6 4/6] power: keep per-lcore " Mattias Rönnblom
2024-05-06 8:27 ` [RFC v6 5/6] service: " Mattias Rönnblom
2024-05-06 8:27 ` [RFC v6 6/6] eal: keep per-lcore power intrinsics " Mattias Rönnblom
2024-02-28 10:09 ` [RFC v5 2/6] eal: add lcore variable test suite Mattias Rönnblom
2024-02-28 10:09 ` [RFC v5 3/6] random: keep PRNG state in lcore variable Mattias Rönnblom
2024-02-28 10:09 ` [RFC v5 4/6] power: keep per-lcore " Mattias Rönnblom
2024-02-28 10:09 ` [RFC v5 5/6] service: " Mattias Rönnblom
2024-02-28 10:09 ` [RFC v5 6/6] eal: keep per-lcore power intrinsics " Mattias Rönnblom
2024-02-25 15:03 ` [RFC v4 2/6] eal: add lcore variable test suite Mattias Rönnblom
2024-02-25 15:03 ` [RFC v4 3/6] random: keep PRNG state in lcore variable Mattias Rönnblom
2024-02-25 15:03 ` [RFC v4 4/6] power: keep per-lcore " Mattias Rönnblom
2024-02-25 15:03 ` [RFC v4 5/6] service: " Mattias Rönnblom
2024-02-25 16:28 ` Mattias Rönnblom
2024-02-25 15:03 ` [RFC v4 6/6] eal: keep per-lcore power intrinsics " Mattias Rönnblom
2024-02-20 8:49 ` [RFC v3 2/6] eal: add lcore variable test suite Mattias Rönnblom
2024-02-20 8:49 ` [RFC v3 3/6] random: keep PRNG state in lcore variable Mattias Rönnblom
2024-02-20 15:31 ` Morten Brørup
2024-02-20 8:49 ` [RFC v3 4/6] power: keep per-lcore " Mattias Rönnblom
2024-02-20 8:49 ` [RFC v3 5/6] service: " Mattias Rönnblom
2024-02-22 9:42 ` Morten Brørup
2024-02-23 10:19 ` Mattias Rönnblom [this message]
2024-02-20 8:49 ` [RFC v3 6/6] eal: keep per-lcore power intrinsics " Mattias Rönnblom
2024-02-19 9:40 ` [RFC v2 2/5] eal: add lcore variable test suite Mattias Rönnblom
2024-02-19 9:40 ` [RFC v2 3/5] random: keep PRNG state in lcore variable Mattias Rönnblom
2024-02-19 11:22 ` Morten Brørup
2024-02-19 14:04 ` Mattias Rönnblom
2024-02-19 15:10 ` Morten Brørup
2024-02-19 9:40 ` [RFC v2 4/5] power: keep per-lcore " Mattias Rönnblom
2024-02-19 9:40 ` [RFC v2 5/5] service: " Mattias Rönnblom
2024-02-08 18:16 ` [RFC 2/5] eal: add lcore variable test suite Mattias Rönnblom
2024-02-08 18:16 ` [RFC 3/5] random: keep PRNG state in lcore variable Mattias Rönnblom
2024-02-08 18:16 ` [RFC 4/5] power: keep per-lcore " Mattias Rönnblom
2024-02-08 18:16 ` [RFC 5/5] service: " Mattias Rönnblom
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cbb680ec-be3f-4ab1-a325-cdd5fffb8420@lysator.liu.se \
--to=hofors@lysator.liu.se \
--cc=dev@dpdk.org \
--cc=mattias.ronnblom@ericsson.com \
--cc=mb@smartsharesystems.com \
--cc=stephen@networkplumber.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).