From: Igor Romanov <igor.romanov@oktetlabs.ru> The service core list is populated, but not used. Incorrect lcore states are examined for a service. Use the populated list to iterate over service cores. Fixes: e30dd31847d2 ("service: add mechanism for quiescing") Cc: stable@dpdk.org Signed-off-by: Igor Romanov <igor.romanov@oktetlabs.ru> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com> --- lib/librte_eal/common/rte_service.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c index 6123a2124d..e2795f857e 100644 --- a/lib/librte_eal/common/rte_service.c +++ b/lib/librte_eal/common/rte_service.c @@ -422,7 +422,7 @@ rte_service_may_be_active(uint32_t id) return -EINVAL; for (i = 0; i < lcore_count; i++) { - if (lcore_states[i].service_active_on_lcore[id]) + if (lcore_states[ids[i]].service_active_on_lcore[id]) return 1; } -- 2.17.1
Hi Andrew/Igor, A effective test case is missing for this, can you please add a test case? Otherwise it looks good. > -----Original Message----- > From: dev <dev-bounces@dpdk.org> On Behalf Of Andrew Rybchenko > Sent: Saturday, July 4, 2020 9:36 AM > To: dev@dpdk.org > Cc: Igor Romanov <igor.romanov@oktetlabs.ru>; stable@dpdk.org; Harry van > Haaren <harry.van.haaren@intel.com> > Subject: [dpdk-dev] [PATCH] service: fix wrong lcore indexes Nit, 'indices' would be better? ^^^^^^^ > > From: Igor Romanov <igor.romanov@oktetlabs.ru> > > The service core list is populated, but not used. Incorrect lcore states are > examined for a service. > > Use the populated list to iterate over service cores. > > Fixes: e30dd31847d2 ("service: add mechanism for quiescing") > Cc: stable@dpdk.org > > Signed-off-by: Igor Romanov <igor.romanov@oktetlabs.ru> > Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com> > --- > lib/librte_eal/common/rte_service.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/librte_eal/common/rte_service.c > b/lib/librte_eal/common/rte_service.c > index 6123a2124d..e2795f857e 100644 > --- a/lib/librte_eal/common/rte_service.c > +++ b/lib/librte_eal/common/rte_service.c > @@ -422,7 +422,7 @@ rte_service_may_be_active(uint32_t id) > return -EINVAL; > > for (i = 0; i < lcore_count; i++) { > - if (lcore_states[i].service_active_on_lcore[id]) > + if (lcore_states[ids[i]].service_active_on_lcore[id]) > return 1; > } > > -- > 2.17.1
On Sat, Jul 4, 2020 at 5:07 PM Honnappa Nagarahalli
<Honnappa.Nagarahalli@arm.com> wrote:
>
> Hi Andrew/Igor,
> A effective test case is missing for this, can you please add a test case? Otherwise it looks good.
+1, I was about to reply this.
--
David Marchand
From: Igor Romanov <igor.romanov@oktetlabs.ru> The service core list is populated, but not used. Incorrect lcore states are examined for a service. Use the populated list to iterate over service cores. Fixes: e30dd31847d2 ("service: add mechanism for quiescing") Cc: stable@dpdk.org Signed-off-by: Igor Romanov <igor.romanov@oktetlabs.ru> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com> --- lib/librte_eal/common/rte_service.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c index 6123a2124d..e2795f857e 100644 --- a/lib/librte_eal/common/rte_service.c +++ b/lib/librte_eal/common/rte_service.c @@ -422,7 +422,7 @@ rte_service_may_be_active(uint32_t id) return -EINVAL; for (i = 0; i < lcore_count; i++) { - if (lcore_states[i].service_active_on_lcore[id]) + if (lcore_states[ids[i]].service_active_on_lcore[id]) return 1; } -- 2.17.1
From: Igor Romanov <igor.romanov@oktetlabs.ru> The test checks that the service may be active API works when there are two cores: a non-service lcore and a service one. The API notes to take care when checking the status of a running service, but the test setup allows for a safe usage in that case. Signed-off-by: Igor Romanov <igor.romanov@oktetlabs.ru> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com> --- app/test/test_service_cores.c | 50 +++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/app/test/test_service_cores.c b/app/test/test_service_cores.c index 981e212130..80dc80ff9c 100644 --- a/app/test/test_service_cores.c +++ b/app/test/test_service_cores.c @@ -910,6 +910,55 @@ service_may_be_active(void) return unregister_all(); } +/* check service may be active when service is running on a second lcore */ +static int +service_active_two_cores(void) +{ + const uint32_t sid = 0; + int i; + + uint32_t lcore = rte_get_next_lcore(/* start core */ -1, + /* skip master */ 1, + /* wrap */ 0); + uint32_t slcore = rte_get_next_lcore(/* start core */ lcore, + /* skip master */ 1, + /* wrap */ 0); + + /* start the service on the second available lcore */ + TEST_ASSERT_EQUAL(0, rte_service_runstate_set(sid, 1), + "Starting valid service failed"); + TEST_ASSERT_EQUAL(0, rte_service_lcore_add(slcore), + "Add service core failed when not in use before"); + TEST_ASSERT_EQUAL(0, rte_service_map_lcore_set(sid, slcore, 1), + "Enabling valid service on valid core failed"); + TEST_ASSERT_EQUAL(0, rte_service_lcore_start(slcore), + "Service core start after add failed"); + + /* ensures core really is running the service function */ + TEST_ASSERT_EQUAL(1, service_lcore_running_check(), + "Service core expected to poll service but it didn't"); + + /* ensures that service may be active reports running state */ + TEST_ASSERT_EQUAL(1, rte_service_may_be_active(sid), + "Service may be active did not report running state"); + + /* stop the service */ + TEST_ASSERT_EQUAL(0, rte_service_runstate_set(sid, 0), + "Error: Service stop returned non-zero"); + + /* give the service 100ms to stop running */ + for (i = 0; i < 100; i++) { + if (!rte_service_may_be_active(sid)) + break; + rte_delay_ms(SERVICE_DELAY); + } + + TEST_ASSERT_EQUAL(0, rte_service_may_be_active(sid), + "Error: Service not stopped after 100ms"); + + return unregister_all(); +} + static struct unit_test_suite service_tests = { .suite_name = "service core test suite", .setup = testsuite_setup, @@ -931,6 +980,7 @@ static struct unit_test_suite service_tests = { TEST_CASE_ST(dummy_register, NULL, service_app_lcore_mt_safe), TEST_CASE_ST(dummy_register, NULL, service_app_lcore_mt_unsafe), TEST_CASE_ST(dummy_register, NULL, service_may_be_active), + TEST_CASE_ST(dummy_register, NULL, service_active_two_cores), TEST_CASES_END() /**< NULL terminate unit test array */ } }; -- 2.17.1
On 7/4/20 6:10 PM, David Marchand wrote:
> On Sat, Jul 4, 2020 at 5:07 PM Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com> wrote:
>>
>> Hi Andrew/Igor,
>> A effective test case is missing for this, can you please add a test case? Otherwise it looks good.
>
> +1, I was about to reply this.
Done in v2
From: Igor Romanov <igor.romanov@oktetlabs.ru> The service core list is populated, but not used. Incorrect lcore states are examined for a service. Use the populated list to iterate over service cores. Fixes: e30dd31847d2 ("service: add mechanism for quiescing") Cc: stable@dpdk.org Signed-off-by: Igor Romanov <igor.romanov@oktetlabs.ru> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com> --- lib/librte_eal/common/rte_service.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c index 6123a2124d..e2795f857e 100644 --- a/lib/librte_eal/common/rte_service.c +++ b/lib/librte_eal/common/rte_service.c @@ -422,7 +422,7 @@ rte_service_may_be_active(uint32_t id) return -EINVAL; for (i = 0; i < lcore_count; i++) { - if (lcore_states[i].service_active_on_lcore[id]) + if (lcore_states[ids[i]].service_active_on_lcore[id]) return 1; } -- 2.17.1
From: Igor Romanov <igor.romanov@oktetlabs.ru> The test checks that the service may be active API works when there are two cores: a non-service lcore and a service one. The API notes to take care when checking the status of a running service, but the test setup allows for a safe usage in that case. Signed-off-by: Igor Romanov <igor.romanov@oktetlabs.ru> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com> --- v3: - skip the test if number of cores is insufficient app/test/test_service_cores.c | 54 +++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/app/test/test_service_cores.c b/app/test/test_service_cores.c index 981e212130..4f003ed1af 100644 --- a/app/test/test_service_cores.c +++ b/app/test/test_service_cores.c @@ -910,6 +910,59 @@ service_may_be_active(void) return unregister_all(); } +/* check service may be active when service is running on a second lcore */ +static int +service_active_two_cores(void) +{ + if (!rte_lcore_is_enabled(0) || !rte_lcore_is_enabled(1) || + !rte_lcore_is_enabled(2)) + return TEST_SKIPPED; + + const uint32_t sid = 0; + int i; + + uint32_t lcore = rte_get_next_lcore(/* start core */ -1, + /* skip master */ 1, + /* wrap */ 0); + uint32_t slcore = rte_get_next_lcore(/* start core */ lcore, + /* skip master */ 1, + /* wrap */ 0); + + /* start the service on the second available lcore */ + TEST_ASSERT_EQUAL(0, rte_service_runstate_set(sid, 1), + "Starting valid service failed"); + TEST_ASSERT_EQUAL(0, rte_service_lcore_add(slcore), + "Add service core failed when not in use before"); + TEST_ASSERT_EQUAL(0, rte_service_map_lcore_set(sid, slcore, 1), + "Enabling valid service on valid core failed"); + TEST_ASSERT_EQUAL(0, rte_service_lcore_start(slcore), + "Service core start after add failed"); + + /* ensures core really is running the service function */ + TEST_ASSERT_EQUAL(1, service_lcore_running_check(), + "Service core expected to poll service but it didn't"); + + /* ensures that service may be active reports running state */ + TEST_ASSERT_EQUAL(1, rte_service_may_be_active(sid), + "Service may be active did not report running state"); + + /* stop the service */ + TEST_ASSERT_EQUAL(0, rte_service_runstate_set(sid, 0), + "Error: Service stop returned non-zero"); + + /* give the service 100ms to stop running */ + for (i = 0; i < 100; i++) { + if (!rte_service_may_be_active(sid)) + break; + rte_delay_ms(SERVICE_DELAY); + } + + TEST_ASSERT_EQUAL(0, rte_service_may_be_active(sid), + "Error: Service not stopped after 100ms"); + + return unregister_all(); +} + static struct unit_test_suite service_tests = { .suite_name = "service core test suite", .setup = testsuite_setup, @@ -931,6 +984,7 @@ static struct unit_test_suite service_tests = { TEST_CASE_ST(dummy_register, NULL, service_app_lcore_mt_safe), TEST_CASE_ST(dummy_register, NULL, service_app_lcore_mt_unsafe), TEST_CASE_ST(dummy_register, NULL, service_may_be_active), + TEST_CASE_ST(dummy_register, NULL, service_active_two_cores), TEST_CASES_END() /**< NULL terminate unit test array */ } }; -- 2.17.1
> -----Original Message----- > From: Andrew Rybchenko <arybchenko@solarflare.com> > Sent: Tuesday, July 7, 2020 11:45 AM > To: dev@dpdk.org > Cc: Igor Romanov <igor.romanov@oktetlabs.ru>; stable@dpdk.org; Van Haaren, > Harry <harry.van.haaren@intel.com> > Subject: [PATCH v3 1/2] service: fix wrong lcore indices > > From: Igor Romanov <igor.romanov@oktetlabs.ru> > > The service core list is populated, but not used. Incorrect > lcore states are examined for a service. > > Use the populated list to iterate over service cores. > > Fixes: e30d d318 47d2 ("service: add mechanism for quiescing") > Cc: stable@dpdk.org I believe the original adding of quiescing did not have this exact bug (ids[] was used)? It seems to have been introduced when reworking to avoid false sharing, so fixes is: Fixes: e484ccddbe1b ("services: avoid false sharing on core state") > Signed-off-by: Igor Romanov <igor.romanov@oktetlabs.ru> > Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com> Code change itself: Acked-by: Harry van Haaren <harry.van.haaren@intel.com> <snip patch>
> -----Original Message-----
> From: Andrew Rybchenko <arybchenko@solarflare.com>
> Sent: Tuesday, July 7, 2020 11:45 AM
> To: dev@dpdk.org
> Cc: Igor Romanov <igor.romanov@oktetlabs.ru>; Van Haaren, Harry
> <harry.van.haaren@intel.com>
> Subject: [PATCH v3 2/2] test/service: add test for service active on two lcores
>
> From: Igor Romanov <igor.romanov@oktetlabs.ru>
>
> The test checks that the service may be active API works
> when there are two cores: a non-service lcore and a service one.
>
> The API notes to take care when checking the status of a running
> service, but the test setup allows for a safe usage in that case.
>
> Signed-off-by: Igor Romanov <igor.romanov@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
Confirmed test case fails without 1/2 fix, and passes with fix.
Acked-by: Harry van Haaren <harry.van.haaren@intel.com>
<snip> > Subject: [dpdk-dev] [PATCH v3 1/2] service: fix wrong lcore indices > > From: Igor Romanov <igor.romanov@oktetlabs.ru> > > The service core list is populated, but not used. Incorrect lcore states are > examined for a service. > > Use the populated list to iterate over service cores. > > Fixes: e30dd31847d2 ("service: add mechanism for quiescing") > Cc: stable@dpdk.org > > Signed-off-by: Igor Romanov <igor.romanov@oktetlabs.ru> > Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com> LGTM Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> > --- > lib/librte_eal/common/rte_service.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/librte_eal/common/rte_service.c > b/lib/librte_eal/common/rte_service.c > index 6123a2124d..e2795f857e 100644 > --- a/lib/librte_eal/common/rte_service.c > +++ b/lib/librte_eal/common/rte_service.c > @@ -422,7 +422,7 @@ rte_service_may_be_active(uint32_t id) > return -EINVAL; > > for (i = 0; i < lcore_count; i++) { > - if (lcore_states[i].service_active_on_lcore[id]) > + if (lcore_states[ids[i]].service_active_on_lcore[id]) > return 1; > } > > -- > 2.17.1
> -----Original Message----- > From: dev <dev-bounces@dpdk.org> On Behalf Of Andrew Rybchenko > Sent: Tuesday, July 7, 2020 5:45 AM > To: dev@dpdk.org > Cc: Igor Romanov <igor.romanov@oktetlabs.ru>; Harry van Haaren > <harry.van.haaren@intel.com> > Subject: [dpdk-dev] [PATCH v3 2/2] test/service: add test for service active on > two lcores > > From: Igor Romanov <igor.romanov@oktetlabs.ru> > > The test checks that the service may be active API works when there are two > cores: a non-service lcore and a service one. > > The API notes to take care when checking the status of a running service, but > the test setup allows for a safe usage in that case. > > Signed-off-by: Igor Romanov <igor.romanov@oktetlabs.ru> > Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> > --- > v3: > - skip the test if number of cores is insufficient > > app/test/test_service_cores.c | 54 +++++++++++++++++++++++++++++++++++ > 1 file changed, 54 insertions(+) > > diff --git a/app/test/test_service_cores.c b/app/test/test_service_cores.c index > 981e212130..4f003ed1af 100644 > --- a/app/test/test_service_cores.c > +++ b/app/test/test_service_cores.c > @@ -910,6 +910,59 @@ service_may_be_active(void) > return unregister_all(); > } > > +/* check service may be active when service is running on a second > +lcore */ static int > +service_active_two_cores(void) > +{ > + if (!rte_lcore_is_enabled(0) || !rte_lcore_is_enabled(1) || > + !rte_lcore_is_enabled(2)) > + return TEST_SKIPPED; > + > + const uint32_t sid = 0; > + int i; > + > + uint32_t lcore = rte_get_next_lcore(/* start core */ -1, > + /* skip master */ 1, > + /* wrap */ 0); > + uint32_t slcore = rte_get_next_lcore(/* start core */ lcore, > + /* skip master */ 1, > + /* wrap */ 0); > + > + /* start the service on the second available lcore */ > + TEST_ASSERT_EQUAL(0, rte_service_runstate_set(sid, 1), > + "Starting valid service failed"); > + TEST_ASSERT_EQUAL(0, rte_service_lcore_add(slcore), > + "Add service core failed when not in use before"); > + TEST_ASSERT_EQUAL(0, rte_service_map_lcore_set(sid, slcore, 1), > + "Enabling valid service on valid core failed"); > + TEST_ASSERT_EQUAL(0, rte_service_lcore_start(slcore), > + "Service core start after add failed"); > + > + /* ensures core really is running the service function */ > + TEST_ASSERT_EQUAL(1, service_lcore_running_check(), > + "Service core expected to poll service but it didn't"); > + > + /* ensures that service may be active reports running state */ > + TEST_ASSERT_EQUAL(1, rte_service_may_be_active(sid), > + "Service may be active did not report running state"); > + > + /* stop the service */ > + TEST_ASSERT_EQUAL(0, rte_service_runstate_set(sid, 0), > + "Error: Service stop returned non-zero"); > + > + /* give the service 100ms to stop running */ > + for (i = 0; i < 100; i++) { > + if (!rte_service_may_be_active(sid)) > + break; > + rte_delay_ms(SERVICE_DELAY); > + } > + > + TEST_ASSERT_EQUAL(0, rte_service_may_be_active(sid), > + "Error: Service not stopped after 100ms"); > + > + return unregister_all(); > +} > + > static struct unit_test_suite service_tests = { > .suite_name = "service core test suite", > .setup = testsuite_setup, > @@ -931,6 +984,7 @@ static struct unit_test_suite service_tests = { > TEST_CASE_ST(dummy_register, NULL, > service_app_lcore_mt_safe), > TEST_CASE_ST(dummy_register, NULL, > service_app_lcore_mt_unsafe), > TEST_CASE_ST(dummy_register, NULL, service_may_be_active), > + TEST_CASE_ST(dummy_register, NULL, > service_active_two_cores), > TEST_CASES_END() /**< NULL terminate unit test array */ > } > }; > -- > 2.17.1
07/07/2020 15:14, Van Haaren, Harry:
> From: Andrew Rybchenko <arybchenko@solarflare.com>
> > From: Igor Romanov <igor.romanov@oktetlabs.ru>
> >
> > The service core list is populated, but not used. Incorrect
> > lcore states are examined for a service.
> >
> > Use the populated list to iterate over service cores.
> >
> > Fixes: e30d d318 47d2 ("service: add mechanism for quiescing")
> > Cc: stable@dpdk.org
>
> I believe the original adding of quiescing did not have this exact bug (ids[] was used)?
> It seems to have been introduced when reworking to avoid false sharing, so fixes is:
> Fixes: e484ccddbe1b ("services: avoid false sharing on core state")
>
>
> > Signed-off-by: Igor Romanov <igor.romanov@oktetlabs.ru>
> > Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>
> Code change itself:
> Acked-by: Harry van Haaren <harry.van.haaren@intel.com>
Series applied, thanks