* [dpdk-dev] [PATCH v2] eal: rename state values in rte_lcore_state
2019-04-02 15:57 ` [dpdk-dev] [PATCH v2] " Stephen Hemminger
@ 2019-04-02 15:57 ` Stephen Hemminger
2019-04-02 20:15 ` David Marchand
2019-04-02 20:53 ` [dpdk-dev] [PATCH v3] " Stephen Hemminger
2 siblings, 0 replies; 20+ messages in thread
From: Stephen Hemminger @ 2019-04-02 15:57 UTC (permalink / raw)
To: dev; +Cc: Stephen Hemminger
C language does not really treat enum's as first class symbols.
The values in an enum live in a global namespace. That means if
DPDK defines "RUNNING" it can't be used by another enum in an
application using DPDK.
To solve this add a prefix "RTE_LCORE_" to the enum values, and
make them grammatically consistent.
Also, simplify the inline thread_is_running() which is copied
in softnic and ip_pipeline.
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
v2 - change prefix to RTE_LCORE_
drivers/event/octeontx/ssovf_evdev_selftest.c | 2 +-
drivers/event/sw/sw_evdev_selftest.c | 4 ++--
drivers/net/softnic/rte_eth_softnic_thread.c | 5 +----
examples/ip_pipeline/thread.c | 5 +----
examples/l2fwd-keepalive/main.c | 2 +-
lib/librte_eal/common/eal_common_launch.c | 12 ++++++------
lib/librte_eal/common/include/rte_launch.h | 6 +++---
lib/librte_eal/common/rte_service.c | 2 +-
lib/librte_eal/freebsd/eal/eal_thread.c | 6 +++---
lib/librte_eal/linux/eal/eal.c | 2 +-
lib/librte_eal/linux/eal/eal_thread.c | 8 ++++----
11 files changed, 24 insertions(+), 30 deletions(-)
diff --git a/drivers/event/octeontx/ssovf_evdev_selftest.c b/drivers/event/octeontx/ssovf_evdev_selftest.c
index 239362fcf549..06370840ba36 100644
--- a/drivers/event/octeontx/ssovf_evdev_selftest.c
+++ b/drivers/event/octeontx/ssovf_evdev_selftest.c
@@ -577,7 +577,7 @@ wait_workers_to_join(int lcore, const rte_atomic32_t *count)
RTE_SET_USED(count);
print_cycles = cycles = rte_get_timer_cycles();
- while (rte_eal_get_lcore_state(lcore) != FINISHED) {
+ while (rte_eal_get_lcore_state(lcore) != RTE_LCORE_FINISHED) {
uint64_t new_cycles = rte_get_timer_cycles();
if (new_cycles - print_cycles > rte_get_timer_hz()) {
diff --git a/drivers/event/sw/sw_evdev_selftest.c b/drivers/event/sw/sw_evdev_selftest.c
index d00d5de613d7..9e16f7d3520f 100644
--- a/drivers/event/sw/sw_evdev_selftest.c
+++ b/drivers/event/sw/sw_evdev_selftest.c
@@ -3116,8 +3116,8 @@ worker_loopback(struct test *t, uint8_t disable_implicit_release)
rte_eal_remote_launch(worker_loopback_worker_fn, t, w_lcore);
print_cycles = cycles = rte_get_timer_cycles();
- while (rte_eal_get_lcore_state(p_lcore) != FINISHED ||
- rte_eal_get_lcore_state(w_lcore) != FINISHED) {
+ while (rte_eal_get_lcore_state(p_lcore) != RTE_LCORE_FINISHED ||
+ rte_eal_get_lcore_state(w_lcore) != RTE_LCORE_FINISHED) {
rte_service_run_iter_on_app_lcore(t->service_id, 1);
diff --git a/drivers/net/softnic/rte_eth_softnic_thread.c b/drivers/net/softnic/rte_eth_softnic_thread.c
index 57989a5aac54..dee3923101bc 100644
--- a/drivers/net/softnic/rte_eth_softnic_thread.c
+++ b/drivers/net/softnic/rte_eth_softnic_thread.c
@@ -118,10 +118,7 @@ thread_is_valid(struct pmd_internals *softnic, uint32_t thread_id)
static inline int
thread_is_running(uint32_t thread_id)
{
- enum rte_lcore_state_t thread_state;
-
- thread_state = rte_eal_get_lcore_state(thread_id);
- return (thread_state == RUNNING)? 1 : 0;
+ return rte_eal_get_lcore_state(thread_id) == RTE_LCORE_RUNNING;
}
static int32_t
diff --git a/examples/ip_pipeline/thread.c b/examples/ip_pipeline/thread.c
index 272fbbeed1bc..3a62a0aee8bc 100644
--- a/examples/ip_pipeline/thread.c
+++ b/examples/ip_pipeline/thread.c
@@ -158,10 +158,7 @@ thread_init(void)
static inline int
thread_is_running(uint32_t thread_id)
{
- enum rte_lcore_state_t thread_state;
-
- thread_state = rte_eal_get_lcore_state(thread_id);
- return (thread_state == RUNNING) ? 1 : 0;
+ return rte_eal_get_lcore_state(thread_id) == RTE_LCORE_RUNNING;
}
/**
diff --git a/examples/l2fwd-keepalive/main.c b/examples/l2fwd-keepalive/main.c
index 0bf2b533648a..2e1edfa05937 100644
--- a/examples/l2fwd-keepalive/main.c
+++ b/examples/l2fwd-keepalive/main.c
@@ -502,7 +502,7 @@ dead_core(__rte_unused void *ptr_data, const int id_core)
if (terminate_signal_received)
return;
printf("Dead core %i - restarting..\n", id_core);
- if (rte_eal_get_lcore_state(id_core) == FINISHED) {
+ if (rte_eal_get_lcore_state(id_core) == RTE_LCORE_FINISHED) {
rte_eal_wait_lcore(id_core);
rte_eal_remote_launch(l2fwd_launch_one_lcore, NULL, id_core);
} else {
diff --git a/lib/librte_eal/common/eal_common_launch.c b/lib/librte_eal/common/eal_common_launch.c
index fe0ba3f0d617..74a3f1a28432 100644
--- a/lib/librte_eal/common/eal_common_launch.c
+++ b/lib/librte_eal/common/eal_common_launch.c
@@ -21,17 +21,17 @@
int
rte_eal_wait_lcore(unsigned slave_id)
{
- if (lcore_config[slave_id].state == WAIT)
+ if (lcore_config[slave_id].state == RTE_LCORE_WAITING)
return 0;
- while (lcore_config[slave_id].state != WAIT &&
- lcore_config[slave_id].state != FINISHED)
+ while (lcore_config[slave_id].state != RTE_LCORE_WAITING &&
+ lcore_config[slave_id].state != RTE_LCORE_FINISHED)
rte_pause();
rte_rmb();
/* we are in finished state, go to wait state */
- lcore_config[slave_id].state = WAIT;
+ lcore_config[slave_id].state = RTE_LCORE_WAITING;
return lcore_config[slave_id].ret;
}
@@ -49,7 +49,7 @@ rte_eal_mp_remote_launch(int (*f)(void *), void *arg,
/* check state of lcores */
RTE_LCORE_FOREACH_SLAVE(lcore_id) {
- if (lcore_config[lcore_id].state != WAIT)
+ if (lcore_config[lcore_id].state != RTE_LCORE_WAITING)
return -EBUSY;
}
@@ -60,7 +60,7 @@ rte_eal_mp_remote_launch(int (*f)(void *), void *arg,
if (call_master == CALL_MASTER) {
lcore_config[master].ret = f(arg);
- lcore_config[master].state = FINISHED;
+ lcore_config[master].state = RTE_LCORE_FINISHED;
}
return 0;
diff --git a/lib/librte_eal/common/include/rte_launch.h b/lib/librte_eal/common/include/rte_launch.h
index 06a671752ace..286ca31c98ab 100644
--- a/lib/librte_eal/common/include/rte_launch.h
+++ b/lib/librte_eal/common/include/rte_launch.h
@@ -19,9 +19,9 @@ extern "C" {
* State of an lcore.
*/
enum rte_lcore_state_t {
- WAIT, /**< waiting a new command */
- RUNNING, /**< executing command */
- FINISHED, /**< command executed */
+ RTE_LCORE_WAITING, /**< waiting a new command */
+ RTE_LCORE_RUNNING, /**< executing command */
+ RTE_LCORE_FINISHED, /**< command executed */
};
/**
diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index 5f75e5a53fbf..8dd7586027ff 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -450,7 +450,7 @@ rte_service_runner_func(void *arg)
rte_smp_rmb();
}
- lcore_config[lcore].state = WAIT;
+ lcore_config[lcore].state = RTE_LCORE_WAITING;
return 0;
}
diff --git a/lib/librte_eal/freebsd/eal/eal_thread.c b/lib/librte_eal/freebsd/eal/eal_thread.c
index 309b5872666b..f25e84fa312b 100644
--- a/lib/librte_eal/freebsd/eal/eal_thread.c
+++ b/lib/librte_eal/freebsd/eal/eal_thread.c
@@ -41,7 +41,7 @@ rte_eal_remote_launch(int (*f)(void *), void *arg, unsigned slave_id)
int m2s = lcore_config[slave_id].pipe_master2slave[1];
int s2m = lcore_config[slave_id].pipe_slave2master[0];
- if (lcore_config[slave_id].state != WAIT)
+ if (lcore_config[slave_id].state != RTE_LCORE_WAITING)
return -EBUSY;
lcore_config[slave_id].f = f;
@@ -136,7 +136,7 @@ eal_thread_loop(__attribute__((unused)) void *arg)
if (n <= 0)
rte_panic("cannot read on configuration pipe\n");
- lcore_config[lcore_id].state = RUNNING;
+ lcore_config[lcore_id].state = RTE_LCORE_RUNNING;
/* send ack */
n = 0;
@@ -153,7 +153,7 @@ eal_thread_loop(__attribute__((unused)) void *arg)
ret = lcore_config[lcore_id].f(fct_arg);
lcore_config[lcore_id].ret = ret;
rte_wmb();
- lcore_config[lcore_id].state = FINISHED;
+ lcore_config[lcore_id].state = RTE_LCORE_FINISHED;
}
/* never reached */
diff --git a/lib/librte_eal/linux/eal/eal.c b/lib/librte_eal/linux/eal/eal.c
index 75ed0cf1029f..3fe25a1feb2f 100644
--- a/lib/librte_eal/linux/eal/eal.c
+++ b/lib/librte_eal/linux/eal/eal.c
@@ -1158,7 +1158,7 @@ rte_eal_init(int argc, char **argv)
if (pipe(lcore_config[i].pipe_slave2master) < 0)
rte_panic("Cannot create pipe\n");
- lcore_config[i].state = WAIT;
+ lcore_config[i].state = RTE_LCORE_WAITING;
/* create a thread for each lcore */
ret = pthread_create(&lcore_config[i].thread_id, NULL,
diff --git a/lib/librte_eal/linux/eal/eal_thread.c b/lib/librte_eal/linux/eal/eal_thread.c
index 379773b683e8..058692a45412 100644
--- a/lib/librte_eal/linux/eal/eal_thread.c
+++ b/lib/librte_eal/linux/eal/eal_thread.c
@@ -41,7 +41,7 @@ rte_eal_remote_launch(int (*f)(void *), void *arg, unsigned slave_id)
int m2s = lcore_config[slave_id].pipe_master2slave[1];
int s2m = lcore_config[slave_id].pipe_slave2master[0];
- if (lcore_config[slave_id].state != WAIT)
+ if (lcore_config[slave_id].state != RTE_LCORE_WAITING)
return -EBUSY;
lcore_config[slave_id].f = f;
@@ -136,7 +136,7 @@ eal_thread_loop(__attribute__((unused)) void *arg)
if (n <= 0)
rte_panic("cannot read on configuration pipe\n");
- lcore_config[lcore_id].state = RUNNING;
+ lcore_config[lcore_id].state = RTE_LCORE_RUNNING;
/* send ack */
n = 0;
@@ -158,9 +158,9 @@ eal_thread_loop(__attribute__((unused)) void *arg)
* state, because the application will not lcore_wait() for it.
*/
if (lcore_config[lcore_id].core_role == ROLE_SERVICE)
- lcore_config[lcore_id].state = WAIT;
+ lcore_config[lcore_id].state = RTE_LCORE_WAITING;
else
- lcore_config[lcore_id].state = FINISHED;
+ lcore_config[lcore_id].state = RTE_LCORE_FINISHED;
}
/* never reached */
--
2.17.1
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [dpdk-dev] [PATCH v2] eal: rename state values in rte_lcore_state
2019-04-02 15:57 ` [dpdk-dev] [PATCH v2] " Stephen Hemminger
2019-04-02 15:57 ` Stephen Hemminger
@ 2019-04-02 20:15 ` David Marchand
2019-04-02 20:15 ` David Marchand
2019-04-02 20:48 ` Stephen Hemminger
2019-04-02 20:53 ` [dpdk-dev] [PATCH v3] " Stephen Hemminger
2 siblings, 2 replies; 20+ messages in thread
From: David Marchand @ 2019-04-02 20:15 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: dev
On Tue, Apr 2, 2019 at 5:57 PM Stephen Hemminger <stephen@networkplumber.org>
wrote:
> C language does not really treat enum's as first class symbols.
> The values in an enum live in a global namespace. That means if
> DPDK defines "RUNNING" it can't be used by another enum in an
> application using DPDK.
>
> To solve this add a prefix "RTE_LCORE_" to the enum values, and
> make them grammatically consistent.
>
Well, I understand this is not clean, but this patch breaks the API.
> Also, simplify the inline thread_is_running() which is copied
> in softnic and ip_pipeline.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> v2 - change prefix to RTE_LCORE_
>
> drivers/event/octeontx/ssovf_evdev_selftest.c | 2 +-
> drivers/event/sw/sw_evdev_selftest.c | 4 ++--
> drivers/net/softnic/rte_eth_softnic_thread.c | 5 +----
> examples/ip_pipeline/thread.c | 5 +----
> examples/l2fwd-keepalive/main.c | 2 +-
> lib/librte_eal/common/eal_common_launch.c | 12 ++++++------
> lib/librte_eal/common/include/rte_launch.h | 6 +++---
> lib/librte_eal/common/rte_service.c | 2 +-
> lib/librte_eal/freebsd/eal/eal_thread.c | 6 +++---
> lib/librte_eal/linux/eal/eal.c | 2 +-
> lib/librte_eal/linux/eal/eal_thread.c | 8 ++++----
> 11 files changed, 24 insertions(+), 30 deletions(-)
>
Missed a few places:
# should probably not access lcore_config directly...
examples/bond/main.c: if
(lcore_config[global_flag_stru_p->LcoreMainCore].state != WAIT) {
examples/bond/main.c: if (lcore_config[slave_core_id].state != WAIT)
# BSD must have trouble building
lib/librte_eal/freebsd/eal/eal.c: lcore_config[i].state =
WAIT;
# I suppose most comments are quite useless, but still unaligned with your
change
examples/ip_pipeline/thread.c: * (B) Its data plane thread is in RUNNING
state.
lib/librte_eal/common/eal_common_launch.c: * Check that every SLAVE lcores
are in WAIT state, then call
lib/librte_eal/common/include/rte_eal.h: * It puts the SLAVE lcores in the
WAIT state.
lib/librte_eal/common/include/rte_launch.h: * is in the WAIT state (this is
true after the first call to
lib/librte_eal/common/include/rte_launch.h: * the RUNNING state, then calls
the function f with argument arg. Once the
lib/librte_eal/common/include/rte_launch.h: * execution is done, the remote
lcore switches to a FINISHED state and
lib/librte_eal/common/include/rte_launch.h: * - (-EBUSY): The remote
lcore is not in a WAIT state.
lib/librte_eal/common/include/rte_launch.h: * Check that each SLAVE lcore
is in a WAIT state, then call
lib/librte_eal/common/include/rte_launch.h: * - (-EBUSY): At least one
remote lcore is not in a WAIT state. In this
lib/librte_eal/common/include/rte_launch.h: * If the slave lcore identified
by the slave_id is in a FINISHED state,
lib/librte_eal/common/include/rte_launch.h: * switch to the WAIT state. If
the lcore is in RUNNING state, wait until
lib/librte_eal/common/include/rte_launch.h: * the lcore finishes its job
and moves to the FINISHED state.
lib/librte_eal/common/include/rte_launch.h: * - 0: If the lcore
identified by the slave_id is in a WAIT state.
lib/librte_eal/common/include/rte_launch.h: * FINISHED or RUNNING
state. In this case, it changes the state
lib/librte_eal/common/include/rte_launch.h: * of the lcore to WAIT.
lib/librte_eal/common/include/rte_launch.h: * that all slave lcores are in
a WAIT state.
lib/librte_eal/freebsd/eal/eal_thread.c: * remote lcore switch in FINISHED
state.
lib/librte_eal/linux/eal/eal_thread.c: * remote lcore switch in FINISHED
state.
lib/librte_eal/linux/eal/eal_thread.c: /* when a service core
returns, it should go directly to WAIT
--
David Marchand
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [dpdk-dev] [PATCH v2] eal: rename state values in rte_lcore_state
2019-04-02 20:15 ` David Marchand
@ 2019-04-02 20:15 ` David Marchand
2019-04-02 20:48 ` Stephen Hemminger
1 sibling, 0 replies; 20+ messages in thread
From: David Marchand @ 2019-04-02 20:15 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: dev
On Tue, Apr 2, 2019 at 5:57 PM Stephen Hemminger <stephen@networkplumber.org>
wrote:
> C language does not really treat enum's as first class symbols.
> The values in an enum live in a global namespace. That means if
> DPDK defines "RUNNING" it can't be used by another enum in an
> application using DPDK.
>
> To solve this add a prefix "RTE_LCORE_" to the enum values, and
> make them grammatically consistent.
>
Well, I understand this is not clean, but this patch breaks the API.
> Also, simplify the inline thread_is_running() which is copied
> in softnic and ip_pipeline.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> v2 - change prefix to RTE_LCORE_
>
> drivers/event/octeontx/ssovf_evdev_selftest.c | 2 +-
> drivers/event/sw/sw_evdev_selftest.c | 4 ++--
> drivers/net/softnic/rte_eth_softnic_thread.c | 5 +----
> examples/ip_pipeline/thread.c | 5 +----
> examples/l2fwd-keepalive/main.c | 2 +-
> lib/librte_eal/common/eal_common_launch.c | 12 ++++++------
> lib/librte_eal/common/include/rte_launch.h | 6 +++---
> lib/librte_eal/common/rte_service.c | 2 +-
> lib/librte_eal/freebsd/eal/eal_thread.c | 6 +++---
> lib/librte_eal/linux/eal/eal.c | 2 +-
> lib/librte_eal/linux/eal/eal_thread.c | 8 ++++----
> 11 files changed, 24 insertions(+), 30 deletions(-)
>
Missed a few places:
# should probably not access lcore_config directly...
examples/bond/main.c: if
(lcore_config[global_flag_stru_p->LcoreMainCore].state != WAIT) {
examples/bond/main.c: if (lcore_config[slave_core_id].state != WAIT)
# BSD must have trouble building
lib/librte_eal/freebsd/eal/eal.c: lcore_config[i].state =
WAIT;
# I suppose most comments are quite useless, but still unaligned with your
change
examples/ip_pipeline/thread.c: * (B) Its data plane thread is in RUNNING
state.
lib/librte_eal/common/eal_common_launch.c: * Check that every SLAVE lcores
are in WAIT state, then call
lib/librte_eal/common/include/rte_eal.h: * It puts the SLAVE lcores in the
WAIT state.
lib/librte_eal/common/include/rte_launch.h: * is in the WAIT state (this is
true after the first call to
lib/librte_eal/common/include/rte_launch.h: * the RUNNING state, then calls
the function f with argument arg. Once the
lib/librte_eal/common/include/rte_launch.h: * execution is done, the remote
lcore switches to a FINISHED state and
lib/librte_eal/common/include/rte_launch.h: * - (-EBUSY): The remote
lcore is not in a WAIT state.
lib/librte_eal/common/include/rte_launch.h: * Check that each SLAVE lcore
is in a WAIT state, then call
lib/librte_eal/common/include/rte_launch.h: * - (-EBUSY): At least one
remote lcore is not in a WAIT state. In this
lib/librte_eal/common/include/rte_launch.h: * If the slave lcore identified
by the slave_id is in a FINISHED state,
lib/librte_eal/common/include/rte_launch.h: * switch to the WAIT state. If
the lcore is in RUNNING state, wait until
lib/librte_eal/common/include/rte_launch.h: * the lcore finishes its job
and moves to the FINISHED state.
lib/librte_eal/common/include/rte_launch.h: * - 0: If the lcore
identified by the slave_id is in a WAIT state.
lib/librte_eal/common/include/rte_launch.h: * FINISHED or RUNNING
state. In this case, it changes the state
lib/librte_eal/common/include/rte_launch.h: * of the lcore to WAIT.
lib/librte_eal/common/include/rte_launch.h: * that all slave lcores are in
a WAIT state.
lib/librte_eal/freebsd/eal/eal_thread.c: * remote lcore switch in FINISHED
state.
lib/librte_eal/linux/eal/eal_thread.c: * remote lcore switch in FINISHED
state.
lib/librte_eal/linux/eal/eal_thread.c: /* when a service core
returns, it should go directly to WAIT
--
David Marchand
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [dpdk-dev] [PATCH v2] eal: rename state values in rte_lcore_state
2019-04-02 20:15 ` David Marchand
2019-04-02 20:15 ` David Marchand
@ 2019-04-02 20:48 ` Stephen Hemminger
2019-04-02 20:48 ` Stephen Hemminger
2019-04-02 21:03 ` David Marchand
1 sibling, 2 replies; 20+ messages in thread
From: Stephen Hemminger @ 2019-04-02 20:48 UTC (permalink / raw)
To: David Marchand; +Cc: dev
On Tue, 2 Apr 2019 22:15:40 +0200
David Marchand <david.marchand@redhat.com> wrote:
> On Tue, Apr 2, 2019 at 5:57 PM Stephen Hemminger <stephen@networkplumber.org>
> wrote:
>
> > C language does not really treat enum's as first class symbols.
> > The values in an enum live in a global namespace. That means if
> > DPDK defines "RUNNING" it can't be used by another enum in an
> > application using DPDK.
> >
> > To solve this add a prefix "RTE_LCORE_" to the enum values, and
> > make them grammatically consistent.
> >
>
> Well, I understand this is not clean, but this patch breaks the API.
The lcore state was marked as internal in the header file, code that
ignores that is going to have problems. The values are the same.
We could defer this to 19.11 (next LTS) since it is cosmetic.
At that time, I am willing to do more work to make lcore_config hidden;
ie no inline's to access it.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [dpdk-dev] [PATCH v2] eal: rename state values in rte_lcore_state
2019-04-02 20:48 ` Stephen Hemminger
@ 2019-04-02 20:48 ` Stephen Hemminger
2019-04-02 21:03 ` David Marchand
1 sibling, 0 replies; 20+ messages in thread
From: Stephen Hemminger @ 2019-04-02 20:48 UTC (permalink / raw)
To: David Marchand; +Cc: dev
On Tue, 2 Apr 2019 22:15:40 +0200
David Marchand <david.marchand@redhat.com> wrote:
> On Tue, Apr 2, 2019 at 5:57 PM Stephen Hemminger <stephen@networkplumber.org>
> wrote:
>
> > C language does not really treat enum's as first class symbols.
> > The values in an enum live in a global namespace. That means if
> > DPDK defines "RUNNING" it can't be used by another enum in an
> > application using DPDK.
> >
> > To solve this add a prefix "RTE_LCORE_" to the enum values, and
> > make them grammatically consistent.
> >
>
> Well, I understand this is not clean, but this patch breaks the API.
The lcore state was marked as internal in the header file, code that
ignores that is going to have problems. The values are the same.
We could defer this to 19.11 (next LTS) since it is cosmetic.
At that time, I am willing to do more work to make lcore_config hidden;
ie no inline's to access it.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [dpdk-dev] [PATCH v2] eal: rename state values in rte_lcore_state
2019-04-02 20:48 ` Stephen Hemminger
2019-04-02 20:48 ` Stephen Hemminger
@ 2019-04-02 21:03 ` David Marchand
2019-04-02 21:03 ` David Marchand
2019-04-02 21:07 ` Stephen Hemminger
1 sibling, 2 replies; 20+ messages in thread
From: David Marchand @ 2019-04-02 21:03 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: dev
On Tue, Apr 2, 2019 at 10:48 PM Stephen Hemminger <
stephen@networkplumber.org> wrote:
> On Tue, 2 Apr 2019 22:15:40 +0200
> David Marchand <david.marchand@redhat.com> wrote:
>
> > On Tue, Apr 2, 2019 at 5:57 PM Stephen Hemminger <
> stephen@networkplumber.org>
> > wrote:
> >
> > > C language does not really treat enum's as first class symbols.
> > > The values in an enum live in a global namespace. That means if
> > > DPDK defines "RUNNING" it can't be used by another enum in an
> > > application using DPDK.
> > >
> > > To solve this add a prefix "RTE_LCORE_" to the enum values, and
> > > make them grammatically consistent.
> > >
> >
> > Well, I understand this is not clean, but this patch breaks the API.
>
> The lcore state was marked as internal in the header file, code that
> ignores that is going to have problems. The values are the same.
>
> We could defer this to 19.11 (next LTS) since it is cosmetic.
>
We must announce it before changing.
At that time, I am willing to do more work to make lcore_config hidden;
> ie no inline's to access it.
>
Ah I was looking at that, to see if we could at least shoot the direct
accesses by calling the existing API.
I can see we are missing one wrapper for the cpu id...
And there are quite some odds things in with cpu affinity in dpaa:
drivers/bus/dpaa/dpaa_bus.c: cpu = lcore_config[lcore].core_id;
drivers/bus/dpaa/dpaa_bus.c:
&lcore_config[lcore].cpuset);
drivers/bus/fslmc/portal/dpaa2_hw_dpio.c: if
(CPU_ISSET(i, &lcore_config[lcore_id].cpuset)) {
--
David Marchand
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [dpdk-dev] [PATCH v2] eal: rename state values in rte_lcore_state
2019-04-02 21:03 ` David Marchand
@ 2019-04-02 21:03 ` David Marchand
2019-04-02 21:07 ` Stephen Hemminger
1 sibling, 0 replies; 20+ messages in thread
From: David Marchand @ 2019-04-02 21:03 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: dev
On Tue, Apr 2, 2019 at 10:48 PM Stephen Hemminger <
stephen@networkplumber.org> wrote:
> On Tue, 2 Apr 2019 22:15:40 +0200
> David Marchand <david.marchand@redhat.com> wrote:
>
> > On Tue, Apr 2, 2019 at 5:57 PM Stephen Hemminger <
> stephen@networkplumber.org>
> > wrote:
> >
> > > C language does not really treat enum's as first class symbols.
> > > The values in an enum live in a global namespace. That means if
> > > DPDK defines "RUNNING" it can't be used by another enum in an
> > > application using DPDK.
> > >
> > > To solve this add a prefix "RTE_LCORE_" to the enum values, and
> > > make them grammatically consistent.
> > >
> >
> > Well, I understand this is not clean, but this patch breaks the API.
>
> The lcore state was marked as internal in the header file, code that
> ignores that is going to have problems. The values are the same.
>
> We could defer this to 19.11 (next LTS) since it is cosmetic.
>
We must announce it before changing.
At that time, I am willing to do more work to make lcore_config hidden;
> ie no inline's to access it.
>
Ah I was looking at that, to see if we could at least shoot the direct
accesses by calling the existing API.
I can see we are missing one wrapper for the cpu id...
And there are quite some odds things in with cpu affinity in dpaa:
drivers/bus/dpaa/dpaa_bus.c: cpu = lcore_config[lcore].core_id;
drivers/bus/dpaa/dpaa_bus.c:
&lcore_config[lcore].cpuset);
drivers/bus/fslmc/portal/dpaa2_hw_dpio.c: if
(CPU_ISSET(i, &lcore_config[lcore_id].cpuset)) {
--
David Marchand
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [dpdk-dev] [PATCH v2] eal: rename state values in rte_lcore_state
2019-04-02 21:03 ` David Marchand
2019-04-02 21:03 ` David Marchand
@ 2019-04-02 21:07 ` Stephen Hemminger
2019-04-02 21:07 ` Stephen Hemminger
2019-04-02 21:21 ` David Marchand
1 sibling, 2 replies; 20+ messages in thread
From: Stephen Hemminger @ 2019-04-02 21:07 UTC (permalink / raw)
To: David Marchand; +Cc: dev
On Tue, 2 Apr 2019 23:03:06 +0200
David Marchand <david.marchand@redhat.com> wrote:
> On Tue, Apr 2, 2019 at 10:48 PM Stephen Hemminger <
> stephen@networkplumber.org> wrote:
>
> > On Tue, 2 Apr 2019 22:15:40 +0200
> > David Marchand <david.marchand@redhat.com> wrote:
> >
> > > On Tue, Apr 2, 2019 at 5:57 PM Stephen Hemminger <
> > stephen@networkplumber.org>
> > > wrote:
> > >
> > > > C language does not really treat enum's as first class symbols.
> > > > The values in an enum live in a global namespace. That means if
> > > > DPDK defines "RUNNING" it can't be used by another enum in an
> > > > application using DPDK.
> > > >
> > > > To solve this add a prefix "RTE_LCORE_" to the enum values, and
> > > > make them grammatically consistent.
> > > >
> > >
> > > Well, I understand this is not clean, but this patch breaks the API.
> >
> > The lcore state was marked as internal in the header file, code that
> > ignores that is going to have problems. The values are the same.
> >
> > We could defer this to 19.11 (next LTS) since it is cosmetic.
> >
>
> We must announce it before changing.
I disagree, if an API is marked as internal it can be changed at any
time (as long as ABI is maintained).
>
>
> At that time, I am willing to do more work to make lcore_config hidden;
> > ie no inline's to access it.
> >
>
> Ah I was looking at that, to see if we could at least shoot the direct
> accesses by calling the existing API.
> I can see we are missing one wrapper for the cpu id...
>
> And there are quite some odds things in with cpu affinity in dpaa:
> drivers/bus/dpaa/dpaa_bus.c: cpu = lcore_config[lcore].core_id;
> drivers/bus/dpaa/dpaa_bus.c:
> &lcore_config[lcore].cpuset);
> drivers/bus/fslmc/portal/dpaa2_hw_dpio.c: if
> (CPU_ISSET(i, &lcore_config[lcore_id].cpuset)) {
>
>
Drivers have no API/ABI restriction, it is only user code.
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [dpdk-dev] [PATCH v2] eal: rename state values in rte_lcore_state
2019-04-02 21:07 ` Stephen Hemminger
@ 2019-04-02 21:07 ` Stephen Hemminger
2019-04-02 21:21 ` David Marchand
1 sibling, 0 replies; 20+ messages in thread
From: Stephen Hemminger @ 2019-04-02 21:07 UTC (permalink / raw)
To: David Marchand; +Cc: dev
On Tue, 2 Apr 2019 23:03:06 +0200
David Marchand <david.marchand@redhat.com> wrote:
> On Tue, Apr 2, 2019 at 10:48 PM Stephen Hemminger <
> stephen@networkplumber.org> wrote:
>
> > On Tue, 2 Apr 2019 22:15:40 +0200
> > David Marchand <david.marchand@redhat.com> wrote:
> >
> > > On Tue, Apr 2, 2019 at 5:57 PM Stephen Hemminger <
> > stephen@networkplumber.org>
> > > wrote:
> > >
> > > > C language does not really treat enum's as first class symbols.
> > > > The values in an enum live in a global namespace. That means if
> > > > DPDK defines "RUNNING" it can't be used by another enum in an
> > > > application using DPDK.
> > > >
> > > > To solve this add a prefix "RTE_LCORE_" to the enum values, and
> > > > make them grammatically consistent.
> > > >
> > >
> > > Well, I understand this is not clean, but this patch breaks the API.
> >
> > The lcore state was marked as internal in the header file, code that
> > ignores that is going to have problems. The values are the same.
> >
> > We could defer this to 19.11 (next LTS) since it is cosmetic.
> >
>
> We must announce it before changing.
I disagree, if an API is marked as internal it can be changed at any
time (as long as ABI is maintained).
>
>
> At that time, I am willing to do more work to make lcore_config hidden;
> > ie no inline's to access it.
> >
>
> Ah I was looking at that, to see if we could at least shoot the direct
> accesses by calling the existing API.
> I can see we are missing one wrapper for the cpu id...
>
> And there are quite some odds things in with cpu affinity in dpaa:
> drivers/bus/dpaa/dpaa_bus.c: cpu = lcore_config[lcore].core_id;
> drivers/bus/dpaa/dpaa_bus.c:
> &lcore_config[lcore].cpuset);
> drivers/bus/fslmc/portal/dpaa2_hw_dpio.c: if
> (CPU_ISSET(i, &lcore_config[lcore_id].cpuset)) {
>
>
Drivers have no API/ABI restriction, it is only user code.
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [dpdk-dev] [PATCH v2] eal: rename state values in rte_lcore_state
2019-04-02 21:07 ` Stephen Hemminger
2019-04-02 21:07 ` Stephen Hemminger
@ 2019-04-02 21:21 ` David Marchand
2019-04-02 21:21 ` David Marchand
1 sibling, 1 reply; 20+ messages in thread
From: David Marchand @ 2019-04-02 21:21 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: dev
On Tue, Apr 2, 2019 at 11:07 PM Stephen Hemminger <
stephen@networkplumber.org> wrote:
> On Tue, 2 Apr 2019 23:03:06 +0200
> David Marchand <david.marchand@redhat.com> wrote:
>
> > On Tue, Apr 2, 2019 at 10:48 PM Stephen Hemminger <
> > stephen@networkplumber.org> wrote:
> >
> > > On Tue, 2 Apr 2019 22:15:40 +0200
> > > David Marchand <david.marchand@redhat.com> wrote:
>
> > > > Well, I understand this is not clean, but this patch breaks the
> API.
> > >
> > > The lcore state was marked as internal in the header file, code that
> > > ignores that is going to have problems. The values are the same.
> > >
> > > We could defer this to 19.11 (next LTS) since it is cosmetic.
> > >
> >
> > We must announce it before changing.
>
> I disagree, if an API is marked as internal it can be changed at any
> time (as long as ABI is maintained).
>
rte_launch.h is exposed to applications.
And specifically, for rte_eal_get_lcore_state():
/**
* Get the state of the lcore identified by slave_id.
*
* To be executed on the MASTER lcore only.
*
* @param slave_id
* The identifier of the lcore.
* @return
* The state of the lcore.
*/
enum rte_lcore_state_t rte_eal_get_lcore_state(unsigned slave_id);
I don't see where this is marked as internal.
--
David Marchand
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [dpdk-dev] [PATCH v2] eal: rename state values in rte_lcore_state
2019-04-02 21:21 ` David Marchand
@ 2019-04-02 21:21 ` David Marchand
0 siblings, 0 replies; 20+ messages in thread
From: David Marchand @ 2019-04-02 21:21 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: dev
On Tue, Apr 2, 2019 at 11:07 PM Stephen Hemminger <
stephen@networkplumber.org> wrote:
> On Tue, 2 Apr 2019 23:03:06 +0200
> David Marchand <david.marchand@redhat.com> wrote:
>
> > On Tue, Apr 2, 2019 at 10:48 PM Stephen Hemminger <
> > stephen@networkplumber.org> wrote:
> >
> > > On Tue, 2 Apr 2019 22:15:40 +0200
> > > David Marchand <david.marchand@redhat.com> wrote:
>
> > > > Well, I understand this is not clean, but this patch breaks the
> API.
> > >
> > > The lcore state was marked as internal in the header file, code that
> > > ignores that is going to have problems. The values are the same.
> > >
> > > We could defer this to 19.11 (next LTS) since it is cosmetic.
> > >
> >
> > We must announce it before changing.
>
> I disagree, if an API is marked as internal it can be changed at any
> time (as long as ABI is maintained).
>
rte_launch.h is exposed to applications.
And specifically, for rte_eal_get_lcore_state():
/**
* Get the state of the lcore identified by slave_id.
*
* To be executed on the MASTER lcore only.
*
* @param slave_id
* The identifier of the lcore.
* @return
* The state of the lcore.
*/
enum rte_lcore_state_t rte_eal_get_lcore_state(unsigned slave_id);
I don't see where this is marked as internal.
--
David Marchand
^ permalink raw reply [flat|nested] 20+ messages in thread
* [dpdk-dev] [PATCH v3] eal: rename state values in rte_lcore_state
2019-04-02 15:57 ` [dpdk-dev] [PATCH v2] " Stephen Hemminger
2019-04-02 15:57 ` Stephen Hemminger
2019-04-02 20:15 ` David Marchand
@ 2019-04-02 20:53 ` Stephen Hemminger
2019-04-02 20:53 ` Stephen Hemminger
2019-04-03 11:59 ` David Marchand
2 siblings, 2 replies; 20+ messages in thread
From: Stephen Hemminger @ 2019-04-02 20:53 UTC (permalink / raw)
To: dev; +Cc: Stephen Hemminger
C language does not really treat enum's as first class symbols.
The values in an enum live in a global namespace. That means if
DPDK defines "RUNNING" it can't be used by another enum in an
application using DPDK.
To solve this add a prefix "RTE_LCORE_" to the enum values, and
make them grammatically consistent.
Also, simplify the inline thread_is_running() which is copied
in softnic and ip_pipeline.
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
v3 - fix BSD and bond driver usage of state
drivers/event/octeontx/ssovf_evdev_selftest.c | 2 +-
drivers/event/sw/sw_evdev_selftest.c | 4 ++--
drivers/net/softnic/rte_eth_softnic_thread.c | 5 +----
examples/bond/main.c | 4 ++--
examples/ip_pipeline/thread.c | 5 +----
examples/l2fwd-keepalive/main.c | 2 +-
lib/librte_eal/common/eal_common_launch.c | 12 ++++++------
lib/librte_eal/common/include/rte_launch.h | 6 +++---
lib/librte_eal/common/rte_service.c | 2 +-
lib/librte_eal/freebsd/eal/eal.c | 2 +-
lib/librte_eal/freebsd/eal/eal_thread.c | 6 +++---
lib/librte_eal/linux/eal/eal.c | 2 +-
lib/librte_eal/linux/eal/eal_thread.c | 8 ++++----
13 files changed, 27 insertions(+), 33 deletions(-)
diff --git a/drivers/event/octeontx/ssovf_evdev_selftest.c b/drivers/event/octeontx/ssovf_evdev_selftest.c
index 239362fcf549..06370840ba36 100644
--- a/drivers/event/octeontx/ssovf_evdev_selftest.c
+++ b/drivers/event/octeontx/ssovf_evdev_selftest.c
@@ -577,7 +577,7 @@ wait_workers_to_join(int lcore, const rte_atomic32_t *count)
RTE_SET_USED(count);
print_cycles = cycles = rte_get_timer_cycles();
- while (rte_eal_get_lcore_state(lcore) != FINISHED) {
+ while (rte_eal_get_lcore_state(lcore) != RTE_LCORE_FINISHED) {
uint64_t new_cycles = rte_get_timer_cycles();
if (new_cycles - print_cycles > rte_get_timer_hz()) {
diff --git a/drivers/event/sw/sw_evdev_selftest.c b/drivers/event/sw/sw_evdev_selftest.c
index d00d5de613d7..9e16f7d3520f 100644
--- a/drivers/event/sw/sw_evdev_selftest.c
+++ b/drivers/event/sw/sw_evdev_selftest.c
@@ -3116,8 +3116,8 @@ worker_loopback(struct test *t, uint8_t disable_implicit_release)
rte_eal_remote_launch(worker_loopback_worker_fn, t, w_lcore);
print_cycles = cycles = rte_get_timer_cycles();
- while (rte_eal_get_lcore_state(p_lcore) != FINISHED ||
- rte_eal_get_lcore_state(w_lcore) != FINISHED) {
+ while (rte_eal_get_lcore_state(p_lcore) != RTE_LCORE_FINISHED ||
+ rte_eal_get_lcore_state(w_lcore) != RTE_LCORE_FINISHED) {
rte_service_run_iter_on_app_lcore(t->service_id, 1);
diff --git a/drivers/net/softnic/rte_eth_softnic_thread.c b/drivers/net/softnic/rte_eth_softnic_thread.c
index 57989a5aac54..dee3923101bc 100644
--- a/drivers/net/softnic/rte_eth_softnic_thread.c
+++ b/drivers/net/softnic/rte_eth_softnic_thread.c
@@ -118,10 +118,7 @@ thread_is_valid(struct pmd_internals *softnic, uint32_t thread_id)
static inline int
thread_is_running(uint32_t thread_id)
{
- enum rte_lcore_state_t thread_state;
-
- thread_state = rte_eal_get_lcore_state(thread_id);
- return (thread_state == RUNNING)? 1 : 0;
+ return rte_eal_get_lcore_state(thread_id) == RTE_LCORE_RUNNING;
}
static int32_t
diff --git a/examples/bond/main.c b/examples/bond/main.c
index ef86194fff4a..12123a4b16d8 100644
--- a/examples/bond/main.c
+++ b/examples/bond/main.c
@@ -527,7 +527,7 @@ static void cmd_start_parsed(__attribute__((unused)) void *parsed_result,
rte_spinlock_trylock(&global_flag_stru_p->lock);
if (global_flag_stru_p->LcoreMainIsRunning == 0) {
- if (lcore_config[global_flag_stru_p->LcoreMainCore].state != WAIT) {
+ if (lcore_config[global_flag_stru_p->LcoreMainCore].state != RTE_LCORE_WAIT) {
rte_spinlock_unlock(&global_flag_stru_p->lock);
return;
}
@@ -796,7 +796,7 @@ main(int argc, char *argv[])
/* check state of lcores */
RTE_LCORE_FOREACH_SLAVE(slave_core_id) {
- if (lcore_config[slave_core_id].state != WAIT)
+ if (lcore_config[slave_core_id].state != RTE_LCORE_WAIT)
return -EBUSY;
}
/* start lcore main on core != master_core - ARP response thread */
diff --git a/examples/ip_pipeline/thread.c b/examples/ip_pipeline/thread.c
index 272fbbeed1bc..3a62a0aee8bc 100644
--- a/examples/ip_pipeline/thread.c
+++ b/examples/ip_pipeline/thread.c
@@ -158,10 +158,7 @@ thread_init(void)
static inline int
thread_is_running(uint32_t thread_id)
{
- enum rte_lcore_state_t thread_state;
-
- thread_state = rte_eal_get_lcore_state(thread_id);
- return (thread_state == RUNNING) ? 1 : 0;
+ return rte_eal_get_lcore_state(thread_id) == RTE_LCORE_RUNNING;
}
/**
diff --git a/examples/l2fwd-keepalive/main.c b/examples/l2fwd-keepalive/main.c
index 0bf2b533648a..2e1edfa05937 100644
--- a/examples/l2fwd-keepalive/main.c
+++ b/examples/l2fwd-keepalive/main.c
@@ -502,7 +502,7 @@ dead_core(__rte_unused void *ptr_data, const int id_core)
if (terminate_signal_received)
return;
printf("Dead core %i - restarting..\n", id_core);
- if (rte_eal_get_lcore_state(id_core) == FINISHED) {
+ if (rte_eal_get_lcore_state(id_core) == RTE_LCORE_FINISHED) {
rte_eal_wait_lcore(id_core);
rte_eal_remote_launch(l2fwd_launch_one_lcore, NULL, id_core);
} else {
diff --git a/lib/librte_eal/common/eal_common_launch.c b/lib/librte_eal/common/eal_common_launch.c
index fe0ba3f0d617..74a3f1a28432 100644
--- a/lib/librte_eal/common/eal_common_launch.c
+++ b/lib/librte_eal/common/eal_common_launch.c
@@ -21,17 +21,17 @@
int
rte_eal_wait_lcore(unsigned slave_id)
{
- if (lcore_config[slave_id].state == WAIT)
+ if (lcore_config[slave_id].state == RTE_LCORE_WAITING)
return 0;
- while (lcore_config[slave_id].state != WAIT &&
- lcore_config[slave_id].state != FINISHED)
+ while (lcore_config[slave_id].state != RTE_LCORE_WAITING &&
+ lcore_config[slave_id].state != RTE_LCORE_FINISHED)
rte_pause();
rte_rmb();
/* we are in finished state, go to wait state */
- lcore_config[slave_id].state = WAIT;
+ lcore_config[slave_id].state = RTE_LCORE_WAITING;
return lcore_config[slave_id].ret;
}
@@ -49,7 +49,7 @@ rte_eal_mp_remote_launch(int (*f)(void *), void *arg,
/* check state of lcores */
RTE_LCORE_FOREACH_SLAVE(lcore_id) {
- if (lcore_config[lcore_id].state != WAIT)
+ if (lcore_config[lcore_id].state != RTE_LCORE_WAITING)
return -EBUSY;
}
@@ -60,7 +60,7 @@ rte_eal_mp_remote_launch(int (*f)(void *), void *arg,
if (call_master == CALL_MASTER) {
lcore_config[master].ret = f(arg);
- lcore_config[master].state = FINISHED;
+ lcore_config[master].state = RTE_LCORE_FINISHED;
}
return 0;
diff --git a/lib/librte_eal/common/include/rte_launch.h b/lib/librte_eal/common/include/rte_launch.h
index 06a671752ace..286ca31c98ab 100644
--- a/lib/librte_eal/common/include/rte_launch.h
+++ b/lib/librte_eal/common/include/rte_launch.h
@@ -19,9 +19,9 @@ extern "C" {
* State of an lcore.
*/
enum rte_lcore_state_t {
- WAIT, /**< waiting a new command */
- RUNNING, /**< executing command */
- FINISHED, /**< command executed */
+ RTE_LCORE_WAITING, /**< waiting a new command */
+ RTE_LCORE_RUNNING, /**< executing command */
+ RTE_LCORE_FINISHED, /**< command executed */
};
/**
diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index 5f75e5a53fbf..8dd7586027ff 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -450,7 +450,7 @@ rte_service_runner_func(void *arg)
rte_smp_rmb();
}
- lcore_config[lcore].state = WAIT;
+ lcore_config[lcore].state = RTE_LCORE_WAITING;
return 0;
}
diff --git a/lib/librte_eal/freebsd/eal/eal.c b/lib/librte_eal/freebsd/eal/eal.c
index 790c6afa7050..aae4243486bd 100644
--- a/lib/librte_eal/freebsd/eal/eal.c
+++ b/lib/librte_eal/freebsd/eal/eal.c
@@ -784,7 +784,7 @@ rte_eal_init(int argc, char **argv)
if (pipe(lcore_config[i].pipe_slave2master) < 0)
rte_panic("Cannot create pipe\n");
- lcore_config[i].state = WAIT;
+ lcore_config[i].state = RTE_LCORE_WAIT;
/* create a thread for each lcore */
ret = pthread_create(&lcore_config[i].thread_id, NULL,
diff --git a/lib/librte_eal/freebsd/eal/eal_thread.c b/lib/librte_eal/freebsd/eal/eal_thread.c
index 309b5872666b..f25e84fa312b 100644
--- a/lib/librte_eal/freebsd/eal/eal_thread.c
+++ b/lib/librte_eal/freebsd/eal/eal_thread.c
@@ -41,7 +41,7 @@ rte_eal_remote_launch(int (*f)(void *), void *arg, unsigned slave_id)
int m2s = lcore_config[slave_id].pipe_master2slave[1];
int s2m = lcore_config[slave_id].pipe_slave2master[0];
- if (lcore_config[slave_id].state != WAIT)
+ if (lcore_config[slave_id].state != RTE_LCORE_WAITING)
return -EBUSY;
lcore_config[slave_id].f = f;
@@ -136,7 +136,7 @@ eal_thread_loop(__attribute__((unused)) void *arg)
if (n <= 0)
rte_panic("cannot read on configuration pipe\n");
- lcore_config[lcore_id].state = RUNNING;
+ lcore_config[lcore_id].state = RTE_LCORE_RUNNING;
/* send ack */
n = 0;
@@ -153,7 +153,7 @@ eal_thread_loop(__attribute__((unused)) void *arg)
ret = lcore_config[lcore_id].f(fct_arg);
lcore_config[lcore_id].ret = ret;
rte_wmb();
- lcore_config[lcore_id].state = FINISHED;
+ lcore_config[lcore_id].state = RTE_LCORE_FINISHED;
}
/* never reached */
diff --git a/lib/librte_eal/linux/eal/eal.c b/lib/librte_eal/linux/eal/eal.c
index 75ed0cf1029f..3fe25a1feb2f 100644
--- a/lib/librte_eal/linux/eal/eal.c
+++ b/lib/librte_eal/linux/eal/eal.c
@@ -1158,7 +1158,7 @@ rte_eal_init(int argc, char **argv)
if (pipe(lcore_config[i].pipe_slave2master) < 0)
rte_panic("Cannot create pipe\n");
- lcore_config[i].state = WAIT;
+ lcore_config[i].state = RTE_LCORE_WAITING;
/* create a thread for each lcore */
ret = pthread_create(&lcore_config[i].thread_id, NULL,
diff --git a/lib/librte_eal/linux/eal/eal_thread.c b/lib/librte_eal/linux/eal/eal_thread.c
index 379773b683e8..058692a45412 100644
--- a/lib/librte_eal/linux/eal/eal_thread.c
+++ b/lib/librte_eal/linux/eal/eal_thread.c
@@ -41,7 +41,7 @@ rte_eal_remote_launch(int (*f)(void *), void *arg, unsigned slave_id)
int m2s = lcore_config[slave_id].pipe_master2slave[1];
int s2m = lcore_config[slave_id].pipe_slave2master[0];
- if (lcore_config[slave_id].state != WAIT)
+ if (lcore_config[slave_id].state != RTE_LCORE_WAITING)
return -EBUSY;
lcore_config[slave_id].f = f;
@@ -136,7 +136,7 @@ eal_thread_loop(__attribute__((unused)) void *arg)
if (n <= 0)
rte_panic("cannot read on configuration pipe\n");
- lcore_config[lcore_id].state = RUNNING;
+ lcore_config[lcore_id].state = RTE_LCORE_RUNNING;
/* send ack */
n = 0;
@@ -158,9 +158,9 @@ eal_thread_loop(__attribute__((unused)) void *arg)
* state, because the application will not lcore_wait() for it.
*/
if (lcore_config[lcore_id].core_role == ROLE_SERVICE)
- lcore_config[lcore_id].state = WAIT;
+ lcore_config[lcore_id].state = RTE_LCORE_WAITING;
else
- lcore_config[lcore_id].state = FINISHED;
+ lcore_config[lcore_id].state = RTE_LCORE_FINISHED;
}
/* never reached */
--
2.17.1
^ permalink raw reply [flat|nested] 20+ messages in thread* [dpdk-dev] [PATCH v3] eal: rename state values in rte_lcore_state
2019-04-02 20:53 ` [dpdk-dev] [PATCH v3] " Stephen Hemminger
@ 2019-04-02 20:53 ` Stephen Hemminger
2019-04-03 11:59 ` David Marchand
1 sibling, 0 replies; 20+ messages in thread
From: Stephen Hemminger @ 2019-04-02 20:53 UTC (permalink / raw)
To: dev; +Cc: Stephen Hemminger
C language does not really treat enum's as first class symbols.
The values in an enum live in a global namespace. That means if
DPDK defines "RUNNING" it can't be used by another enum in an
application using DPDK.
To solve this add a prefix "RTE_LCORE_" to the enum values, and
make them grammatically consistent.
Also, simplify the inline thread_is_running() which is copied
in softnic and ip_pipeline.
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
v3 - fix BSD and bond driver usage of state
drivers/event/octeontx/ssovf_evdev_selftest.c | 2 +-
drivers/event/sw/sw_evdev_selftest.c | 4 ++--
drivers/net/softnic/rte_eth_softnic_thread.c | 5 +----
examples/bond/main.c | 4 ++--
examples/ip_pipeline/thread.c | 5 +----
examples/l2fwd-keepalive/main.c | 2 +-
lib/librte_eal/common/eal_common_launch.c | 12 ++++++------
lib/librte_eal/common/include/rte_launch.h | 6 +++---
lib/librte_eal/common/rte_service.c | 2 +-
lib/librte_eal/freebsd/eal/eal.c | 2 +-
lib/librte_eal/freebsd/eal/eal_thread.c | 6 +++---
lib/librte_eal/linux/eal/eal.c | 2 +-
lib/librte_eal/linux/eal/eal_thread.c | 8 ++++----
13 files changed, 27 insertions(+), 33 deletions(-)
diff --git a/drivers/event/octeontx/ssovf_evdev_selftest.c b/drivers/event/octeontx/ssovf_evdev_selftest.c
index 239362fcf549..06370840ba36 100644
--- a/drivers/event/octeontx/ssovf_evdev_selftest.c
+++ b/drivers/event/octeontx/ssovf_evdev_selftest.c
@@ -577,7 +577,7 @@ wait_workers_to_join(int lcore, const rte_atomic32_t *count)
RTE_SET_USED(count);
print_cycles = cycles = rte_get_timer_cycles();
- while (rte_eal_get_lcore_state(lcore) != FINISHED) {
+ while (rte_eal_get_lcore_state(lcore) != RTE_LCORE_FINISHED) {
uint64_t new_cycles = rte_get_timer_cycles();
if (new_cycles - print_cycles > rte_get_timer_hz()) {
diff --git a/drivers/event/sw/sw_evdev_selftest.c b/drivers/event/sw/sw_evdev_selftest.c
index d00d5de613d7..9e16f7d3520f 100644
--- a/drivers/event/sw/sw_evdev_selftest.c
+++ b/drivers/event/sw/sw_evdev_selftest.c
@@ -3116,8 +3116,8 @@ worker_loopback(struct test *t, uint8_t disable_implicit_release)
rte_eal_remote_launch(worker_loopback_worker_fn, t, w_lcore);
print_cycles = cycles = rte_get_timer_cycles();
- while (rte_eal_get_lcore_state(p_lcore) != FINISHED ||
- rte_eal_get_lcore_state(w_lcore) != FINISHED) {
+ while (rte_eal_get_lcore_state(p_lcore) != RTE_LCORE_FINISHED ||
+ rte_eal_get_lcore_state(w_lcore) != RTE_LCORE_FINISHED) {
rte_service_run_iter_on_app_lcore(t->service_id, 1);
diff --git a/drivers/net/softnic/rte_eth_softnic_thread.c b/drivers/net/softnic/rte_eth_softnic_thread.c
index 57989a5aac54..dee3923101bc 100644
--- a/drivers/net/softnic/rte_eth_softnic_thread.c
+++ b/drivers/net/softnic/rte_eth_softnic_thread.c
@@ -118,10 +118,7 @@ thread_is_valid(struct pmd_internals *softnic, uint32_t thread_id)
static inline int
thread_is_running(uint32_t thread_id)
{
- enum rte_lcore_state_t thread_state;
-
- thread_state = rte_eal_get_lcore_state(thread_id);
- return (thread_state == RUNNING)? 1 : 0;
+ return rte_eal_get_lcore_state(thread_id) == RTE_LCORE_RUNNING;
}
static int32_t
diff --git a/examples/bond/main.c b/examples/bond/main.c
index ef86194fff4a..12123a4b16d8 100644
--- a/examples/bond/main.c
+++ b/examples/bond/main.c
@@ -527,7 +527,7 @@ static void cmd_start_parsed(__attribute__((unused)) void *parsed_result,
rte_spinlock_trylock(&global_flag_stru_p->lock);
if (global_flag_stru_p->LcoreMainIsRunning == 0) {
- if (lcore_config[global_flag_stru_p->LcoreMainCore].state != WAIT) {
+ if (lcore_config[global_flag_stru_p->LcoreMainCore].state != RTE_LCORE_WAIT) {
rte_spinlock_unlock(&global_flag_stru_p->lock);
return;
}
@@ -796,7 +796,7 @@ main(int argc, char *argv[])
/* check state of lcores */
RTE_LCORE_FOREACH_SLAVE(slave_core_id) {
- if (lcore_config[slave_core_id].state != WAIT)
+ if (lcore_config[slave_core_id].state != RTE_LCORE_WAIT)
return -EBUSY;
}
/* start lcore main on core != master_core - ARP response thread */
diff --git a/examples/ip_pipeline/thread.c b/examples/ip_pipeline/thread.c
index 272fbbeed1bc..3a62a0aee8bc 100644
--- a/examples/ip_pipeline/thread.c
+++ b/examples/ip_pipeline/thread.c
@@ -158,10 +158,7 @@ thread_init(void)
static inline int
thread_is_running(uint32_t thread_id)
{
- enum rte_lcore_state_t thread_state;
-
- thread_state = rte_eal_get_lcore_state(thread_id);
- return (thread_state == RUNNING) ? 1 : 0;
+ return rte_eal_get_lcore_state(thread_id) == RTE_LCORE_RUNNING;
}
/**
diff --git a/examples/l2fwd-keepalive/main.c b/examples/l2fwd-keepalive/main.c
index 0bf2b533648a..2e1edfa05937 100644
--- a/examples/l2fwd-keepalive/main.c
+++ b/examples/l2fwd-keepalive/main.c
@@ -502,7 +502,7 @@ dead_core(__rte_unused void *ptr_data, const int id_core)
if (terminate_signal_received)
return;
printf("Dead core %i - restarting..\n", id_core);
- if (rte_eal_get_lcore_state(id_core) == FINISHED) {
+ if (rte_eal_get_lcore_state(id_core) == RTE_LCORE_FINISHED) {
rte_eal_wait_lcore(id_core);
rte_eal_remote_launch(l2fwd_launch_one_lcore, NULL, id_core);
} else {
diff --git a/lib/librte_eal/common/eal_common_launch.c b/lib/librte_eal/common/eal_common_launch.c
index fe0ba3f0d617..74a3f1a28432 100644
--- a/lib/librte_eal/common/eal_common_launch.c
+++ b/lib/librte_eal/common/eal_common_launch.c
@@ -21,17 +21,17 @@
int
rte_eal_wait_lcore(unsigned slave_id)
{
- if (lcore_config[slave_id].state == WAIT)
+ if (lcore_config[slave_id].state == RTE_LCORE_WAITING)
return 0;
- while (lcore_config[slave_id].state != WAIT &&
- lcore_config[slave_id].state != FINISHED)
+ while (lcore_config[slave_id].state != RTE_LCORE_WAITING &&
+ lcore_config[slave_id].state != RTE_LCORE_FINISHED)
rte_pause();
rte_rmb();
/* we are in finished state, go to wait state */
- lcore_config[slave_id].state = WAIT;
+ lcore_config[slave_id].state = RTE_LCORE_WAITING;
return lcore_config[slave_id].ret;
}
@@ -49,7 +49,7 @@ rte_eal_mp_remote_launch(int (*f)(void *), void *arg,
/* check state of lcores */
RTE_LCORE_FOREACH_SLAVE(lcore_id) {
- if (lcore_config[lcore_id].state != WAIT)
+ if (lcore_config[lcore_id].state != RTE_LCORE_WAITING)
return -EBUSY;
}
@@ -60,7 +60,7 @@ rte_eal_mp_remote_launch(int (*f)(void *), void *arg,
if (call_master == CALL_MASTER) {
lcore_config[master].ret = f(arg);
- lcore_config[master].state = FINISHED;
+ lcore_config[master].state = RTE_LCORE_FINISHED;
}
return 0;
diff --git a/lib/librte_eal/common/include/rte_launch.h b/lib/librte_eal/common/include/rte_launch.h
index 06a671752ace..286ca31c98ab 100644
--- a/lib/librte_eal/common/include/rte_launch.h
+++ b/lib/librte_eal/common/include/rte_launch.h
@@ -19,9 +19,9 @@ extern "C" {
* State of an lcore.
*/
enum rte_lcore_state_t {
- WAIT, /**< waiting a new command */
- RUNNING, /**< executing command */
- FINISHED, /**< command executed */
+ RTE_LCORE_WAITING, /**< waiting a new command */
+ RTE_LCORE_RUNNING, /**< executing command */
+ RTE_LCORE_FINISHED, /**< command executed */
};
/**
diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index 5f75e5a53fbf..8dd7586027ff 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -450,7 +450,7 @@ rte_service_runner_func(void *arg)
rte_smp_rmb();
}
- lcore_config[lcore].state = WAIT;
+ lcore_config[lcore].state = RTE_LCORE_WAITING;
return 0;
}
diff --git a/lib/librte_eal/freebsd/eal/eal.c b/lib/librte_eal/freebsd/eal/eal.c
index 790c6afa7050..aae4243486bd 100644
--- a/lib/librte_eal/freebsd/eal/eal.c
+++ b/lib/librte_eal/freebsd/eal/eal.c
@@ -784,7 +784,7 @@ rte_eal_init(int argc, char **argv)
if (pipe(lcore_config[i].pipe_slave2master) < 0)
rte_panic("Cannot create pipe\n");
- lcore_config[i].state = WAIT;
+ lcore_config[i].state = RTE_LCORE_WAIT;
/* create a thread for each lcore */
ret = pthread_create(&lcore_config[i].thread_id, NULL,
diff --git a/lib/librte_eal/freebsd/eal/eal_thread.c b/lib/librte_eal/freebsd/eal/eal_thread.c
index 309b5872666b..f25e84fa312b 100644
--- a/lib/librte_eal/freebsd/eal/eal_thread.c
+++ b/lib/librte_eal/freebsd/eal/eal_thread.c
@@ -41,7 +41,7 @@ rte_eal_remote_launch(int (*f)(void *), void *arg, unsigned slave_id)
int m2s = lcore_config[slave_id].pipe_master2slave[1];
int s2m = lcore_config[slave_id].pipe_slave2master[0];
- if (lcore_config[slave_id].state != WAIT)
+ if (lcore_config[slave_id].state != RTE_LCORE_WAITING)
return -EBUSY;
lcore_config[slave_id].f = f;
@@ -136,7 +136,7 @@ eal_thread_loop(__attribute__((unused)) void *arg)
if (n <= 0)
rte_panic("cannot read on configuration pipe\n");
- lcore_config[lcore_id].state = RUNNING;
+ lcore_config[lcore_id].state = RTE_LCORE_RUNNING;
/* send ack */
n = 0;
@@ -153,7 +153,7 @@ eal_thread_loop(__attribute__((unused)) void *arg)
ret = lcore_config[lcore_id].f(fct_arg);
lcore_config[lcore_id].ret = ret;
rte_wmb();
- lcore_config[lcore_id].state = FINISHED;
+ lcore_config[lcore_id].state = RTE_LCORE_FINISHED;
}
/* never reached */
diff --git a/lib/librte_eal/linux/eal/eal.c b/lib/librte_eal/linux/eal/eal.c
index 75ed0cf1029f..3fe25a1feb2f 100644
--- a/lib/librte_eal/linux/eal/eal.c
+++ b/lib/librte_eal/linux/eal/eal.c
@@ -1158,7 +1158,7 @@ rte_eal_init(int argc, char **argv)
if (pipe(lcore_config[i].pipe_slave2master) < 0)
rte_panic("Cannot create pipe\n");
- lcore_config[i].state = WAIT;
+ lcore_config[i].state = RTE_LCORE_WAITING;
/* create a thread for each lcore */
ret = pthread_create(&lcore_config[i].thread_id, NULL,
diff --git a/lib/librte_eal/linux/eal/eal_thread.c b/lib/librte_eal/linux/eal/eal_thread.c
index 379773b683e8..058692a45412 100644
--- a/lib/librte_eal/linux/eal/eal_thread.c
+++ b/lib/librte_eal/linux/eal/eal_thread.c
@@ -41,7 +41,7 @@ rte_eal_remote_launch(int (*f)(void *), void *arg, unsigned slave_id)
int m2s = lcore_config[slave_id].pipe_master2slave[1];
int s2m = lcore_config[slave_id].pipe_slave2master[0];
- if (lcore_config[slave_id].state != WAIT)
+ if (lcore_config[slave_id].state != RTE_LCORE_WAITING)
return -EBUSY;
lcore_config[slave_id].f = f;
@@ -136,7 +136,7 @@ eal_thread_loop(__attribute__((unused)) void *arg)
if (n <= 0)
rte_panic("cannot read on configuration pipe\n");
- lcore_config[lcore_id].state = RUNNING;
+ lcore_config[lcore_id].state = RTE_LCORE_RUNNING;
/* send ack */
n = 0;
@@ -158,9 +158,9 @@ eal_thread_loop(__attribute__((unused)) void *arg)
* state, because the application will not lcore_wait() for it.
*/
if (lcore_config[lcore_id].core_role == ROLE_SERVICE)
- lcore_config[lcore_id].state = WAIT;
+ lcore_config[lcore_id].state = RTE_LCORE_WAITING;
else
- lcore_config[lcore_id].state = FINISHED;
+ lcore_config[lcore_id].state = RTE_LCORE_FINISHED;
}
/* never reached */
--
2.17.1
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [dpdk-dev] [PATCH v3] eal: rename state values in rte_lcore_state
2019-04-02 20:53 ` [dpdk-dev] [PATCH v3] " Stephen Hemminger
2019-04-02 20:53 ` Stephen Hemminger
@ 2019-04-03 11:59 ` David Marchand
2019-04-03 11:59 ` David Marchand
1 sibling, 1 reply; 20+ messages in thread
From: David Marchand @ 2019-04-03 11:59 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: dev
On Tue, Apr 2, 2019 at 10:54 PM Stephen Hemminger <
stephen@networkplumber.org> wrote:
> C language does not really treat enum's as first class symbols.
> The values in an enum live in a global namespace. That means if
> DPDK defines "RUNNING" it can't be used by another enum in an
> application using DPDK.
>
> To solve this add a prefix "RTE_LCORE_" to the enum values, and
> make them grammatically consistent.
>
> Also, simplify the inline thread_is_running() which is copied
> in softnic and ip_pipeline.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> v3 - fix BSD and bond driver usage of state
>
>
>
> diff --git a/examples/bond/main.c b/examples/bond/main.c
> index ef86194fff4a..12123a4b16d8 100644
> --- a/examples/bond/main.c
> +++ b/examples/bond/main.c
> @@ -527,7 +527,7 @@ static void cmd_start_parsed(__attribute__((unused))
> void *parsed_result,
>
> rte_spinlock_trylock(&global_flag_stru_p->lock);
> if (global_flag_stru_p->LcoreMainIsRunning == 0) {
> - if (lcore_config[global_flag_stru_p->LcoreMainCore].state
> != WAIT) {
> + if (lcore_config[global_flag_stru_p->LcoreMainCore].state
> != RTE_LCORE_WAIT) {
> rte_spinlock_unlock(&global_flag_stru_p->lock);
> return;
> }
> @@ -796,7 +796,7 @@ main(int argc, char *argv[])
>
> /* check state of lcores */
> RTE_LCORE_FOREACH_SLAVE(slave_core_id) {
> - if (lcore_config[slave_core_id].state != WAIT)
> + if (lcore_config[slave_core_id].state != RTE_LCORE_WAIT)
> return -EBUSY;
> }
> /* start lcore main on core != master_core - ARP response thread */
>
diff --git a/lib/librte_eal/freebsd/eal/eal.c
> b/lib/librte_eal/freebsd/eal/eal.c
> index 790c6afa7050..aae4243486bd 100644
> --- a/lib/librte_eal/freebsd/eal/eal.c
> +++ b/lib/librte_eal/freebsd/eal/eal.c
> @@ -784,7 +784,7 @@ rte_eal_init(int argc, char **argv)
> if (pipe(lcore_config[i].pipe_slave2master) < 0)
> rte_panic("Cannot create pipe\n");
>
> - lcore_config[i].state = WAIT;
> + lcore_config[i].state = RTE_LCORE_WAIT;
>
> /* create a thread for each lcore */
> ret = pthread_create(&lcore_config[i].thread_id, NULL,
>
s/RTE_LCORE_WAIT/RTE_LCORE_WAITING/g
--
David Marchand
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [dpdk-dev] [PATCH v3] eal: rename state values in rte_lcore_state
2019-04-03 11:59 ` David Marchand
@ 2019-04-03 11:59 ` David Marchand
0 siblings, 0 replies; 20+ messages in thread
From: David Marchand @ 2019-04-03 11:59 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: dev
On Tue, Apr 2, 2019 at 10:54 PM Stephen Hemminger <
stephen@networkplumber.org> wrote:
> C language does not really treat enum's as first class symbols.
> The values in an enum live in a global namespace. That means if
> DPDK defines "RUNNING" it can't be used by another enum in an
> application using DPDK.
>
> To solve this add a prefix "RTE_LCORE_" to the enum values, and
> make them grammatically consistent.
>
> Also, simplify the inline thread_is_running() which is copied
> in softnic and ip_pipeline.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> v3 - fix BSD and bond driver usage of state
>
>
>
> diff --git a/examples/bond/main.c b/examples/bond/main.c
> index ef86194fff4a..12123a4b16d8 100644
> --- a/examples/bond/main.c
> +++ b/examples/bond/main.c
> @@ -527,7 +527,7 @@ static void cmd_start_parsed(__attribute__((unused))
> void *parsed_result,
>
> rte_spinlock_trylock(&global_flag_stru_p->lock);
> if (global_flag_stru_p->LcoreMainIsRunning == 0) {
> - if (lcore_config[global_flag_stru_p->LcoreMainCore].state
> != WAIT) {
> + if (lcore_config[global_flag_stru_p->LcoreMainCore].state
> != RTE_LCORE_WAIT) {
> rte_spinlock_unlock(&global_flag_stru_p->lock);
> return;
> }
> @@ -796,7 +796,7 @@ main(int argc, char *argv[])
>
> /* check state of lcores */
> RTE_LCORE_FOREACH_SLAVE(slave_core_id) {
> - if (lcore_config[slave_core_id].state != WAIT)
> + if (lcore_config[slave_core_id].state != RTE_LCORE_WAIT)
> return -EBUSY;
> }
> /* start lcore main on core != master_core - ARP response thread */
>
diff --git a/lib/librte_eal/freebsd/eal/eal.c
> b/lib/librte_eal/freebsd/eal/eal.c
> index 790c6afa7050..aae4243486bd 100644
> --- a/lib/librte_eal/freebsd/eal/eal.c
> +++ b/lib/librte_eal/freebsd/eal/eal.c
> @@ -784,7 +784,7 @@ rte_eal_init(int argc, char **argv)
> if (pipe(lcore_config[i].pipe_slave2master) < 0)
> rte_panic("Cannot create pipe\n");
>
> - lcore_config[i].state = WAIT;
> + lcore_config[i].state = RTE_LCORE_WAIT;
>
> /* create a thread for each lcore */
> ret = pthread_create(&lcore_config[i].thread_id, NULL,
>
s/RTE_LCORE_WAIT/RTE_LCORE_WAITING/g
--
David Marchand
^ permalink raw reply [flat|nested] 20+ messages in thread