[v3,07/12] service: remove rte prefix from static functions
diff mbox series

Message ID 1584407863-774-8-git-send-email-phil.yang@arm.com
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series
  • generic rte atomic APIs deprecate proposal
Related show

Checks

Context Check Description
ci/Intel-compilation fail Compilation issues
ci/checkpatch success coding style OK

Commit Message

Phil Yang March 17, 2020, 1:17 a.m. UTC
Fixes: 3cf5eb1546ed ("service: fix and refactor atomic service accesses")
Fixes: 21698354c832 ("service: introduce service cores concept")
Cc: stable@dpdk.org

Signed-off-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
---
 lib/librte_eal/common/rte_service.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

Comments

Van Haaren, Harry April 3, 2020, 11:57 a.m. UTC | #1
> From: Phil Yang <phil.yang@arm.com>
> Sent: Tuesday, March 17, 2020 1:18 AM
> To: thomas@monjalon.net; Van Haaren, Harry <harry.van.haaren@intel.com>;
> Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> stephen@networkplumber.org; maxime.coquelin@redhat.com; dev@dpdk.org
> Cc: david.marchand@redhat.com; jerinj@marvell.com; hemant.agrawal@nxp.com;
> Honnappa.Nagarahalli@arm.com; gavin.hu@arm.com; ruifeng.wang@arm.com;
> joyce.kong@arm.com; nd@arm.com; stable@dpdk.org
> Subject: [PATCH v3 07/12] service: remove rte prefix from static functions
> 
> Fixes: 3cf5eb1546ed ("service: fix and refactor atomic service accesses")
> Fixes: 21698354c832 ("service: introduce service cores concept")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>


This patchset needs a rebase since the EAL file movement got merged,
however I'll review here so we can include some Acks etc and make
progress.

Is this really a "Fix"? The internal function names were not exported
in the .map file, so are not part of public ABI. This is an internal
naming improvement (thanks for doing cleanup), but I don't think the
Fixes: tags make sense?

Also I'm not sure if we want to port this patch back to stable? Changing (internal) function names seems like unnecessary churn, and hence risk to a stable release, without any benefit?

---

<snip patch diff>
Honnappa Nagarahalli April 5, 2020, 9:35 p.m. UTC | #2
<snip>

