[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(-)
  

> 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>
  

<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>
  

> -----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>
  

> -----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>
  

> -----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
  

> -----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