mbox series

[RFC,0/5] Use correct memory ordering in eal functions

Message ID 20210224212018.17576-1-honnappa.nagarahalli@arm.com
Headers show
Series Use correct memory ordering in eal functions | expand

Message

Honnappa Nagarahalli Feb. 24, 2021, 9:20 p.m. UTC
rte_eal_remote_launch and rte_eal_wait_lcore need to provide
correct memory ordering to address the data communication from
main core to worker core and vice versa.

There are 2 use cases:
1) All the store operations (meant for worker) by main core
should be visible to worker core before the worker core runs the
assigned function

2) All the store operations by the worker core should be visible
to the main core after rte_eal_wait_lcore returns.

For the data that needs to be communicated to the worker after
the rte_eal_remote_launch returns, load-acquire and store-release
semantics should be used (just like any other writer-reader use case).

For the main to worker communication, the pointer to function
to execute is used as the guard variable. Hence, resetting of
the function pointer is important.

For the worker to main communication, the existing code uses the
lcore state as the guard variable. However, it looks like
the FINISHED state is not really required. Hence the FINISHED state
is removed before using the state as the guard variable. I would like
some feedback on why the FINISHED state is required. I have not
paid attention to what it means for backward compatibility.
If it is decided to remove this state, documentation changes are
required.


Honnappa Nagarahalli (5):
  eal: reset lcore function pointer and argument
  eal: ensure memory operations are visible to worker
  eal: lcore state FINISHED is not required
  eal: ensure memory operations are visible to main
  test/ring: use relaxed barriers for ring stress test

 app/test/test_ring_stress_impl.h              | 18 ++++-----
 drivers/event/dpaa2/dpaa2_eventdev_selftest.c |  2 +-
 drivers/event/octeontx/ssovf_evdev_selftest.c |  2 +-
 drivers/event/sw/sw_evdev_selftest.c          |  4 +-
 examples/l2fwd-keepalive/main.c               |  2 +-
 lib/librte_eal/common/eal_common_launch.c     | 13 +++---
 lib/librte_eal/freebsd/eal_thread.c           | 31 +++++++++++---
 lib/librte_eal/linux/eal_thread.c             | 40 +++++++++++++------
 lib/librte_eal/windows/eal_thread.c           | 34 +++++++++++-----
 9 files changed, 94 insertions(+), 52 deletions(-)

-- 
2.17.1

Comments

Stephen Hemminger March 1, 2021, 4:41 p.m. UTC | #1
On Wed, 24 Feb 2021 15:20:13 -0600
Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> wrote:

> rte_eal_remote_launch and rte_eal_wait_lcore need to provide

> correct memory ordering to address the data communication from

> main core to worker core and vice versa.

> 

> There are 2 use cases:

> 1) All the store operations (meant for worker) by main core

> should be visible to worker core before the worker core runs the

> assigned function

> 

> 2) All the store operations by the worker core should be visible

> to the main core after rte_eal_wait_lcore returns.

> 

> For the data that needs to be communicated to the worker after

> the rte_eal_remote_launch returns, load-acquire and store-release

> semantics should be used (just like any other writer-reader use case).

> 

> For the main to worker communication, the pointer to function

> to execute is used as the guard variable. Hence, resetting of

> the function pointer is important.

> 

> For the worker to main communication, the existing code uses the

> lcore state as the guard variable. However, it looks like

> the FINISHED state is not really required. Hence the FINISHED state

> is removed before using the state as the guard variable. I would like

> some feedback on why the FINISHED state is required. I have not

> paid attention to what it means for backward compatibility.

> If it is decided to remove this state, documentation changes are

> required.

> 



Most likely all use of volatile in DPDK is suspect.
Perhaps we should re-enable the "volatile considered harmful" warning
in checkpatch
Honnappa Nagarahalli March 2, 2021, 4:06 p.m. UTC | #2
<snip>

> 

> > rte_eal_remote_launch and rte_eal_wait_lcore need to provide correct