> Subject: [PATCH v3 07/12] service: remove rte prefix from static functions
> 
> Fixes: 3cf5eb1546ed ("service: fix and refactor atomic service accesses")
> Fixes: 21698354c832 ("service: introduce service cores concept")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> ---
>  lib/librte_eal/common/rte_service.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/librte_eal/common/rte_service.c
> b/lib/librte_eal/common/rte_service.c
> index b0b78ba..2117726 100644
> --- a/lib/librte_eal/common/rte_service.c
> +++ b/lib/librte_eal/common/rte_service.c
> @@ -336,7 +336,7 @@ rte_service_runstate_get(uint32_t id)  }
> 
>  static inline void
> -rte_service_runner_do_callback(struct rte_service_spec_impl *s,
> +service_runner_do_callback(struct rte_service_spec_impl *s,
>  			       struct core_state *cs, uint32_t service_idx)  {
>  	void *userdata = s->spec.callback_userdata; @@ -379,10 +379,10
> @@ service_run(uint32_t i, struct core_state *cs, uint64_t service_mask,
>  		if (!rte_atomic32_cmpset((uint32_t *)&s->execute_lock, 0, 1))
>  			return -EBUSY;
> 
> -		rte_service_runner_do_callback(s, cs, i);
> +		service_runner_do_callback(s, cs, i);
>  		rte_atomic32_clear(&s->execute_lock);
>  	} else
> -		rte_service_runner_do_callback(s, cs, i);
> +		service_runner_do_callback(s, cs, i);
> 
>  	return 0;
>  }
> @@ -436,7 +436,7 @@ rte_service_run_iter_on_app_lcore(uint32_t id,
> uint32_t serialize_mt_unsafe)  }
> 
>  static int32_t
> -rte_service_runner_func(void *arg)
> +service_runner_func(void *arg)
>  {
>  	RTE_SET_USED(arg);
>  	uint32_t i;
This is a minor comment.
Since you are touching 'service_runner_func', please consider doing the below improvement:

struct core_state *cs = &lcore_states[lcore];
while (lcore_states[lcore].runstate == RUNSTATE_RUNNING) {   

The while above can be changed as follows to make it more readable

while (cs->runstate == RUNSTATE_RUNNING) {   

> @@ -706,7 +706,7 @@ rte_service_lcore_start(uint32_t lcore)
>  	 */
>  	lcore_states[lcore].runstate = RUNSTATE_RUNNING;
> 
> -	int ret = rte_eal_remote_launch(rte_service_runner_func, 0, lcore);
> +	int ret = rte_eal_remote_launch(service_runner_func, 0, lcore);
>  	/* returns -EBUSY if the core is already launched, 0 on success */
>  	return ret;
>  }
> @@ -785,7 +785,7 @@ rte_service_lcore_attr_get(uint32_t lcore, uint32_t
> attr_id,  }
> 
<snip>
Phil Yang April 8, 2020, 10:14 a.m. UTC | #3
> -----Original Message-----
> From: Van Haaren, Harry <harry.van.haaren@intel.com>
> Sent: Friday, April 3, 2020 7:58 PM
> To: Phil Yang <Phil.Yang@arm.com>; thomas@monjalon.net; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>;
> stephen@networkplumber.org; maxime.coquelin@redhat.com;
> dev@dpdk.org
> Cc: david.marchand@redhat.com; jerinj@marvell.com;
> hemant.agrawal@nxp.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Gavin Hu <Gavin.Hu@arm.com>;
> Ruifeng Wang <Ruifeng.Wang@arm.com>; Joyce Kong
> <Joyce.Kong@arm.com>; nd <nd@arm.com>; stable@dpdk.org
> Subject: RE: [PATCH v3 07/12] service: remove rte prefix from static functions
> 
> > From: Phil Yang <phil.yang@arm.com>
> > Sent: Tuesday, March 17, 2020 1:18 AM
> > To: thomas@monjalon.net; Van Haaren, Harry
> <harry.van.haaren@intel.com>;
> > Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> > stephen@networkplumber.org; maxime.coquelin@redhat.com;
> dev@dpdk.org
> > Cc: david.marchand@redhat.com; jerinj@marvell.com;
> hemant.agrawal@nxp.com;
> > Honnappa.Nagarahalli@arm.com; gavin.hu@arm.com;
> ruifeng.wang@arm.com;
> > joyce.kong@arm.com; nd@arm.com; stable@dpdk.org
> > Subject: [PATCH v3 07/12] service: remove rte prefix from static functions
> >
> > Fixes: 3cf5eb1546ed ("service: fix and refactor atomic service accesses")
> > Fixes: 21698354c832 ("service: introduce service cores concept")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> 
> 
> This patchset needs a rebase since the EAL file movement got merged,
> however I'll review here so we can include some Acks etc and make
> progress.
> 
> Is this really a "Fix"? The internal function names were not exported
> in the .map file, so are not part of public ABI. This is an internal
> naming improvement (thanks for doing cleanup), but I don't think the
> Fixes: tags make sense?
> 
> Also I'm not sure if we want to port this patch back to stable? Changing
> (internal) function names seems like unnecessary churn, and hence risk to a
> stable release, without any benefit?
OK.
I will remove these tags in the next version and split the service core patches from the original series into a series by itself.

Thanks,
Phil

> 
> ---
> 
> <snip patch diff>
Phil Yang April 8, 2020, 10:14 a.m. UTC | #4
> -----Original Message-----
> From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Sent: Monday, April 6, 2020 5:35 AM
> To: Phil Yang <Phil.Yang@arm.com>; thomas@monjalon.net;
> harry.van.haaren@intel.com; konstantin.ananyev@intel.com;
> stephen@networkplumber.org; maxime.coquelin@redhat.com;
> dev@dpdk.org
> Cc: david.marchand@redhat.com; jerinj@marvell.com;
> hemant.agrawal@nxp.com; Gavin Hu <Gavin.Hu@arm.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; Joyce Kong <Joyce.Kong@arm.com>; nd
> <nd@arm.com>; stable@dpdk.org; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
> Subject: RE: [PATCH v3 07/12] service: remove rte prefix from static functions
> 
> <snip>
> 
> > Subject: [PATCH v3 07/12] service: remove rte prefix from static functions
> >
> > Fixes: 3cf5eb1546ed ("service: fix and refactor atomic service accesses")
> > Fixes: 21698354c832 ("service: introduce service cores concept")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > ---
> >  lib/librte_eal/common/rte_service.c | 18 +++++++++---------
> >  1 file changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/lib/librte_eal/common/rte_service.c
> > b/lib/librte_eal/common/rte_service.c
> > index b0b78ba..2117726 100644
> > --- a/lib/librte_eal/common/rte_service.c
> > +++ b/lib/librte_eal/common/rte_service.c
> > @@ -336,7 +336,7 @@ rte_service_runstate_get(uint32_t id)  }
> >
> >  static inline void
> > -rte_service_runner_do_callback(struct rte_service_spec_impl *s,
> > +service_runner_do_callback(struct rte_service_spec_impl *s,
> >  			       struct core_state *cs, uint32_t service_idx)  {
> >  	void *userdata = s->spec.callback_userdata; @@ -379,10 +379,10
> > @@ service_run(uint32_t i, struct core_state *cs, uint64_t service_mask,
> >  		if (!rte_atomic32_cmpset((uint32_t *)&s->execute_lock, 0,
> 1))
> >  			return -EBUSY;
> >
> > -		rte_service_runner_do_callback(s, cs, i);
> > +		service_runner_do_callback(s, cs, i);
> >  		rte_atomic32_clear(&s->execute_lock);
> >  	} else
> > -		rte_service_runner_do_callback(s, cs, i);
> > +		service_runner_do_callback(s, cs, i);
> >
> >  	return 0;
> >  }
> > @@ -436,7 +436,7 @@ rte_service_run_iter_on_app_lcore(uint32_t id,
> > uint32_t serialize_mt_unsafe)  }
> >
> >  static int32_t
> > -rte_service_runner_func(void *arg)
> > +service_runner_func(void *arg)
> >  {
> >  	RTE_SET_USED(arg);
> >  	uint32_t i;
> This is a minor comment.
> Since you are touching 'service_runner_func', please consider doing the
> below improvement:
> 
> struct core_state *cs = &lcore_states[lcore];
> while (lcore_states[lcore].runstate == RUNSTATE_RUNNING) {
> 
> The while above can be changed as follows to make it more readable
> 
> while (cs->runstate == RUNSTATE_RUNNING) {
OK. I will clean it up in the next version.

Thanks,
Phil

> 
> > @@ -706,7 +706,7 @@ rte_service_lcore_start(uint32_t lcore)
> >  	 */
> >  	lcore_states[lcore].runstate = RUNSTATE_RUNNING;
> >
> > -	int ret = rte_eal_remote_launch(rte_service_runner_func, 0, lcore);
> > +	int ret = rte_eal_remote_launch(service_runner_func, 0, lcore);
> >  	/* returns -EBUSY if the core is already launched, 0 on success */
> >  	return ret;
> >  }
> > @@ -785,7 +785,7 @@ rte_service_lcore_attr_get(uint32_t lcore, uint32_t
> > attr_id,  }
> >
> <snip>
Van Haaren, Harry April 8, 2020, 10:36 a.m. UTC | #5
> -----Original Message-----
> From: Phil Yang <Phil.Yang@arm.com>
> Sent: Wednesday, April 8, 2020 11:15 AM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>; thomas@monjalon.net;
> Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> stephen@networkplumber.org; maxime.coquelin@redhat.com; dev@dpdk.org
> Cc: david.marchand@redhat.com; jerinj@marvell.com; hemant.agrawal@nxp.com;
> Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Gavin Hu
> <Gavin.Hu@arm.com>; Ruifeng Wang <Ruifeng.Wang@arm.com>; Joyce Kong
> <Joyce.Kong@arm.com>; nd <nd@arm.com>; stable@dpdk.org; nd <nd@arm.com>
> Subject: RE: [PATCH v3 07/12] service: remove rte prefix from static functions
<snip>
> > Is this really a "Fix"? The internal function names were not exported
> > in the .map file, so are not part of public ABI. This is an internal
> > naming improvement (thanks for doing cleanup), but I don't think the
> > Fixes: tags make sense?
> >
> > Also I'm not sure if we want to port this patch back to stable? Changing
> > (internal) function names seems like unnecessary churn, and hence risk to a
> > stable release, without any benefit?
> OK.
> I will remove these tags in the next version and split the service core
> patches from the original series into a series by itself.

Cool - good idea to split.

Perhaps we should focus on getting bugfixes in for the existing code, before doing cleanup? It would make backports easier if churn is minimal.

Suggesting patches order (first to last)
1. bugfixes/things to backport
2. cleanups
3. C11 atomic optimizations


> Thanks,
> Phil

Thanks, and I'll get to reading/reviewing your and Honnappa's feedback later today.

-H
Phil Yang April 8, 2020, 10:49 a.m. UTC | #6
> -----Original Message-----
> From: Van Haaren, Harry <harry.van.haaren@intel.com>
> Sent: Wednesday, April 8, 2020 6:37 PM
> To: Phil Yang <Phil.Yang@arm.com>; thomas@monjalon.net; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>;
> stephen@networkplumber.org; maxime.coquelin@redhat.com;
> dev@dpdk.org
> Cc: david.marchand@redhat.com; jerinj@marvell.com;
> hemant.agrawal@nxp.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Gavin Hu <Gavin.Hu@arm.com>;
> Ruifeng Wang <Ruifeng.Wang@arm.com>; Joyce Kong
> <Joyce.Kong@arm.com>; nd <nd@arm.com>; stable@dpdk.org; nd
> <nd@arm.com>
> Subject: RE: [PATCH v3 07/12] service: remove rte prefix from static functions
> 
> > -----Original Message-----
> > From: Phil Yang <Phil.Yang@arm.com>
> > Sent: Wednesday, April 8, 2020 11:15 AM
> > To: Van Haaren, Harry <harry.van.haaren@intel.com>;
> thomas@monjalon.net;
> > Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> > stephen@networkplumber.org; maxime.coquelin@redhat.com;
> dev@dpdk.org
> > Cc: david.marchand@redhat.com; jerinj@marvell.com;
> hemant.agrawal@nxp.com;
> > Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Gavin Hu
> > <Gavin.Hu@arm.com>; Ruifeng Wang <Ruifeng.Wang@arm.com>; Joyce
> Kong
> > <Joyce.Kong@arm.com>; nd <nd@arm.com>; stable@dpdk.org; nd
> <nd@arm.com>
> > Subject: RE: [PATCH v3 07/12] service: remove rte prefix from static
> functions
> <snip>
> > > Is this really a "Fix"? The internal function names were not exported
> > > in the .map file, so are not part of public ABI. This is an internal
> > > naming improvement (thanks for doing cleanup), but I don't think the
> > > Fixes: tags make sense?
> > >
> > > Also I'm not sure if we want to port this patch back to stable? Changing
> > > (internal) function names seems like unnecessary churn, and hence risk
> to a
> > > stable release, without any benefit?
> > OK.
> > I will remove these tags in the next version and split the service core
> > patches from the original series into a series by itself.
> 
> Cool - good idea to split.
> 
> Perhaps we should focus on getting bugfixes in for the existing code, before
> doing cleanup? It would make backports easier if churn is minimal.
> 
> Suggesting patches order (first to last)
> 1. bugfixes/things to backport
> 2. cleanups
> 3. C11 atomic optimizations

That is a good idea. I will follow this order.

> 
> 
> > Thanks,
> > Phil
> 
> Thanks, and I'll get to reading/reviewing your and Honnappa's feedback later
> today.
> 
> -H

Patch
diff mbox series

diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index b0b78ba..2117726 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -336,7 +336,7 @@  rte_service_runstate_get(uint32_t id)
 }
 
 static inline void
-rte_service_runner_do_callback(struct rte_service_spec_impl *s,
+service_runner_do_callback(struct rte_service_spec_impl *s,
 			       struct core_state *cs, uint32_t service_idx)
 {
 	void *userdata = s->spec.callback_userdata;
@@ -379,10 +379,10 @@  service_run(uint32_t i, struct core_state *cs, uint64_t service_mask,
 		if (!rte_atomic32_cmpset((uint32_t *)&s->execute_lock, 0, 1))
 			return -EBUSY;
 
-		rte_service_runner_do_callback(s, cs, i);
+		service_runner_do_callback(s, cs, i);
 		rte_atomic32_clear(&s->execute_lock);
 	} else
-		rte_service_runner_do_callback(s, cs, i);
+		service_runner_do_callback(s, cs, i);
 
 	return 0;
 }
@@ -436,7 +436,7 @@  rte_service_run_iter_on_app_lcore(uint32_t id, uint32_t serialize_mt_unsafe)
 }
 
 static int32_t
-rte_service_runner_func(void *arg)
+service_runner_func(void *arg)
 {
 	RTE_SET_USED(arg);
 	uint32_t i;
@@ -706,7 +706,7 @@  rte_service_lcore_start(uint32_t lcore)
 	 */
 	lcore_states[lcore].runstate = RUNSTATE_RUNNING;
 
-	int ret = rte_eal_remote_launch(rte_service_runner_func, 0, lcore);
+	int ret = rte_eal_remote_launch(service_runner_func, 0, lcore);
 	/* returns -EBUSY if the core is already launched, 0 on success */
 	return ret;
 }
@@ -785,7 +785,7 @@  rte_service_lcore_attr_get(uint32_t lcore, uint32_t attr_id,
 }
 
 static void
-rte_service_dump_one(FILE *f, struct rte_service_spec_impl *s,
+service_dump_one(FILE *f, struct rte_service_spec_impl *s,
 		     uint64_t all_cycles, uint32_t reset)
 {
 	/* avoid divide by zero */
@@ -818,7 +818,7 @@  rte_service_attr_reset_all(uint32_t id)
 	SERVICE_VALID_GET_OR_ERR_RET(id, s, -EINVAL);
 
 	int reset = 1;
-	rte_service_dump_one(NULL, s, 0, reset);
+	service_dump_one(NULL, s, 0, reset);
 	return 0;
 }
 
@@ -876,7 +876,7 @@  rte_service_dump(FILE *f, uint32_t id)
 		SERVICE_VALID_GET_OR_ERR_RET(id, s, -EINVAL);
 		fprintf(f, "Service %s Summary\n", s->spec.name);
 		uint32_t reset = 0;
-		rte_service_dump_one(f, s, total_cycles, reset);
+		service_dump_one(f, s, total_cycles, reset);
 		return 0;
 	}
 
@@ -886,7 +886,7 @@  rte_service_dump(FILE *f, uint32_t id)
 		if (!service_valid(i))
 			continue;
 		uint32_t reset = 0;
-		rte_service_dump_one(f, &rte_services[i], total_cycles, reset);
+		service_dump_one(f, &rte_services[i], total_cycles, reset);
 	}
 
 	fprintf(f, "Service Cores Summary\n");