diff mbox series

[v2,1/6] eal: reset lcore function pointer and argument

Message ID 20210909231312.2572006-2-honnappa.nagarahalli@arm.com
State New
Headers show
Series Use correct memory ordering in eal functions | expand

Commit Message

Honnappa Nagarahalli Sept. 9, 2021, 11:13 p.m. UTC 
In the rte_eal_remote_launch function, the lcore function
pointer is checked for NULL. However, the pointer is never
reset to NULL. Reset the lcore function pointer and argument
after the worker has completed executing the lcore function.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>

Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>

Reviewed-by: Feifei Wang <feifei.wang@arm.com>

---
 lib/eal/freebsd/eal_thread.c | 2 ++
 lib/eal/linux/eal_thread.c   | 2 ++
 lib/eal/windows/eal_thread.c | 2 ++
 3 files changed, 6 insertions(+)

-- 
2.25.1

Comments

Bruce Richardson Sept. 10, 2021, 7:49 a.m. UTC | #1 
On Thu, Sep 09, 2021 at 06:13:07PM -0500, Honnappa Nagarahalli wrote:
> In the rte_eal_remote_launch function, the lcore function pointer is

> checked for NULL. However, the pointer is never reset to NULL. Reset the

> lcore function pointer and argument after the worker has completed

> executing the lcore function.

> 

No problems with this patch, but just in general observation.  It would be
good if we had test cases to cover these sorts of problems. If a test were
added to the autotests for this issue we could observe the issue reproduced
and easily verify it were fixed by this patch, giving us a high degree of
confidence in the fix.

Regards,
/Bruce
David Marchand Sept. 10, 2021, 8:12 a.m. UTC | #2 
On Fri, Sep 10, 2021 at 9:49 AM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>

> On Thu, Sep 09, 2021 at 06:13:07PM -0500, Honnappa Nagarahalli wrote:

> > In the rte_eal_remote_launch function, the lcore function pointer is

> > checked for NULL. However, the pointer is never reset to NULL. Reset the

> > lcore function pointer and argument after the worker has completed

> > executing the lcore function.

> >

> No problems with this patch, but just in general observation.  It would be

> good if we had test cases to cover these sorts of problems. If a test were

> added to the autotests for this issue we could observe the issue reproduced

> and easily verify it were fixed by this patch, giving us a high degree of

> confidence in the fix.


+1


-- 
David Marchand
Honnappa Nagarahalli Sept. 11, 2021, 10:19 p.m. UTC | #3 
<snip>

> 

> On Fri, Sep 10, 2021 at 9:49 AM Bruce Richardson

> <bruce.richardson@intel.com> wrote:

> >

> > On Thu, Sep 09, 2021 at 06:13:07PM -0500, Honnappa Nagarahalli wrote:

> > > In the rte_eal_remote_launch function, the lcore function pointer is

> > > checked for NULL. However, the pointer is never reset to NULL. Reset

> > > the lcore function pointer and argument after the worker has

> > > completed executing the lcore function.

> > >

> > No problems with this patch, but just in general observation.  It

> > would be good if we had test cases to cover these sorts of problems.

> > If a test were added to the autotests for this issue we could observe

> > the issue reproduced and easily verify it were fixed by this patch,

> > giving us a high degree of confidence in the fix.

> 

> +1

Thanks for the comments.
Currently, the eal_thread_loop function calls rte_panic if the function pointer is set to NULL. This needs to be changed to return an error code. However, we need to distinguish between the error codes from EAL and the error codes from the function the lcore would run. Currently, there is no mechanism to return the error codes from EAL.

We could add a new member to " struct lcore_config" to capture the error codes from EAL. We need to add a new API to return the EAL error code to the application.

With this we should be able to add a new test case.

> 

> 

> --

> David Marchand
diff mbox series

Patch

diff --git a/lib/eal/freebsd/eal_thread.c b/lib/eal/freebsd/eal_thread.c
index 1dce9b04f2..bbc3a8e985 100644
--- a/lib/eal/freebsd/eal_thread.c
+++ b/lib/eal/freebsd/eal_thread.c
@@ -126,6 +126,8 @@  eal_thread_loop(__rte_unused void *arg)
 		fct_arg = lcore_config[lcore_id].arg;
 		ret = lcore_config[lcore_id].f(fct_arg);
 		lcore_config[lcore_id].ret = ret;
+		lcore_config[lcore_id].f = NULL;
+		lcore_config[lcore_id].arg = NULL;
 		rte_wmb();
 		lcore_config[lcore_id].state = FINISHED;
 	}
diff --git a/lib/eal/linux/eal_thread.c b/lib/eal/linux/eal_thread.c
index 83c2034b93..8f3c0dafd6 100644
--- a/lib/eal/linux/eal_thread.c
+++ b/lib/eal/linux/eal_thread.c
@@ -126,6 +126,8 @@  eal_thread_loop(__rte_unused void *arg)
 		fct_arg = lcore_config[lcore_id].arg;
 		ret = lcore_config[lcore_id].f(fct_arg);
 		lcore_config[lcore_id].ret = ret;
+		lcore_config[lcore_id].f = NULL;
+		lcore_config[lcore_id].arg = NULL;
 		rte_wmb();
 
 		/* when a service core returns, it should go directly to WAIT
diff --git a/lib/eal/windows/eal_thread.c b/lib/eal/windows/eal_thread.c
index 9c3f6d69fd..df1df5d02c 100644
--- a/lib/eal/windows/eal_thread.c
+++ b/lib/eal/windows/eal_thread.c
@@ -110,6 +110,8 @@  eal_thread_loop(void *arg __rte_unused)
 		fct_arg = lcore_config[lcore_id].arg;
 		ret = lcore_config[lcore_id].f(fct_arg);
 		lcore_config[lcore_id].ret = ret;
+		lcore_config[lcore_id].f = NULL;
+		lcore_config[lcore_id].arg = NULL;
 		rte_wmb();
 
 		/* when a service core returns, it should go directly to WAIT