[v2] eal: rename state values in rte_lcore_state
Checks
Commit Message
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(-)
Comments
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
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.
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)) {
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.
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.
@@ -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()) {
@@ -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);
@@ -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
@@ -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;
}
/**
@@ -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 {
@@ -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;
@@ -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 */
};
/**
@@ -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;
}
@@ -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 */
@@ -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,
@@ -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 */