From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
To: Harry van Haaren <harry.van.haaren@intel.com>,
"dev@dpdk.org" <dev@dpdk.org>
Cc: "david.marchand@redhat.com" <david.marchand@redhat.com>,
"igor.romanov@oktetlabs.ru" <igor.romanov@oktetlabs.ru>,
"ferruh.yigit@intel.com" <ferruh.yigit@intel.com>,
nd <nd@arm.com>, "aconole@redhat.com" <aconole@redhat.com>,
"l.wojciechow@partner.samsung.com"
<l.wojciechow@partner.samsung.com>,
Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>,
Phil Yang <Phil.Yang@arm.com>, nd <nd@arm.com>
Subject: Re: [dpdk-dev] [PATCH v2 1/2] service: add API to retrieve service core active
Date: Tue, 21 Jul 2020 19:43:05 +0000 [thread overview]
Message-ID: <DB6PR0802MB221605308636E4B7039B491798780@DB6PR0802MB2216.eurprd08.prod.outlook.com> (raw)
In-Reply-To: <20200720143829.46280-1-harry.van.haaren@intel.com>
<snip>
> Subject: [PATCH v2 1/2] service: add API to retrieve service core active
>
> This commit adds a new experimental API which allows the user to retrieve
> the active state of an lcore. Knowing when the service lcore is completed its
> polling loop can be useful to applications to avoid race conditions when e.g.
> finalizing statistics.
>
> The service thread itself now has a variable to indicate if its thread is active.
> When zero the service thread has completed its service, and has returned
> from the service_runner_func() function.
>
> Suggested-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
>
> ---
>
> Thanks for feedback Lukasz, please have a look and see if this was what you
> were expecting?
>
> Honnappa/Phil, are the __atomic_load/store's correct?
>
> ---
> lib/librte_eal/common/rte_service.c | 14 ++++++++++++++
> lib/librte_eal/include/rte_service.h | 9 +++++++++
> lib/librte_eal/rte_eal_version.map | 1 +
> 3 files changed, 24 insertions(+)
>
> diff --git a/lib/librte_eal/common/rte_service.c
> b/lib/librte_eal/common/rte_service.c
> index 6a0e0ff65..4c276b006 100644
> --- a/lib/librte_eal/common/rte_service.c
> +++ b/lib/librte_eal/common/rte_service.c
> @@ -65,6 +65,7 @@ struct core_state {
> /* map of services IDs are run on this core */
> uint64_t service_mask;
> uint8_t runstate; /* running or stopped */
> + uint8_t thread_active; /* indicates when thread is in service_run() */
Nit, if possible would be good to describe the difference between 'runstate' and 'thread_active'.
> uint8_t is_service_core; /* set if core is currently a service core */
> uint8_t service_active_on_lcore[RTE_SERVICE_NUM_MAX];
> uint64_t loops;
> @@ -457,6 +458,8 @@ service_runner_func(void *arg)
> const int lcore = rte_lcore_id();
> struct core_state *cs = &lcore_states[lcore];
>
> + __atomic_store_n(&cs->thread_active, 1, __ATOMIC_RELAXED);
Essentially, we have to ensure that all the memory operations are contained within both stores (this one and the one below) to 'thread_active'.
We should use __ATOMIC_SEQ_CST, which will avoid any memory operations from getting hoisted above this line.
Performance should not be an issue as it will get executed only when the service core is started.
It would be good to add comment reasoning the memory order used.
Also, what happens if the user calls 'rte_service_lcore_active' just before the above statement is executed? It might not be a valid use case, but it is good to document the race conditions and correct sequence of API calls.
> +
> /* runstate act as the guard variable. Use load-acquire
> * memory order here to synchronize with store-release
> * in runstate update functions.
> @@ -475,9 +478,20 @@ service_runner_func(void *arg)
> cs->loops++;
> }
>
> + __atomic_store_n(&cs->thread_active, 0, __ATOMIC_RELAXED);
__ATOMIC_RELEASE would be enough. But, __ATOMIC_SEQ_CST should not cause any performance issues.
> return 0;
> }
>
> +int32_t
> +rte_service_lcore_active(uint32_t lcore) {
> + if (lcore >= RTE_MAX_LCORE || !lcore_states[lcore].is_service_core)
> + return -EINVAL;
> +
> + return __atomic_load_n(&lcore_states[lcore].thread_active,
> + __ATOMIC_RELAXED);
> +}
> +
> int32_t
> rte_service_lcore_count(void)
> {
> diff --git a/lib/librte_eal/include/rte_service.h
> b/lib/librte_eal/include/rte_service.h
> index e2d0a6dd3..f7134b5c0 100644
> --- a/lib/librte_eal/include/rte_service.h
> +++ b/lib/librte_eal/include/rte_service.h
> @@ -261,6 +261,15 @@ int32_t rte_service_lcore_start(uint32_t lcore_id);
> */
> int32_t rte_service_lcore_stop(uint32_t lcore_id);
>
> +/**
> + * Reports if a service lcore is currently running.
> + * @retval 0 Service thread is not active, and has been returned to EAL.
> + * @retval 1 Service thread is in the service core polling loop.
> + * @retval -EINVAL Invalid *lcore_id* provided.
> + */
> +__rte_experimental
> +int32_t rte_service_lcore_active(uint32_t lcore_id);
Would 'rte_service_lcore_may_be_active' better? It would be inline with 'rte_service_may_be_active'?
I think we need additional documentation for 'rte_service_lcore_stop' to indicate that the caller should not assume that the service thread on the lcore has stopped running and should call the above API to check.
> +
> /**
> * Adds lcore to the list of service cores.
> *
> diff --git a/lib/librte_eal/rte_eal_version.map
> b/lib/librte_eal/rte_eal_version.map
> index bf0c17c23..d53d5d5b9 100644
> --- a/lib/librte_eal/rte_eal_version.map
> +++ b/lib/librte_eal/rte_eal_version.map
> @@ -401,6 +401,7 @@ EXPERIMENTAL {
> rte_lcore_dump;
> rte_lcore_iterate;
> rte_mp_disable;
> + rte_service_lcore_active;
> rte_thread_register;
> rte_thread_unregister;
> };
> --
> 2.17.1
next prev parent reply other threads:[~2020-07-21 19:43 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20200720120850eucas1p10debcafa273d244a7e63d225c50cc9df@eucas1p1.samsung.com>
2020-07-20 12:09 ` [dpdk-dev] [PATCH] service: fix stop API to wait for service thread Harry van Haaren
2020-07-20 12:51 ` Lukasz Wojciechowski
2020-07-20 14:20 ` Van Haaren, Harry
2020-07-20 14:38 ` [dpdk-dev] [PATCH v2 1/2] service: add API to retrieve service core active Harry van Haaren
2020-07-20 14:38 ` [dpdk-dev] [PATCH v2 2/2] test/service: fix race condition on stopping lcore Harry van Haaren
2020-07-20 17:45 ` Lukasz Wojciechowski
2020-07-21 8:38 ` Phil Yang
2020-07-22 10:26 ` Van Haaren, Harry
2020-07-20 17:45 ` [dpdk-dev] [PATCH v2 1/2] service: add API to retrieve service core active Lukasz Wojciechowski
2020-07-21 7:47 ` Phil Yang
2020-07-21 19:43 ` Honnappa Nagarahalli [this message]
2020-07-21 19:50 ` David Marchand
2020-07-21 20:23 ` Honnappa Nagarahalli
2020-07-22 10:14 ` Van Haaren, Harry
2020-07-22 18:50 ` Honnappa Nagarahalli
2020-07-23 16:59 ` Van Haaren, Harry
2020-07-22 10:37 ` [dpdk-dev] [PATCH v3 " Harry van Haaren
2020-07-22 10:37 ` [dpdk-dev] [PATCH v3 2/2] test/service: fix race condition on stopping lcore Harry van Haaren
2020-07-22 21:40 ` Honnappa Nagarahalli
2020-07-22 21:39 ` [dpdk-dev] [PATCH v3 1/2] service: add API to retrieve service core active Honnappa Nagarahalli
2020-07-24 12:45 ` [dpdk-dev] [PATCH v4 " Harry van Haaren
2020-07-24 12:45 ` [dpdk-dev] [PATCH v4 2/2] test/service: fix race condition on stopping lcore Harry van Haaren
2020-07-24 13:45 ` [dpdk-dev] [PATCH v5 1/2] service: add API to retrieve service core active Harry van Haaren
2020-07-24 13:45 ` [dpdk-dev] [PATCH v5 2/2] test/service: fix race condition on stopping lcore Harry van Haaren
2020-09-14 8:36 ` David Marchand
2020-09-14 14:33 ` Van Haaren, Harry
2020-09-14 14:31 ` [dpdk-dev] [PATCH v6 1/2] service: add API to retrieve service core active Harry van Haaren
2020-09-14 14:31 ` [dpdk-dev] [PATCH v6 2/2] test/service: fix race condition on stopping lcore Harry van Haaren
2020-09-21 14:51 ` David Marchand
2020-10-13 19:45 ` David Marchand
2020-10-15 8:11 ` Van Haaren, Harry
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=DB6PR0802MB221605308636E4B7039B491798780@DB6PR0802MB2216.eurprd08.prod.outlook.com \
--to=honnappa.nagarahalli@arm.com \
--cc=Phil.Yang@arm.com \
--cc=aconole@redhat.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=harry.van.haaren@intel.com \
--cc=igor.romanov@oktetlabs.ru \
--cc=l.wojciechow@partner.samsung.com \
--cc=nd@arm.com \
/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 NNTP newsgroup(s).