> > memory ordering to address the data communication from main core to

> > worker core and vice versa.

> >

> > There are 2 use cases:

> > 1) All the store operations (meant for worker) by main core should be

> > visible to worker core before the worker core runs the assigned

> > function

> >

> > 2) All the store operations by the worker core should be visible to

> > the main core after rte_eal_wait_lcore returns.

> >

> > For the data that needs to be communicated to the worker after the

> > rte_eal_remote_launch returns, load-acquire and store-release

> > semantics should be used (just like any other writer-reader use case).

> >

> > For the main to worker communication, the pointer to function to

> > execute is used as the guard variable. Hence, resetting of the

> > function pointer is important.

> >

> > For the worker to main communication, the existing code uses the lcore

> > state as the guard variable. However, it looks like the FINISHED state

> > is not really required. Hence the FINISHED state is removed before

> > using the state as the guard variable. I would like some feedback on

> > why the FINISHED state is required. I have not paid attention to what

> > it means for backward compatibility.

> > If it is decided to remove this state, documentation changes are

> > required.

> >

> 

> 

> Most likely all use of volatile in DPDK is suspect.

> Perhaps we should re-enable the "volatile considered harmful" warning in

> checkpatch

Agree.
David Marchand Oct. 25, 2021, 4:23 p.m. UTC | #3
On Mon, Oct 25, 2021 at 6:53 AM Honnappa Nagarahalli
<honnappa.nagarahalli@arm.com> wrote:
>

> v3:

> a) Added Fixes, Cc:stable#dpdk.org in 1/6

> b) Merged 3/6 & 4/6 and moved after the first commit in the series

> c) Merged 2/6 & 5/6 as they need to be in a single commit

> d) Removed use of volatile in 6/6 (Konstantin)

>

> rte_eal_remote_launch and rte_eal_wait_lcore need to provide

> correct memory ordering to address the data communication from

> main core to worker core.

>

> There are 2 use cases:

> 1) All the store operations (meant for worker) by main core

> should be visible to worker core before the worker core runs the

> assigned function

>

> 2) All the store operations by the worker core should be visible

> to the main core after rte_eal_wait_lcore returns.

>

> For the data that needs to be communicated to the worker after

> the rte_eal_remote_launch returns, load-acquire and store-release

> semantics should be used.

>

> For the main to worker communication, the pointer to function

> to execute is used as the guard variable. Hence, resetting of

> the function pointer is important.

>

> For the worker to main communication, the existing code uses the

> lcore state as the guard variable. However, it looks like

> the FINISHED state is not really required. Hence the FINISHED state

> is removed before using the state as the guard variable.

>

> Honnappa Nagarahalli (4):

>   eal: reset lcore function pointer and argument

>   eal: lcore state FINISHED is not required

>   eal: ensure correct memory ordering

>   test/ring: use relaxed barriers for ring stress test

>

>  app/test/test_ring_stress_impl.h              | 18 +++----

>  drivers/event/dpaa2/dpaa2_eventdev_selftest.c |  2 +-

>  drivers/event/octeontx/ssovf_evdev_selftest.c |  2 +-

>  drivers/event/sw/sw_evdev_selftest.c          |  4 +-

>  examples/l2fwd-keepalive/main.c               |  3 +-

>  lib/eal/common/eal_common_launch.c            | 13 ++---

>  lib/eal/freebsd/eal_thread.c                  | 45 +++++++++++++----

>  lib/eal/include/rte_launch.h                  | 21 ++++----

>  lib/eal/include/rte_service.h                 |  4 +-

>  lib/eal/linux/eal_thread.c                    | 48 +++++++++++++------

>  lib/eal/windows/eal_thread.c                  | 48 +++++++++++++------

>  11 files changed, 132 insertions(+), 76 deletions(-)


Tweaked commit titles, removed deprecation notice and updated release
notes in patch 2.
Series applied, thanks.


-- 
David Marchand