[PATCH] eal: added new `rte_lcore_is_service_lcore` API.

[PATCH] eal: added new `rte_lcore_is_service_lcore` API.

[Pavan Nikhilesh]

This API can be used to test if an lcore(EAL thread) is a service lcore.

Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
---
 lib/librte_eal/common/include/rte_lcore.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/lib/librte_eal/common/include/rte_lcore.h b/lib/librte_eal/common/include/rte_lcore.h
index 50e0d0f..7854ea1 100644
--- a/lib/librte_eal/common/include/rte_lcore.h
+++ b/lib/librte_eal/common/include/rte_lcore.h
@@ -180,6 +180,24 @@ rte_lcore_is_enabled(unsigned lcore_id)
 }
 
 /**
+ * Test if an lcore is service lcore.
+ *
+ * @param lcore_id
+ *   The identifier of the lcore, which MUST be between 0 and
+ *   RTE_MAX_LCORE-1.
+ * @return
+ *   True if the given lcore is service; false otherwise.
+ */
+static inline int
+rte_lcore_is_service_lcore(unsigned lcore_id)
+{
+	struct rte_config *cfg = rte_eal_get_configuration();
+	if (lcore_id >= RTE_MAX_LCORE)
+		return 0;
+	return cfg->lcore_role[lcore_id] == ROLE_SERVICE;
+}
+
+/**
  * Get the next enabled lcore ID.
  *
  * @param i
-- 
2.7.4

Re: [PATCH] eal: added new `rte_lcore_is_service_lcore` API.

[Van Haaren, Harry]

> From: Pavan Nikhilesh [mailto:pbhagavatula@caviumnetworks.com]
> Sent: Wednesday, August 23, 2017 4:10 PM
> To: dev@dpdk.org
> Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; Pavan Nikhilesh
> <pbhagavatula@caviumnetworks.com>
> Subject: [dpdk-dev] [PATCH] eal: added new `rte_lcore_is_service_lcore` API.
> 
> This API can be used to test if an lcore(EAL thread) is a service lcore.
> 
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
> ---
>  lib/librte_eal/common/include/rte_lcore.h | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/lib/librte_eal/common/include/rte_lcore.h
> b/lib/librte_eal/common/include/rte_lcore.h
> index 50e0d0f..7854ea1 100644
> --- a/lib/librte_eal/common/include/rte_lcore.h
> +++ b/lib/librte_eal/common/include/rte_lcore.h
> @@ -180,6 +180,24 @@ rte_lcore_is_enabled(unsigned lcore_id)
>  }
> 
>  /**
> + * Test if an lcore is service lcore.
> + *
> + * @param lcore_id
> + *   The identifier of the lcore, which MUST be between 0 and
> + *   RTE_MAX_LCORE-1.
> + * @return
> + *   True if the given lcore is service; false otherwise.
> + */
> +static inline int
> +rte_lcore_is_service_lcore(unsigned lcore_id)
> +{
> +	struct rte_config *cfg = rte_eal_get_configuration();
> +	if (lcore_id >= RTE_MAX_LCORE)
> +		return 0;
> +	return cfg->lcore_role[lcore_id] == ROLE_SERVICE;
> +}

No header file and Static inline - so this is only to be used internally in the service cores library?
If so, the function should actually be used, instead of only added but not used in the library itself.

Or am I mis-understanding the intent?

-Harry

Re: [PATCH] eal: added new `rte_lcore_is_service_lcore` API.

[Pavan Nikhilesh Bhagavatula]

On Mon, Aug 28, 2017 at 10:59:51AM +0000, Van Haaren, Harry wrote:
> > From: Pavan Nikhilesh [mailto:pbhagavatula@caviumnetworks.com]
> > Sent: Wednesday, August 23, 2017 4:10 PM
> > To: dev@dpdk.org
> > Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; Pavan Nikhilesh
> > <pbhagavatula@caviumnetworks.com>
> > Subject: [dpdk-dev] [PATCH] eal: added new `rte_lcore_is_service_lcore` API.
> >
> > This API can be used to test if an lcore(EAL thread) is a service lcore.
> >
> > Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
> > ---
> >  lib/librte_eal/common/include/rte_lcore.h | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >
> > diff --git a/lib/librte_eal/common/include/rte_lcore.h
> > b/lib/librte_eal/common/include/rte_lcore.h
> > index 50e0d0f..7854ea1 100644
> > --- a/lib/librte_eal/common/include/rte_lcore.h
> > +++ b/lib/librte_eal/common/include/rte_lcore.h
> > @@ -180,6 +180,24 @@ rte_lcore_is_enabled(unsigned lcore_id)
> >  }
> >
> >  /**
> > + * Test if an lcore is service lcore.
> > + *
> > + * @param lcore_id
> > + *   The identifier of the lcore, which MUST be between 0 and
> > + *   RTE_MAX_LCORE-1.
> > + * @return
> > + *   True if the given lcore is service; false otherwise.
> > + */
> > +static inline int
> > +rte_lcore_is_service_lcore(unsigned lcore_id)
> > +{
> > +	struct rte_config *cfg = rte_eal_get_configuration();
> > +	if (lcore_id >= RTE_MAX_LCORE)
> > +		return 0;
> > +	return cfg->lcore_role[lcore_id] == ROLE_SERVICE;
> > +}
>
> No header file and Static inline - so this is only to be used internally in the service cores library?
> If so, the function should actually be used, instead of only added but not used in the library itself.
>

The enum rte_lcore_role_t has ROLE_SERVICE which tells that a particular lcore is
a service lcore as well as an EAL thread some libraries such as rte_timer allow
specific operations only over EAL threads.

The rte_timer lib uses the rte_is_lcore_enabled() call to check if a lcore is an
EAL thread, Which checks if the lcore role is  ROLE_RTE. But it should also
allow timers to be registered on a service core as processing those timers can
be done on them. This new function allows such libraries to check if the role is
ROLE_SERVICE and allow those operations.

Currently, the only rte_timer library has this specific role check. The
following patch shows the usage in rte_timer library.

http://dpdk.org/dev/patchwork/patch/27819/

> Or am I mis-understanding the intent?
>
> -Harry

Thanks,
Pavan.

Re: [PATCH] eal: added new `rte_lcore_is_service_lcore` API.

[Van Haaren, Harry]

> From: Pavan Nikhilesh Bhagavatula [mailto:pbhagavatula@caviumnetworks.com]
> Sent: Monday, August 28, 2017 12:33 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] eal: added new `rte_lcore_is_service_lcore` API.
> 
> On Mon, Aug 28, 2017 at 10:59:51AM +0000, Van Haaren, Harry wrote:
> > > From: Pavan Nikhilesh [mailto:pbhagavatula@caviumnetworks.com]
> > > Sent: Wednesday, August 23, 2017 4:10 PM
> > > To: dev@dpdk.org
> > > Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; Pavan Nikhilesh
> > > <pbhagavatula@caviumnetworks.com>
> > > Subject: [dpdk-dev] [PATCH] eal: added new `rte_lcore_is_service_lcore` API.
> > >
> > > This API can be used to test if an lcore(EAL thread) is a service lcore.
> > >
> > > Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
> > > ---
> > >  lib/librte_eal/common/include/rte_lcore.h | 18 ++++++++++++++++++
> > >  1 file changed, 18 insertions(+)
> > >
> > > diff --git a/lib/librte_eal/common/include/rte_lcore.h
> > > b/lib/librte_eal/common/include/rte_lcore.h
> > > index 50e0d0f..7854ea1 100644
> > > --- a/lib/librte_eal/common/include/rte_lcore.h
> > > +++ b/lib/librte_eal/common/include/rte_lcore.h
> > > @@ -180,6 +180,24 @@ rte_lcore_is_enabled(unsigned lcore_id)
> > >  }
> > >
> > >  /**
> > > + * Test if an lcore is service lcore.
> > > + *
> > > + * @param lcore_id
> > > + *   The identifier of the lcore, which MUST be between 0 and
> > > + *   RTE_MAX_LCORE-1.
> > > + * @return
> > > + *   True if the given lcore is service; false otherwise.
> > > + */
> > > +static inline int
> > > +rte_lcore_is_service_lcore(unsigned lcore_id)
> > > +{
> > > +	struct rte_config *cfg = rte_eal_get_configuration();
> > > +	if (lcore_id >= RTE_MAX_LCORE)
> > > +		return 0;
> > > +	return cfg->lcore_role[lcore_id] == ROLE_SERVICE;
> > > +}
> >
> > No header file and Static inline - so this is only to be used internally in the service
> cores library?
> > If so, the function should actually be used, instead of only added but not used in the
> library itself.
> >
> 
> The enum rte_lcore_role_t has ROLE_SERVICE which tells that a particular lcore is
> a service lcore as well as an EAL thread some libraries such as rte_timer allow
> specific operations only over EAL threads.

Understood that role of cores is important, and that rte_timer might require this information.


> The rte_timer lib uses the rte_is_lcore_enabled() call to check if a lcore is an
> EAL thread, Which checks if the lcore role is  ROLE_RTE. But it should also
> allow timers to be registered on a service core as processing those timers can
> be done on them.

No problem from me here either - although it's the Timers library maintainer that should check this.


> This new function allows such libraries to check if the role is
> ROLE_SERVICE and allow those operations.

If the timers library requires information about service-cores, it should use a public API to retrieve that information. Having "internal" functions between libraries is not nice.

I think a better design would be to add this function as a public function, (add it to the .map files etc) and then call the public function from the timers library.

Does that sound like a good solution? -Harry


> Currently, the only rte_timer library has this specific role check. The
> following patch shows the usage in rte_timer library.
> 
> http://dpdk.org/dev/patchwork/patch/27819/
> 
> > Or am I mis-understanding the intent?
> >
> > -Harry
> 
> Thanks,
> Pavan.

Re: [PATCH] eal: added new `rte_lcore_is_service_lcore` API.

[Pavan Nikhilesh Bhagavatula]

On Mon, Aug 28, 2017 at 01:49:37PM +0000, Van Haaren, Harry wrote:
> > From: Pavan Nikhilesh Bhagavatula [mailto:pbhagavatula@caviumnetworks.com]
> > Sent: Monday, August 28, 2017 12:33 PM
> > To: Van Haaren, Harry <harry.van.haaren@intel.com>
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] eal: added new `rte_lcore_is_service_lcore` API.
> >
> > On Mon, Aug 28, 2017 at 10:59:51AM +0000, Van Haaren, Harry wrote:
> > > > From: Pavan Nikhilesh [mailto:pbhagavatula@caviumnetworks.com]
> > > > Sent: Wednesday, August 23, 2017 4:10 PM
> > > > To: dev@dpdk.org
> > > > Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; Pavan Nikhilesh
> > > > <pbhagavatula@caviumnetworks.com>
> > > > Subject: [dpdk-dev] [PATCH] eal: added new `rte_lcore_is_service_lcore` API.
> > > >
> > > > This API can be used to test if an lcore(EAL thread) is a service lcore.
> > > >
> > > > Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
> > > > ---
> > > >  lib/librte_eal/common/include/rte_lcore.h | 18 ++++++++++++++++++
> > > >  1 file changed, 18 insertions(+)
> > > >
> > > > diff --git a/lib/librte_eal/common/include/rte_lcore.h
> > > > b/lib/librte_eal/common/include/rte_lcore.h
> > > > index 50e0d0f..7854ea1 100644
> > > > --- a/lib/librte_eal/common/include/rte_lcore.h
> > > > +++ b/lib/librte_eal/common/include/rte_lcore.h
> > > > @@ -180,6 +180,24 @@ rte_lcore_is_enabled(unsigned lcore_id)
> > > >  }
> > > >
> > > >  /**
> > > > + * Test if an lcore is service lcore.
> > > > + *
> > > > + * @param lcore_id
> > > > + *   The identifier of the lcore, which MUST be between 0 and
> > > > + *   RTE_MAX_LCORE-1.
> > > > + * @return
> > > > + *   True if the given lcore is service; false otherwise.
> > > > + */
> > > > +static inline int
> > > > +rte_lcore_is_service_lcore(unsigned lcore_id)
> > > > +{
> > > > +	struct rte_config *cfg = rte_eal_get_configuration();
> > > > +	if (lcore_id >= RTE_MAX_LCORE)
> > > > +		return 0;
> > > > +	return cfg->lcore_role[lcore_id] == ROLE_SERVICE;
> > > > +}
> > >
> > > No header file and Static inline - so this is only to be used internally in the service
> > cores library?
> > > If so, the function should actually be used, instead of only added but not used in the
> > library itself.
> > >
> >
> > The enum rte_lcore_role_t has ROLE_SERVICE which tells that a particular lcore is
> > a service lcore as well as an EAL thread some libraries such as rte_timer allow
> > specific operations only over EAL threads.
>
> Understood that role of cores is important, and that rte_timer might require this information.
>
>
> > The rte_timer lib uses the rte_is_lcore_enabled() call to check if a lcore is an
> > EAL thread, Which checks if the lcore role is  ROLE_RTE. But it should also
> > allow timers to be registered on a service core as processing those timers can
> > be done on them.
>
> No problem from me here either - although it's the Timers library maintainer that should check this.
>
>
> > This new function allows such libraries to check if the role is
> > ROLE_SERVICE and allow those operations.
>
> If the timers library requires information about service-cores, it should use a public API to retrieve that information. Having "internal" functions between libraries is not nice.
>
> I think a better design would be to add this function as a public function, (add it to the .map files etc) and then call the public function from the timers library.
>
> Does that sound like a good solution? -Harry
>

The file rte_lcore.h is in librte_eal/common/include I couldn't find a .map
file for eal/common and also other functions that are present in rte_lcore.h
aren't mapped in eal/linuxapp or eal/bsdapp.
I think it is fine as the functions are static inline.

-Pavan

>
> > Currently, the only rte_timer library has this specific role check. The
> > following patch shows the usage in rte_timer library.
> >
> > http://dpdk.org/dev/patchwork/patch/27819/
> >
> > > Or am I mis-understanding the intent?
> > >
> > > -Harry
> >
> > Thanks,
> > Pavan.

Re: [PATCH] eal: added new `rte_lcore_is_service_lcore` API.

[Van Haaren, Harry]

> -----Original Message-----
> From: Pavan Nikhilesh Bhagavatula [mailto:pbhagavatula@caviumnetworks.com]
> Sent: Monday, August 28, 2017 4:10 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] eal: added new `rte_lcore_is_service_lcore` API.
> 
> On Mon, Aug 28, 2017 at 01:49:37PM +0000, Van Haaren, Harry wrote:
> > > From: Pavan Nikhilesh Bhagavatula [mailto:pbhagavatula@caviumnetworks.com]
> > > Sent: Monday, August 28, 2017 12:33 PM
> > > To: Van Haaren, Harry <harry.van.haaren@intel.com>
> > > Cc: dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH] eal: added new `rte_lcore_is_service_lcore` API.
> > >
> > > On Mon, Aug 28, 2017 at 10:59:51AM +0000, Van Haaren, Harry wrote:
> > > > > From: Pavan Nikhilesh [mailto:pbhagavatula@caviumnetworks.com]
> > > > > Sent: Wednesday, August 23, 2017 4:10 PM
> > > > > To: dev@dpdk.org
> > > > > Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; Pavan Nikhilesh
> > > > > <pbhagavatula@caviumnetworks.com>
> > > > > Subject: [dpdk-dev] [PATCH] eal: added new `rte_lcore_is_service_lcore` API.
> > > > >
> > > > > This API can be used to test if an lcore(EAL thread) is a service lcore.
> > > > >
> > > > > Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
> > > > > ---
> > > > >  lib/librte_eal/common/include/rte_lcore.h | 18 ++++++++++++++++++
> > > > >  1 file changed, 18 insertions(+)
> > > > >
> > > > > diff --git a/lib/librte_eal/common/include/rte_lcore.h
> > > > > b/lib/librte_eal/common/include/rte_lcore.h
> > > > > index 50e0d0f..7854ea1 100644
> > > > > --- a/lib/librte_eal/common/include/rte_lcore.h
> > > > > +++ b/lib/librte_eal/common/include/rte_lcore.h
> > > > > @@ -180,6 +180,24 @@ rte_lcore_is_enabled(unsigned lcore_id)
> > > > >  }
> > > > >
> > > > >  /**
> > > > > + * Test if an lcore is service lcore.
> > > > > + *
> > > > > + * @param lcore_id
> > > > > + *   The identifier of the lcore, which MUST be between 0 and
> > > > > + *   RTE_MAX_LCORE-1.
> > > > > + * @return
> > > > > + *   True if the given lcore is service; false otherwise.
> > > > > + */
> > > > > +static inline int
> > > > > +rte_lcore_is_service_lcore(unsigned lcore_id)
> > > > > +{
> > > > > +	struct rte_config *cfg = rte_eal_get_configuration();
> > > > > +	if (lcore_id >= RTE_MAX_LCORE)
> > > > > +		return 0;
> > > > > +	return cfg->lcore_role[lcore_id] == ROLE_SERVICE;
> > > > > +}
> > > >
> > > > No header file and Static inline - so this is only to be used internally in the service
> > > cores library?
> > > > If so, the function should actually be used, instead of only added but not used in the
> > > library itself.
> > > >
> > >
> > > The enum rte_lcore_role_t has ROLE_SERVICE which tells that a particular lcore is
> > > a service lcore as well as an EAL thread some libraries such as rte_timer allow
> > > specific operations only over EAL threads.
> >
> > Understood that role of cores is important, and that rte_timer might require this
> information.
> >
> >
> > > The rte_timer lib uses the rte_is_lcore_enabled() call to check if a lcore is an
> > > EAL thread, Which checks if the lcore role is  ROLE_RTE. But it should also
> > > allow timers to be registered on a service core as processing those timers can
> > > be done on them.
> >
> > No problem from me here either - although it's the Timers library maintainer that should
> check this.
> >
> >
> > > This new function allows such libraries to check if the role is
> > > ROLE_SERVICE and allow those operations.
> >
> > If the timers library requires information about service-cores, it should use a public API
> to retrieve that information. Having "internal" functions between libraries is not nice.
> >
> > I think a better design would be to add this function as a public function, (add it to the
> .map files etc) and then call the public function from the timers library.
> >
> > Does that sound like a good solution? -Harry
> >
> 
> The file rte_lcore.h is in librte_eal/common/include I couldn't find a .map
> file for eal/common and also other functions that are present in rte_lcore.h
> aren't mapped in eal/linuxapp or eal/bsdapp.
> I think it is fine as the functions are static inline.
> 
> -Pavan

OK - I was looking at this from a service core library POV. The intent seems to be to update EAL in order to allow detection of a core having a ROLE_SERVICE. Now I see your intent better, no problem with the approach. Correct that static-inline functions don't need .map file entries (cause they're inlined :)

One technical issue:
> +	if (lcore_id >= RTE_MAX_LCORE)
> +		return 0;

This should return a -ERROR value, as 0 is a valid return value that should indicate one thing (and one thing only) "not a service core".

Please spin a v2 and I'll Ack.

> 
> >
> > > Currently, the only rte_timer library has this specific role check. The
> > > following patch shows the usage in rte_timer library.
> > >
> > > http://dpdk.org/dev/patchwork/patch/27819/
> > >
> > > > Or am I mis-understanding the intent?
> > > >
> > > > -Harry
> > >
> > > Thanks,
> > > Pavan.

Re: [PATCH] eal: added new `rte_lcore_is_service_lcore` API.

[Pavan Nikhilesh Bhagavatula]

On Mon, Aug 28, 2017 at 03:24:06PM +0000, Van Haaren, Harry wrote:
>
>
> > -----Original Message-----
> > From: Pavan Nikhilesh Bhagavatula [mailto:pbhagavatula@caviumnetworks.com]
> > Sent: Monday, August 28, 2017 4:10 PM
> > To: Van Haaren, Harry <harry.van.haaren@intel.com>
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] eal: added new `rte_lcore_is_service_lcore` API.
> >
> > On Mon, Aug 28, 2017 at 01:49:37PM +0000, Van Haaren, Harry wrote:
> > > > From: Pavan Nikhilesh Bhagavatula [mailto:pbhagavatula@caviumnetworks.com]
> > > > Sent: Monday, August 28, 2017 12:33 PM
> > > > To: Van Haaren, Harry <harry.van.haaren@intel.com>
> > > > Cc: dev@dpdk.org
> > > > Subject: Re: [dpdk-dev] [PATCH] eal: added new `rte_lcore_is_service_lcore` API.
> > > >
> > > > On Mon, Aug 28, 2017 at 10:59:51AM +0000, Van Haaren, Harry wrote:
> > > > > > From: Pavan Nikhilesh [mailto:pbhagavatula@caviumnetworks.com]
> > > > > > Sent: Wednesday, August 23, 2017 4:10 PM
> > > > > > To: dev@dpdk.org
> > > > > > Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; Pavan Nikhilesh
> > > > > > <pbhagavatula@caviumnetworks.com>
> > > > > > Subject: [dpdk-dev] [PATCH] eal: added new `rte_lcore_is_service_lcore` API.
> > > > > >
> > > > > > This API can be used to test if an lcore(EAL thread) is a service lcore.
> > > > > >
> > > > > > Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
> > > > > > ---
> > > > > >  lib/librte_eal/common/include/rte_lcore.h | 18 ++++++++++++++++++
> > > > > >  1 file changed, 18 insertions(+)
> > > > > >
> > > > > > diff --git a/lib/librte_eal/common/include/rte_lcore.h
> > > > > > b/lib/librte_eal/common/include/rte_lcore.h
> > > > > > index 50e0d0f..7854ea1 100644
> > > > > > --- a/lib/librte_eal/common/include/rte_lcore.h
> > > > > > +++ b/lib/librte_eal/common/include/rte_lcore.h
> > > > > > @@ -180,6 +180,24 @@ rte_lcore_is_enabled(unsigned lcore_id)
> > > > > >  }
> > > > > >
> > > > > >  /**
> > > > > > + * Test if an lcore is service lcore.
> > > > > > + *
> > > > > > + * @param lcore_id
> > > > > > + *   The identifier of the lcore, which MUST be between 0 and
> > > > > > + *   RTE_MAX_LCORE-1.
> > > > > > + * @return
> > > > > > + *   True if the given lcore is service; false otherwise.
> > > > > > + */
> > > > > > +static inline int
> > > > > > +rte_lcore_is_service_lcore(unsigned lcore_id)
> > > > > > +{
> > > > > > +	struct rte_config *cfg = rte_eal_get_configuration();
> > > > > > +	if (lcore_id >= RTE_MAX_LCORE)
> > > > > > +		return 0;
> > > > > > +	return cfg->lcore_role[lcore_id] == ROLE_SERVICE;
> > > > > > +}
> > > > >
> > > > > No header file and Static inline - so this is only to be used internally in the service
> > > > cores library?
> > > > > If so, the function should actually be used, instead of only added but not used in the
> > > > library itself.
> > > > >
> > > >
> > > > The enum rte_lcore_role_t has ROLE_SERVICE which tells that a particular lcore is
> > > > a service lcore as well as an EAL thread some libraries such as rte_timer allow
> > > > specific operations only over EAL threads.
> > >
> > > Understood that role of cores is important, and that rte_timer might require this
> > information.
> > >
> > >
> > > > The rte_timer lib uses the rte_is_lcore_enabled() call to check if a lcore is an
> > > > EAL thread, Which checks if the lcore role is  ROLE_RTE. But it should also
> > > > allow timers to be registered on a service core as processing those timers can
> > > > be done on them.
> > >
> > > No problem from me here either - although it's the Timers library maintainer that should
> > check this.
> > >
> > >
> > > > This new function allows such libraries to check if the role is
> > > > ROLE_SERVICE and allow those operations.
> > >
> > > If the timers library requires information about service-cores, it should use a public API
> > to retrieve that information. Having "internal" functions between libraries is not nice.
> > >
> > > I think a better design would be to add this function as a public function, (add it to the
> > .map files etc) and then call the public function from the timers library.
> > >
> > > Does that sound like a good solution? -Harry
> > >
> >
> > The file rte_lcore.h is in librte_eal/common/include I couldn't find a .map
> > file for eal/common and also other functions that are present in rte_lcore.h
> > aren't mapped in eal/linuxapp or eal/bsdapp.
> > I think it is fine as the functions are static inline.
> >
> > -Pavan
>
> OK - I was looking at this from a service core library POV. The intent seems to be to update EAL in order to allow detection of a core having a ROLE_SERVICE. Now I see your intent better, no problem with the approach. Correct that static-inline functions don't need .map file entries (cause they're inlined :)
>
> One technical issue:
> > +	if (lcore_id >= RTE_MAX_LCORE)
> > +		return 0;
>
> This should return a -ERROR value, as 0 is a valid return value that should indicate one thing (and one thing only) "not a service core".

The function function follows the same structure as rte_lcore_is_enabled i.e.
returns either true(1) or false(0). So, I think returning 0 would be fine?. If
not I'll gladly send a v2.

Thanks for the inputs :). -Pavan.
>
> Please spin a v2 and I'll Ack.
>
> >
> > >
> > > > Currently, the only rte_timer library has this specific role check. The
> > > > following patch shows the usage in rte_timer library.
> > > >
> > > > http://dpdk.org/dev/patchwork/patch/27819/
> > > >
> > > > > Or am I mis-understanding the intent?
> > > > >
> > > > > -Harry
> > > >
> > > > Thanks,
> > > > Pavan.

Re: [PATCH] eal: added new `rte_lcore_is_service_lcore` API.

[Van Haaren, Harry]

> From: Pavan Nikhilesh Bhagavatula [mailto:pbhagavatula@caviumnetworks.com]
> Sent: Monday, August 28, 2017 4:43 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] eal: added new `rte_lcore_is_service_lcore` API.
> 
> On Mon, Aug 28, 2017 at 03:24:06PM +0000, Van Haaren, Harry wrote:
> >
> >
> > > -----Original Message-----
> > > From: Pavan Nikhilesh Bhagavatula [mailto:pbhagavatula@caviumnetworks.com]
> > > Sent: Monday, August 28, 2017 4:10 PM
> > > To: Van Haaren, Harry <harry.van.haaren@intel.com>
> > > Cc: dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH] eal: added new `rte_lcore_is_service_lcore` API.
> > >
> > > On Mon, Aug 28, 2017 at 01:49:37PM +0000, Van Haaren, Harry wrote:
> > > > > From: Pavan Nikhilesh Bhagavatula [mailto:pbhagavatula@caviumnetworks.com]
> > > > > Sent: Monday, August 28, 2017 12:33 PM
> > > > > To: Van Haaren, Harry <harry.van.haaren@intel.com>
> > > > > Cc: dev@dpdk.org
> > > > > Subject: Re: [dpdk-dev] [PATCH] eal: added new `rte_lcore_is_service_lcore` API.
> > > > >
> > > > > On Mon, Aug 28, 2017 at 10:59:51AM +0000, Van Haaren, Harry wrote:
> > > > > > > From: Pavan Nikhilesh [mailto:pbhagavatula@caviumnetworks.com]
> > > > > > > Sent: Wednesday, August 23, 2017 4:10 PM
> > > > > > > To: dev@dpdk.org
> > > > > > > Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; Pavan Nikhilesh
> > > > > > > <pbhagavatula@caviumnetworks.com>
> > > > > > > Subject: [dpdk-dev] [PATCH] eal: added new `rte_lcore_is_service_lcore` API.
> > > > > > >
> > > > > > > This API can be used to test if an lcore(EAL thread) is a service lcore.
> > > > > > >
> > > > > > > Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
> > > > > > > ---
> > > > > > >  lib/librte_eal/common/include/rte_lcore.h | 18 ++++++++++++++++++
> > > > > > >  1 file changed, 18 insertions(+)
> > > > > > >
> > > > > > > diff --git a/lib/librte_eal/common/include/rte_lcore.h
> > > > > > > b/lib/librte_eal/common/include/rte_lcore.h
> > > > > > > index 50e0d0f..7854ea1 100644
> > > > > > > --- a/lib/librte_eal/common/include/rte_lcore.h
> > > > > > > +++ b/lib/librte_eal/common/include/rte_lcore.h
> > > > > > > @@ -180,6 +180,24 @@ rte_lcore_is_enabled(unsigned lcore_id)
> > > > > > >  }
> > > > > > >
> > > > > > >  /**
> > > > > > > + * Test if an lcore is service lcore.
> > > > > > > + *
> > > > > > > + * @param lcore_id
> > > > > > > + *   The identifier of the lcore, which MUST be between 0 and
> > > > > > > + *   RTE_MAX_LCORE-1.
> > > > > > > + * @return
> > > > > > > + *   True if the given lcore is service; false otherwise.
> > > > > > > + */
> > > > > > > +static inline int
> > > > > > > +rte_lcore_is_service_lcore(unsigned lcore_id)
> > > > > > > +{
> > > > > > > +	struct rte_config *cfg = rte_eal_get_configuration();
> > > > > > > +	if (lcore_id >= RTE_MAX_LCORE)
> > > > > > > +		return 0;
> > > > > > > +	return cfg->lcore_role[lcore_id] == ROLE_SERVICE;
> > > > > > > +}
> > > > > >
> > > > > > No header file and Static inline - so this is only to be used internally in the
> service
> > > > > cores library?
> > > > > > If so, the function should actually be used, instead of only added but not used in
> the
> > > > > library itself.
> > > > > >
> > > > >
> > > > > The enum rte_lcore_role_t has ROLE_SERVICE which tells that a particular lcore is
> > > > > a service lcore as well as an EAL thread some libraries such as rte_timer allow
> > > > > specific operations only over EAL threads.
> > > >
> > > > Understood that role of cores is important, and that rte_timer might require this
> > > information.
> > > >
> > > >
> > > > > The rte_timer lib uses the rte_is_lcore_enabled() call to check if a lcore is an
> > > > > EAL thread, Which checks if the lcore role is  ROLE_RTE. But it should also
> > > > > allow timers to be registered on a service core as processing those timers can
> > > > > be done on them.
> > > >
> > > > No problem from me here either - although it's the Timers library maintainer that should
> > > check this.
> > > >
> > > >
> > > > > This new function allows such libraries to check if the role is
> > > > > ROLE_SERVICE and allow those operations.
> > > >
> > > > If the timers library requires information about service-cores, it should use a public
> API
> > > to retrieve that information. Having "internal" functions between libraries is not nice.
> > > >
> > > > I think a better design would be to add this function as a public function, (add it to
> the
> > > .map files etc) and then call the public function from the timers library.
> > > >
> > > > Does that sound like a good solution? -Harry
> > > >
> > >
> > > The file rte_lcore.h is in librte_eal/common/include I couldn't find a .map
> > > file for eal/common and also other functions that are present in rte_lcore.h
> > > aren't mapped in eal/linuxapp or eal/bsdapp.
> > > I think it is fine as the functions are static inline.
> > >
> > > -Pavan
> >
> > OK - I was looking at this from a service core library POV. The intent seems to be to update
> EAL in order to allow detection of a core having a ROLE_SERVICE. Now I see your intent better,
> no problem with the approach. Correct that static-inline functions don't need .map file
> entries (cause they're inlined :)
> >
> > One technical issue:
> > > +	if (lcore_id >= RTE_MAX_LCORE)
> > > +		return 0;
> >
> > This should return a -ERROR value, as 0 is a valid return value that should indicate one
> thing (and one thing only) "not a service core".
> 
> The function function follows the same structure as rte_lcore_is_enabled i.e.
> returns either true(1) or false(0). So, I think returning 0 would be fine?. If
> not I'll gladly send a v2.

I looked that that function too - I'm not sure what's better API design...
Lets stay consistent with other functions in the file; so keep your current implementation.

Note that these service core patches depend on the Service Cores rework patchset (currently
v2 available here: http://dpdk.org/dev/patchwork/patch/27684/ )

@Pavan, if you have time to Ack the patches this one is based on that would be fantastic.

Acked-by: Harry van Haaren <harry.van.haaren@intel.com>

Re: [PATCH] eal: added new `rte_lcore_is_service_lcore` API.

[Pavan Nikhilesh Bhagavatula]

On Tue, Aug 29, 2017 at 01:17:18PM +0000, Van Haaren, Harry wrote:
> > From: Pavan Nikhilesh Bhagavatula [mailto:pbhagavatula@caviumnetworks.com]
> > Sent: Monday, August 28, 2017 4:43 PM
> > To: Van Haaren, Harry <harry.van.haaren@intel.com>
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] eal: added new `rte_lcore_is_service_lcore` API.
> >
> > On Mon, Aug 28, 2017 at 03:24:06PM +0000, Van Haaren, Harry wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Pavan Nikhilesh Bhagavatula [mailto:pbhagavatula@caviumnetworks.com]
> > > > Sent: Monday, August 28, 2017 4:10 PM
> > > > To: Van Haaren, Harry <harry.van.haaren@intel.com>
> > > > Cc: dev@dpdk.org
> > > > Subject: Re: [dpdk-dev] [PATCH] eal: added new `rte_lcore_is_service_lcore` API.
> > > >
> > > > On Mon, Aug 28, 2017 at 01:49:37PM +0000, Van Haaren, Harry wrote:
> > > > > > From: Pavan Nikhilesh Bhagavatula [mailto:pbhagavatula@caviumnetworks.com]
> > > > > > Sent: Monday, August 28, 2017 12:33 PM
> > > > > > To: Van Haaren, Harry <harry.van.haaren@intel.com>
> > > > > > Cc: dev@dpdk.org
> > > > > > Subject: Re: [dpdk-dev] [PATCH] eal: added new `rte_lcore_is_service_lcore` API.
> > > > > >
> > > > > > On Mon, Aug 28, 2017 at 10:59:51AM +0000, Van Haaren, Harry wrote:
> > > > > > > > From: Pavan Nikhilesh [mailto:pbhagavatula@caviumnetworks.com]
> > > > > > > > Sent: Wednesday, August 23, 2017 4:10 PM
> > > > > > > > To: dev@dpdk.org
> > > > > > > > Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; Pavan Nikhilesh
> > > > > > > > <pbhagavatula@caviumnetworks.com>
> > > > > > > > Subject: [dpdk-dev] [PATCH] eal: added new `rte_lcore_is_service_lcore` API.
> > > > > > > >
> > > > > > > > This API can be used to test if an lcore(EAL thread) is a service lcore.
> > > > > > > >
> > > > > > > > Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
> > > > > > > > ---
> > > > > > > >  lib/librte_eal/common/include/rte_lcore.h | 18 ++++++++++++++++++
> > > > > > > >  1 file changed, 18 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/lib/librte_eal/common/include/rte_lcore.h
> > > > > > > > b/lib/librte_eal/common/include/rte_lcore.h
> > > > > > > > index 50e0d0f..7854ea1 100644
> > > > > > > > --- a/lib/librte_eal/common/include/rte_lcore.h
> > > > > > > > +++ b/lib/librte_eal/common/include/rte_lcore.h
> > > > > > > > @@ -180,6 +180,24 @@ rte_lcore_is_enabled(unsigned lcore_id)
> > > > > > > >  }
> > > > > > > >
> > > > > > > >  /**
> > > > > > > > + * Test if an lcore is service lcore.
> > > > > > > > + *
> > > > > > > > + * @param lcore_id
> > > > > > > > + *   The identifier of the lcore, which MUST be between 0 and
> > > > > > > > + *   RTE_MAX_LCORE-1.
> > > > > > > > + * @return
> > > > > > > > + *   True if the given lcore is service; false otherwise.
> > > > > > > > + */
> > > > > > > > +static inline int
> > > > > > > > +rte_lcore_is_service_lcore(unsigned lcore_id)
> > > > > > > > +{
> > > > > > > > +	struct rte_config *cfg = rte_eal_get_configuration();
> > > > > > > > +	if (lcore_id >= RTE_MAX_LCORE)
> > > > > > > > +		return 0;
> > > > > > > > +	return cfg->lcore_role[lcore_id] == ROLE_SERVICE;
> > > > > > > > +}
> > > > > > >
> > > > > > > No header file and Static inline - so this is only to be used internally in the
> > service
> > > > > > cores library?
> > > > > > > If so, the function should actually be used, instead of only added but not used in
> > the
> > > > > > library itself.
> > > > > > >
> > > > > >
> > > > > > The enum rte_lcore_role_t has ROLE_SERVICE which tells that a particular lcore is
> > > > > > a service lcore as well as an EAL thread some libraries such as rte_timer allow
> > > > > > specific operations only over EAL threads.
> > > > >
> > > > > Understood that role of cores is important, and that rte_timer might require this
> > > > information.
> > > > >
> > > > >
> > > > > > The rte_timer lib uses the rte_is_lcore_enabled() call to check if a lcore is an
> > > > > > EAL thread, Which checks if the lcore role is  ROLE_RTE. But it should also
> > > > > > allow timers to be registered on a service core as processing those timers can
> > > > > > be done on them.
> > > > >
> > > > > No problem from me here either - although it's the Timers library maintainer that should
> > > > check this.
> > > > >
> > > > >
> > > > > > This new function allows such libraries to check if the role is
> > > > > > ROLE_SERVICE and allow those operations.
> > > > >
> > > > > If the timers library requires information about service-cores, it should use a public
> > API
> > > > to retrieve that information. Having "internal" functions between libraries is not nice.
> > > > >
> > > > > I think a better design would be to add this function as a public function, (add it to
> > the
> > > > .map files etc) and then call the public function from the timers library.
> > > > >
> > > > > Does that sound like a good solution? -Harry
> > > > >
> > > >
> > > > The file rte_lcore.h is in librte_eal/common/include I couldn't find a .map
> > > > file for eal/common and also other functions that are present in rte_lcore.h
> > > > aren't mapped in eal/linuxapp or eal/bsdapp.
> > > > I think it is fine as the functions are static inline.
> > > >
> > > > -Pavan
> > >
> > > OK - I was looking at this from a service core library POV. The intent seems to be to update
> > EAL in order to allow detection of a core having a ROLE_SERVICE. Now I see your intent better,
> > no problem with the approach. Correct that static-inline functions don't need .map file
> > entries (cause they're inlined :)
> > >
> > > One technical issue:
> > > > +	if (lcore_id >= RTE_MAX_LCORE)
> > > > +		return 0;
> > >
> > > This should return a -ERROR value, as 0 is a valid return value that should indicate one
> > thing (and one thing only) "not a service core".
> >
> > The function function follows the same structure as rte_lcore_is_enabled i.e.
> > returns either true(1) or false(0). So, I think returning 0 would be fine?. If
> > not I'll gladly send a v2.
>
> I looked that that function too - I'm not sure what's better API design...
> Lets stay consistent with other functions in the file; so keep your current implementation.
>
> Note that these service core patches depend on the Service Cores rework patchset (currently
> v2 available here: http://dpdk.org/dev/patchwork/patch/27684/ )
>
> @Pavan, if you have time to Ack the patches this one is based on that would be fantastic.

Sure Harry will go through the patch set.

>
> Acked-by: Harry van Haaren <harry.van.haaren@intel.com>

Thanks,
Pavan.

Re: [PATCH] eal: added new `rte_lcore_is_service_lcore` API.

[Thomas Monjalon]

28/08/2017 17:09, Pavan Nikhilesh Bhagavatula:
> On Mon, Aug 28, 2017 at 01:49:37PM +0000, Van Haaren, Harry wrote:
> > From: Pavan Nikhilesh Bhagavatula [mailto:pbhagavatula@caviumnetworks.com]
> > > On Mon, Aug 28, 2017 at 10:59:51AM +0000, Van Haaren, Harry wrote:
> > > > From: Pavan Nikhilesh [mailto:pbhagavatula@caviumnetworks.com]
> > > > > --- a/lib/librte_eal/common/include/rte_lcore.h
> > > > > +++ b/lib/librte_eal/common/include/rte_lcore.h
> > > > > @@ -180,6 +180,24 @@ rte_lcore_is_enabled(unsigned lcore_id)
> > > > >  }
> > > > >
> > > > >  /**
> > > > > + * Test if an lcore is service lcore.
> > > > > + *
> > > > > + * @param lcore_id
> > > > > + *   The identifier of the lcore, which MUST be between 0 and
> > > > > + *   RTE_MAX_LCORE-1.
> > > > > + * @return
> > > > > + *   True if the given lcore is service; false otherwise.
> > > > > + */
> > > > > +static inline int
> > > > > +rte_lcore_is_service_lcore(unsigned lcore_id)
> > > > > +{
> > > > > +	struct rte_config *cfg = rte_eal_get_configuration();
> > > > > +	if (lcore_id >= RTE_MAX_LCORE)
> > > > > +		return 0;
> > > > > +	return cfg->lcore_role[lcore_id] == ROLE_SERVICE;
> > > > > +}
> > > >
> > > > No header file and Static inline - so this is only to be used internally in the service
> > > cores library?
> > > > If so, the function should actually be used, instead of only added but not used in the
> > > library itself.
> > > >
> > >
> > > The enum rte_lcore_role_t has ROLE_SERVICE which tells that a particular lcore is
> > > a service lcore as well as an EAL thread some libraries such as rte_timer allow
> > > specific operations only over EAL threads.
> >
> > Understood that role of cores is important, and that rte_timer might require this information.
> >
> >
> > > The rte_timer lib uses the rte_is_lcore_enabled() call to check if a lcore is an
> > > EAL thread, Which checks if the lcore role is  ROLE_RTE. But it should also
> > > allow timers to be registered on a service core as processing those timers can
> > > be done on them.
> >
> > No problem from me here either - although it's the Timers library maintainer that should check this.
> >
> >
> > > This new function allows such libraries to check if the role is
> > > ROLE_SERVICE and allow those operations.
> >
> > If the timers library requires information about service-cores, it should use a public API to retrieve that information. Having "internal" functions between libraries is not nice.
> >
> > I think a better design would be to add this function as a public function, (add it to the .map files etc) and then call the public function from the timers library.
> >
> > Does that sound like a good solution? -Harry
> >
> 
> The file rte_lcore.h is in librte_eal/common/include I couldn't find a .map
> file for eal/common and also other functions that are present in rte_lcore.h
> aren't mapped in eal/linuxapp or eal/bsdapp.
> I think it is fine as the functions are static inline.

We must avoid adding more inline functions without a good justification.
The inline functions are tolerated for performance reasons only.

We could also choose to add this function to rte_service.h ?

Re: [PATCH] eal: added new `rte_lcore_is_service_lcore` API.

[Van Haaren, Harry]

> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Friday, September 15, 2017 2:53 PM
> To: Pavan Nikhilesh Bhagavatula <pbhagavatula@caviumnetworks.com>; Van Haaren,
> Harry <harry.van.haaren@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] eal: added new `rte_lcore_is_service_lcore`
> API.
> 
> 28/08/2017 17:09, Pavan Nikhilesh Bhagavatula:
> > On Mon, Aug 28, 2017 at 01:49:37PM +0000, Van Haaren, Harry wrote:
> > > From: Pavan Nikhilesh Bhagavatula [mailto:pbhagavatula@caviumnetworks.com]
> > > > On Mon, Aug 28, 2017 at 10:59:51AM +0000, Van Haaren, Harry wrote:
> > > > > From: Pavan Nikhilesh [mailto:pbhagavatula@caviumnetworks.com]
> > > > > > --- a/lib/librte_eal/common/include/rte_lcore.h
> > > > > > +++ b/lib/librte_eal/common/include/rte_lcore.h
> > > > > > @@ -180,6 +180,24 @@ rte_lcore_is_enabled(unsigned lcore_id)
> > > > > >  }
> > > > > >
> > > > > >  /**
> > > > > > + * Test if an lcore is service lcore.
> > > > > > + *
> > > > > > + * @param lcore_id
> > > > > > + *   The identifier of the lcore, which MUST be between 0 and
> > > > > > + *   RTE_MAX_LCORE-1.
> > > > > > + * @return
> > > > > > + *   True if the given lcore is service; false otherwise.
> > > > > > + */
> > > > > > +static inline int
> > > > > > +rte_lcore_is_service_lcore(unsigned lcore_id)
> > > > > > +{
> > > > > > +	struct rte_config *cfg = rte_eal_get_configuration();
> > > > > > +	if (lcore_id >= RTE_MAX_LCORE)
> > > > > > +		return 0;
> > > > > > +	return cfg->lcore_role[lcore_id] == ROLE_SERVICE;
> > > > > > +}
> > > > >
> > > > > No header file and Static inline - so this is only to be used
> internally in the service
> > > > cores library?
> > > > > If so, the function should actually be used, instead of only added but
> not used in the
> > > > library itself.
> > > > >
> > > >
> > > > The enum rte_lcore_role_t has ROLE_SERVICE which tells that a particular
> lcore is
> > > > a service lcore as well as an EAL thread some libraries such as rte_timer
> allow
> > > > specific operations only over EAL threads.
> > >
> > > Understood that role of cores is important, and that rte_timer might
> require this information.
> > >
> > >
> > > > The rte_timer lib uses the rte_is_lcore_enabled() call to check if a
> lcore is an
> > > > EAL thread, Which checks if the lcore role is  ROLE_RTE. But it should
> also
> > > > allow timers to be registered on a service core as processing those
> timers can
> > > > be done on them.
> > >
> > > No problem from me here either - although it's the Timers library
> maintainer that should check this.
> > >
> > >
> > > > This new function allows such libraries to check if the role is
> > > > ROLE_SERVICE and allow those operations.
> > >
> > > If the timers library requires information about service-cores, it should
> use a public API to retrieve that information. Having "internal" functions
> between libraries is not nice.
> > >
> > > I think a better design would be to add this function as a public function,
> (add it to the .map files etc) and then call the public function from the
> timers library.
> > >
> > > Does that sound like a good solution? -Harry
> > >
> >
> > The file rte_lcore.h is in librte_eal/common/include I couldn't find a .map
> > file for eal/common and also other functions that are present in rte_lcore.h
> > aren't mapped in eal/linuxapp or eal/bsdapp.
> > I think it is fine as the functions are static inline.
> 
> We must avoid adding more inline functions without a good justification.
> The inline functions are tolerated for performance reasons only.
> 
> We could also choose to add this function to rte_service.h ?

Yes that is an option, and OK with me.

@Pavan what do you think of adding it to service.h, implement in .c and add to .map? 

Re: [PATCH] eal: added new `rte_lcore_is_service_lcore` API.

[Pavan Nikhilesh Bhagavatula]

On Fri, Sep 15, 2017 at 01:57:42PM +0000, Van Haaren, Harry wrote:
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Sent: Friday, September 15, 2017 2:53 PM
> > To: Pavan Nikhilesh Bhagavatula <pbhagavatula@caviumnetworks.com>; Van Haaren,
> > Harry <harry.van.haaren@intel.com>
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] eal: added new `rte_lcore_is_service_lcore`
> > API.
> >
> > 28/08/2017 17:09, Pavan Nikhilesh Bhagavatula:
> > > On Mon, Aug 28, 2017 at 01:49:37PM +0000, Van Haaren, Harry wrote:
> > > > From: Pavan Nikhilesh Bhagavatula [mailto:pbhagavatula@caviumnetworks.com]
> > > > > On Mon, Aug 28, 2017 at 10:59:51AM +0000, Van Haaren, Harry wrote:
> > > > > > From: Pavan Nikhilesh [mailto:pbhagavatula@caviumnetworks.com]
> > > > > > > --- a/lib/librte_eal/common/include/rte_lcore.h
> > > > > > > +++ b/lib/librte_eal/common/include/rte_lcore.h
> > > > > > > @@ -180,6 +180,24 @@ rte_lcore_is_enabled(unsigned lcore_id)
> > > > > > >  }
> > > > > > >
> > > > > > >  /**
> > > > > > > + * Test if an lcore is service lcore.
> > > > > > > + *
> > > > > > > + * @param lcore_id
> > > > > > > + *   The identifier of the lcore, which MUST be between 0 and
> > > > > > > + *   RTE_MAX_LCORE-1.
> > > > > > > + * @return
> > > > > > > + *   True if the given lcore is service; false otherwise.
> > > > > > > + */
> > > > > > > +static inline int
> > > > > > > +rte_lcore_is_service_lcore(unsigned lcore_id)
> > > > > > > +{
> > > > > > > +	struct rte_config *cfg = rte_eal_get_configuration();
> > > > > > > +	if (lcore_id >= RTE_MAX_LCORE)
> > > > > > > +		return 0;
> > > > > > > +	return cfg->lcore_role[lcore_id] == ROLE_SERVICE;
> > > > > > > +}
> > > > > >
> > > > > > No header file and Static inline - so this is only to be used
> > internally in the service
> > > > > cores library?
> > > > > > If so, the function should actually be used, instead of only added but
> > not used in the
> > > > > library itself.
> > > > > >
> > > > >
> > > > > The enum rte_lcore_role_t has ROLE_SERVICE which tells that a particular
> > lcore is
> > > > > a service lcore as well as an EAL thread some libraries such as rte_timer
> > allow
> > > > > specific operations only over EAL threads.
> > > >
> > > > Understood that role of cores is important, and that rte_timer might
> > require this information.
> > > >
> > > >
> > > > > The rte_timer lib uses the rte_is_lcore_enabled() call to check if a
> > lcore is an
> > > > > EAL thread, Which checks if the lcore role is  ROLE_RTE. But it should
> > also
> > > > > allow timers to be registered on a service core as processing those
> > timers can
> > > > > be done on them.
> > > >
> > > > No problem from me here either - although it's the Timers library
> > maintainer that should check this.
> > > >
> > > >
> > > > > This new function allows such libraries to check if the role is
> > > > > ROLE_SERVICE and allow those operations.
> > > >
> > > > If the timers library requires information about service-cores, it should
> > use a public API to retrieve that information. Having "internal" functions
> > between libraries is not nice.
> > > >
> > > > I think a better design would be to add this function as a public function,
> > (add it to the .map files etc) and then call the public function from the
> > timers library.
> > > >
> > > > Does that sound like a good solution? -Harry
> > > >
> > >
> > > The file rte_lcore.h is in librte_eal/common/include I couldn't find a .map
> > > file for eal/common and also other functions that are present in rte_lcore.h
> > > aren't mapped in eal/linuxapp or eal/bsdapp.
> > > I think it is fine as the functions are static inline.
> >
> > We must avoid adding more inline functions without a good justification.
> > The inline functions are tolerated for performance reasons only.
> >
> > We could also choose to add this function to rte_service.h ?
>
> Yes that is an option, and OK with me.
>
> @Pavan what do you think of adding it to service.h, implement in .c and add to .map?
>

The ROLE_SERVICE/ROLE_RTE defines the role of a lcore so it made sense to put
it in rte_lcore.h as lcore properties are accessed mostly through this header.
I'm fine with adding it to service.h as suggested by Harry.

-Pavan

Re: [PATCH] eal: added new `rte_lcore_is_service_lcore` API.

[Van Haaren, Harry]

> -----Original Message-----
> From: Pavan Nikhilesh Bhagavatula [mailto:pbhagavatula@caviumnetworks.com]
> Sent: Friday, September 15, 2017 3:42 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>
> Cc: thomas@monjalon.net; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] eal: added new `rte_lcore_is_service_lcore`
> API.
> 
> On Fri, Sep 15, 2017 at 01:57:42PM +0000, Van Haaren, Harry wrote:
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > Sent: Friday, September 15, 2017 2:53 PM
> > > To: Pavan Nikhilesh Bhagavatula <pbhagavatula@caviumnetworks.com>; Van
> Haaren,
> > > Harry <harry.van.haaren@intel.com>
> > > Cc: dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH] eal: added new `rte_lcore_is_service_lcore`
> > > API.
> > >
> > > 28/08/2017 17:09, Pavan Nikhilesh Bhagavatula:
> > > > On Mon, Aug 28, 2017 at 01:49:37PM +0000, Van Haaren, Harry wrote:
> > > > > From: Pavan Nikhilesh Bhagavatula
> [mailto:pbhagavatula@caviumnetworks.com]
> > > > > > On Mon, Aug 28, 2017 at 10:59:51AM +0000, Van Haaren, Harry wrote:
> > > > > > > From: Pavan Nikhilesh [mailto:pbhagavatula@caviumnetworks.com]
> > > > > > > > --- a/lib/librte_eal/common/include/rte_lcore.h
> > > > > > > > +++ b/lib/librte_eal/common/include/rte_lcore.h
> > > > > > > > @@ -180,6 +180,24 @@ rte_lcore_is_enabled(unsigned lcore_id)
> > > > > > > >  }
> > > > > > > >
> > > > > > > >  /**
> > > > > > > > + * Test if an lcore is service lcore.
> > > > > > > > + *
> > > > > > > > + * @param lcore_id
> > > > > > > > + *   The identifier of the lcore, which MUST be between 0 and
> > > > > > > > + *   RTE_MAX_LCORE-1.
> > > > > > > > + * @return
> > > > > > > > + *   True if the given lcore is service; false otherwise.
> > > > > > > > + */
> > > > > > > > +static inline int
> > > > > > > > +rte_lcore_is_service_lcore(unsigned lcore_id)
> > > > > > > > +{
> > > > > > > > +	struct rte_config *cfg = rte_eal_get_configuration();
> > > > > > > > +	if (lcore_id >= RTE_MAX_LCORE)
> > > > > > > > +		return 0;
> > > > > > > > +	return cfg->lcore_role[lcore_id] == ROLE_SERVICE;
> > > > > > > > +}
> > > > > > >
> > > > > > > No header file and Static inline - so this is only to be used
> > > internally in the service
> > > > > > cores library?
> > > > > > > If so, the function should actually be used, instead of only added
> but
> > > not used in the
> > > > > > library itself.
> > > > > > >
> > > > > >
> > > > > > The enum rte_lcore_role_t has ROLE_SERVICE which tells that a
> particular
> > > lcore is
> > > > > > a service lcore as well as an EAL thread some libraries such as
> rte_timer
> > > allow
> > > > > > specific operations only over EAL threads.
> > > > >
> > > > > Understood that role of cores is important, and that rte_timer might
> > > require this information.
> > > > >
> > > > >
> > > > > > The rte_timer lib uses the rte_is_lcore_enabled() call to check if a
> > > lcore is an
> > > > > > EAL thread, Which checks if the lcore role is  ROLE_RTE. But it
> should
> > > also
> > > > > > allow timers to be registered on a service core as processing those
> > > timers can
> > > > > > be done on them.
> > > > >
> > > > > No problem from me here either - although it's the Timers library
> > > maintainer that should check this.
> > > > >
> > > > >
> > > > > > This new function allows such libraries to check if the role is
> > > > > > ROLE_SERVICE and allow those operations.
> > > > >
> > > > > If the timers library requires information about service-cores, it
> should
> > > use a public API to retrieve that information. Having "internal" functions
> > > between libraries is not nice.
> > > > >
> > > > > I think a better design would be to add this function as a public
> function,
> > > (add it to the .map files etc) and then call the public function from the
> > > timers library.
> > > > >
> > > > > Does that sound like a good solution? -Harry
> > > > >
> > > >
> > > > The file rte_lcore.h is in librte_eal/common/include I couldn't find a
> .map
> > > > file for eal/common and also other functions that are present in
> rte_lcore.h
> > > > aren't mapped in eal/linuxapp or eal/bsdapp.
> > > > I think it is fine as the functions are static inline.
> > >
> > > We must avoid adding more inline functions without a good justification.
> > > The inline functions are tolerated for performance reasons only.
> > >
> > > We could also choose to add this function to rte_service.h ?
> >
> > Yes that is an option, and OK with me.
> >
> > @Pavan what do you think of adding it to service.h, implement in .c and add
> to .map?
> >
> 
> The ROLE_SERVICE/ROLE_RTE defines the role of a lcore so it made sense to put
> it in rte_lcore.h as lcore properties are accessed mostly through this header.
> I'm fine with adding it to service.h as suggested by Harry.
> 
> -Pavan

*as suggested by Thomas ;)

Initially I thought it made more sense in lcore.h too, however the application
should only require knowing if core X is a service core if it cares about
services / service-cores, hence I'm fine with rte_service.h too.

-Harry

Re: [PATCH] eal: added new `rte_lcore_is_service_lcore` API.

[Pavan Nikhilesh Bhagavatula]

On Fri, Sep 15, 2017 at 02:44:57PM +0000, Van Haaren, Harry wrote:
>
>
> > -----Original Message-----
> > From: Pavan Nikhilesh Bhagavatula [mailto:pbhagavatula@caviumnetworks.com]
> > Sent: Friday, September 15, 2017 3:42 PM
> > To: Van Haaren, Harry <harry.van.haaren@intel.com>
> > Cc: thomas@monjalon.net; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] eal: added new `rte_lcore_is_service_lcore`
> > API.
> >
> > On Fri, Sep 15, 2017 at 01:57:42PM +0000, Van Haaren, Harry wrote:
> > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > Sent: Friday, September 15, 2017 2:53 PM
> > > > To: Pavan Nikhilesh Bhagavatula <pbhagavatula@caviumnetworks.com>; Van
> > Haaren,
> > > > Harry <harry.van.haaren@intel.com>
> > > > Cc: dev@dpdk.org
> > > > Subject: Re: [dpdk-dev] [PATCH] eal: added new `rte_lcore_is_service_lcore`
> > > > API.
> > > >
> > > > 28/08/2017 17:09, Pavan Nikhilesh Bhagavatula:
> > > > > On Mon, Aug 28, 2017 at 01:49:37PM +0000, Van Haaren, Harry wrote:
> > > > > > From: Pavan Nikhilesh Bhagavatula
> > [mailto:pbhagavatula@caviumnetworks.com]
> > > > > > > On Mon, Aug 28, 2017 at 10:59:51AM +0000, Van Haaren, Harry wrote:
> > > > > > > > From: Pavan Nikhilesh [mailto:pbhagavatula@caviumnetworks.com]
> > > > > > > > > --- a/lib/librte_eal/common/include/rte_lcore.h
> > > > > > > > > +++ b/lib/librte_eal/common/include/rte_lcore.h
> > > > > > > > > @@ -180,6 +180,24 @@ rte_lcore_is_enabled(unsigned lcore_id)
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > >  /**
> > > > > > > > > + * Test if an lcore is service lcore.
> > > > > > > > > + *
> > > > > > > > > + * @param lcore_id
> > > > > > > > > + *   The identifier of the lcore, which MUST be between 0 and
> > > > > > > > > + *   RTE_MAX_LCORE-1.
> > > > > > > > > + * @return
> > > > > > > > > + *   True if the given lcore is service; false otherwise.
> > > > > > > > > + */
> > > > > > > > > +static inline int
> > > > > > > > > +rte_lcore_is_service_lcore(unsigned lcore_id)
> > > > > > > > > +{
> > > > > > > > > +	struct rte_config *cfg = rte_eal_get_configuration();
> > > > > > > > > +	if (lcore_id >= RTE_MAX_LCORE)
> > > > > > > > > +		return 0;
> > > > > > > > > +	return cfg->lcore_role[lcore_id] == ROLE_SERVICE;
> > > > > > > > > +}
> > > > > > > >
> > > > > > > > No header file and Static inline - so this is only to be used
> > > > internally in the service
> > > > > > > cores library?
> > > > > > > > If so, the function should actually be used, instead of only added
> > but
> > > > not used in the
> > > > > > > library itself.
> > > > > > > >
> > > > > > >
> > > > > > > The enum rte_lcore_role_t has ROLE_SERVICE which tells that a
> > particular
> > > > lcore is
> > > > > > > a service lcore as well as an EAL thread some libraries such as
> > rte_timer
> > > > allow
> > > > > > > specific operations only over EAL threads.
> > > > > >
> > > > > > Understood that role of cores is important, and that rte_timer might
> > > > require this information.
> > > > > >
> > > > > >
> > > > > > > The rte_timer lib uses the rte_is_lcore_enabled() call to check if a
> > > > lcore is an
> > > > > > > EAL thread, Which checks if the lcore role is  ROLE_RTE. But it
> > should
> > > > also
> > > > > > > allow timers to be registered on a service core as processing those
> > > > timers can
> > > > > > > be done on them.
> > > > > >
> > > > > > No problem from me here either - although it's the Timers library
> > > > maintainer that should check this.
> > > > > >
> > > > > >
> > > > > > > This new function allows such libraries to check if the role is
> > > > > > > ROLE_SERVICE and allow those operations.
> > > > > >
> > > > > > If the timers library requires information about service-cores, it
> > should
> > > > use a public API to retrieve that information. Having "internal" functions
> > > > between libraries is not nice.
> > > > > >
> > > > > > I think a better design would be to add this function as a public
> > function,
> > > > (add it to the .map files etc) and then call the public function from the
> > > > timers library.
> > > > > >
> > > > > > Does that sound like a good solution? -Harry
> > > > > >
> > > > >
> > > > > The file rte_lcore.h is in librte_eal/common/include I couldn't find a
> > .map
> > > > > file for eal/common and also other functions that are present in
> > rte_lcore.h
> > > > > aren't mapped in eal/linuxapp or eal/bsdapp.
> > > > > I think it is fine as the functions are static inline.
> > > >
> > > > We must avoid adding more inline functions without a good justification.
> > > > The inline functions are tolerated for performance reasons only.
> > > >
> > > > We could also choose to add this function to rte_service.h ?
> > >
> > > Yes that is an option, and OK with me.
> > >
> > > @Pavan what do you think of adding it to service.h, implement in .c and add
> > to .map?
> > >
> >
> > The ROLE_SERVICE/ROLE_RTE defines the role of a lcore so it made sense to put
> > it in rte_lcore.h as lcore properties are accessed mostly through this header.
> > I'm fine with adding it to service.h as suggested by Harry.
> >
> > -Pavan
>
> *as suggested by Thomas ;)
>
> Initially I thought it made more sense in lcore.h too, however the application
> should only require knowing if core X is a service core if it cares about
> services / service-cores, hence I'm fine with rte_service.h too.
>
> -Harry
>
Agreed, will spin up a v2.

Thanks,
Pavan
>
>
>

Re: [PATCH] eal: added new `rte_lcore_is_service_lcore` API.

[Thomas Monjalon]

15/09/2017 16:59, Pavan Nikhilesh Bhagavatula:
> On Fri, Sep 15, 2017 at 02:44:57PM +0000, Van Haaren, Harry wrote:

> > > > > We could also choose to add this function to rte_service.h ?
> > > >
> > > > Yes that is an option, and OK with me.
> > > >
> > > > @Pavan what do you think of adding it to service.h, implement in .c and add
> > > to .map?
> > > >
> > >
> > > The ROLE_SERVICE/ROLE_RTE defines the role of a lcore so it made sense to put
> > > it in rte_lcore.h as lcore properties are accessed mostly through this header.
> > > I'm fine with adding it to service.h as suggested by Harry.
> > >
> > > -Pavan
> >
> > *as suggested by Thomas ;)
> >
> > Initially I thought it made more sense in lcore.h too, however the application
> > should only require knowing if core X is a service core if it cares about
> > services / service-cores, hence I'm fine with rte_service.h too.
> >
> > -Harry
> >
> Agreed, will spin up a v2.

The most difficult is to find a good name for this function :)

Re: [PATCH] eal: added new `rte_lcore_is_service_lcore` API.

[Pavan Nikhilesh Bhagavatula]

On Fri, Sep 15, 2017 at 05:51:36PM +0200, Thomas Monjalon wrote:
> 15/09/2017 16:59, Pavan Nikhilesh Bhagavatula:
> > On Fri, Sep 15, 2017 at 02:44:57PM +0000, Van Haaren, Harry wrote:
>
> > > > > > We could also choose to add this function to rte_service.h ?
> > > > >
> > > > > Yes that is an option, and OK with me.
> > > > >
> > > > > @Pavan what do you think of adding it to service.h, implement in .c and add
> > > > to .map?
> > > > >
> > > >
> > > > The ROLE_SERVICE/ROLE_RTE defines the role of a lcore so it made sense to put
> > > > it in rte_lcore.h as lcore properties are accessed mostly through this header.
> > > > I'm fine with adding it to service.h as suggested by Harry.
> > > >
> > > > -Pavan
> > >
> > > *as suggested by Thomas ;)
> > >
> > > Initially I thought it made more sense in lcore.h too, however the application
> > > should only require knowing if core X is a service core if it cares about
> > > services / service-cores, hence I'm fine with rte_service.h too.
> > >
> > > -Harry
> > >
> > Agreed, will spin up a v2.
>
> The most difficult is to find a good name for this function :)

If not rte_lcore_is_service_core then how about rte_lcore_is_role_service?
But this would need a sibling api rte_lcore_is_role_rte (or a better one) which
is satisfied by rte_lcore_is_enabled :(
IMO when role was limited to RTE & OFF rte_lcore_is_enabled fits now with
new role SERVICE it looks out of place cause even service lcores are
"enabled".
Modifying rte_lcore_is_enabled would be a huge task (API change) as it is used
widely in many places.

-Pavan

Re: [PATCH] eal: added new `rte_lcore_is_service_lcore` API.

[Van Haaren, Harry]

> From: Pavan Nikhilesh Bhagavatula [mailto:pbhagavatula@caviumnetworks.com]
> Sent: Friday, September 15, 2017 6:38 PM
> To: Thomas Monjalon <thomas@monjalon.net>
> Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] eal: added new `rte_lcore_is_service_lcore`
> API.
> 
> On Fri, Sep 15, 2017 at 05:51:36PM +0200, Thomas Monjalon wrote:
> > 15/09/2017 16:59, Pavan Nikhilesh Bhagavatula:
> > > On Fri, Sep 15, 2017 at 02:44:57PM +0000, Van Haaren, Harry wrote:
> >
> > > > > > > We could also choose to add this function to rte_service.h ?
> > > > > >
> > > > > > Yes that is an option, and OK with me.
> > > > > >
> > > > > > @Pavan what do you think of adding it to service.h, implement in .c
> and add
> > > > > to .map?
> > > > > >
> > > > >
> > > > > The ROLE_SERVICE/ROLE_RTE defines the role of a lcore so it made sense
> to put
> > > > > it in rte_lcore.h as lcore properties are accessed mostly through this
> header.
> > > > > I'm fine with adding it to service.h as suggested by Harry.
> > > > >
> > > > > -Pavan
> > > >
> > > > *as suggested by Thomas ;)
> > > >
> > > > Initially I thought it made more sense in lcore.h too, however the
> application
> > > > should only require knowing if core X is a service core if it cares about
> > > > services / service-cores, hence I'm fine with rte_service.h too.
> > > >
> > > > -Harry
> > > >
> > > Agreed, will spin up a v2.
> >
> > The most difficult is to find a good name for this function :)
> 
> If not rte_lcore_is_service_core then how about rte_lcore_is_role_service?
> But this would need a sibling api rte_lcore_is_role_rte (or a better one) which
> is satisfied by rte_lcore_is_enabled :(
> IMO when role was limited to RTE & OFF rte_lcore_is_enabled fits now with
> new role SERVICE it looks out of place cause even service lcores are
> "enabled".
> Modifying rte_lcore_is_enabled would be a huge task (API change) as it is used
> widely in many places.

Hey all,

I've been thinking a little, and adding the "is service core" functionality in the
rte_service_* namespace might be the wrong place. The function name certainly doesn't
roll off the tongue ( rte_service_lcore_has_service_role() ?? )

What if we add a new function to rte_lcore.h? The implementation could be in a
new file, rte_lcore.c, to avoid "static inline" in a control-path function.

In my eyes, this approach is the cleanest as it allows re-use of the same function
for various types, including SERVICE, RTE, OFF etc. 


/** Probes if the calling core has a specific role.
 * @retval 1 If the core has role matching the *role* passed in
 * @retval 0 If the core's role does not match *role* passed in
 */
int
rte_lcore_has_role(enum rte_lcore_role_t role);


Application code becomes pretty self-documenting:
if (rte_lcore_has_role(ROLE_SERVICE)) {
    // do something
}

Thoughts? -Harry

Re: [PATCH] eal: added new `rte_lcore_is_service_lcore` API.

[Thomas Monjalon]

20/09/2017 16:53, Van Haaren, Harry:
> From: Pavan Nikhilesh Bhagavatula [mailto:pbhagavatula@caviumnetworks.com]
> > On Fri, Sep 15, 2017 at 05:51:36PM +0200, Thomas Monjalon wrote:
> > > 15/09/2017 16:59, Pavan Nikhilesh Bhagavatula:
> > > > On Fri, Sep 15, 2017 at 02:44:57PM +0000, Van Haaren, Harry wrote:
> > >
> > > > > > > > We could also choose to add this function to rte_service.h ?
> > > > > > >
> > > > > > > Yes that is an option, and OK with me.
> > > > > > >
> > > > > > > @Pavan what do you think of adding it to service.h, implement in .c
> > and add
> > > > > > to .map?
> > > > > > >
> > > > > >
> > > > > > The ROLE_SERVICE/ROLE_RTE defines the role of a lcore so it made sense
> > to put
> > > > > > it in rte_lcore.h as lcore properties are accessed mostly through this
> > header.
> > > > > > I'm fine with adding it to service.h as suggested by Harry.
> > > > > >
> > > > > > -Pavan
> > > > >
> > > > > *as suggested by Thomas ;)
> > > > >
> > > > > Initially I thought it made more sense in lcore.h too, however the
> > application
> > > > > should only require knowing if core X is a service core if it cares about
> > > > > services / service-cores, hence I'm fine with rte_service.h too.
> > > > >
> > > > > -Harry
> > > > >
> > > > Agreed, will spin up a v2.
> > >
> > > The most difficult is to find a good name for this function :)
> > 
> > If not rte_lcore_is_service_core then how about rte_lcore_is_role_service?
> > But this would need a sibling api rte_lcore_is_role_rte (or a better one) which
> > is satisfied by rte_lcore_is_enabled :(
> > IMO when role was limited to RTE & OFF rte_lcore_is_enabled fits now with
> > new role SERVICE it looks out of place cause even service lcores are
> > "enabled".
> > Modifying rte_lcore_is_enabled would be a huge task (API change) as it is used
> > widely in many places.
> 
> Hey all,
> 
> I've been thinking a little, and adding the "is service core" functionality in the
> rte_service_* namespace might be the wrong place. The function name certainly doesn't
> roll off the tongue ( rte_service_lcore_has_service_role() ?? )
> 
> What if we add a new function to rte_lcore.h? The implementation could be in a
> new file, rte_lcore.c, to avoid "static inline" in a control-path function.
> 
> In my eyes, this approach is the cleanest as it allows re-use of the same function
> for various types, including SERVICE, RTE, OFF etc. 
> 
> 
> /** Probes if the calling core has a specific role.
>  * @retval 1 If the core has role matching the *role* passed in
>  * @retval 0 If the core's role does not match *role* passed in
>  */
> int
> rte_lcore_has_role(enum rte_lcore_role_t role);
> 
> 
> Application code becomes pretty self-documenting:
> if (rte_lcore_has_role(ROLE_SERVICE)) {
>     // do something
> }
> 
> Thoughts? -Harry

OK, no problem

Re: [PATCH] eal: added new `rte_lcore_is_service_lcore` API.

[Pavan Nikhilesh Bhagavatula]

On Wed, Sep 20, 2017 at 05:53:04PM +0200, Thomas Monjalon wrote:
> 20/09/2017 16:53, Van Haaren, Harry:
> > From: Pavan Nikhilesh Bhagavatula [mailto:pbhagavatula@caviumnetworks.com]
> > > On Fri, Sep 15, 2017 at 05:51:36PM +0200, Thomas Monjalon wrote:
> > > > 15/09/2017 16:59, Pavan Nikhilesh Bhagavatula:
> > > > > On Fri, Sep 15, 2017 at 02:44:57PM +0000, Van Haaren, Harry wrote:
> > > >
> > > > > > > > > We could also choose to add this function to rte_service.h ?
> > > > > > > >
> > > > > > > > Yes that is an option, and OK with me.
> > > > > > > >
> > > > > > > > @Pavan what do you think of adding it to service.h, implement in .c
> > > and add
> > > > > > > to .map?
> > > > > > > >
> > > > > > >
> > > > > > > The ROLE_SERVICE/ROLE_RTE defines the role of a lcore so it made sense
> > > to put
> > > > > > > it in rte_lcore.h as lcore properties are accessed mostly through this
> > > header.
> > > > > > > I'm fine with adding it to service.h as suggested by Harry.
> > > > > > >
> > > > > > > -Pavan
> > > > > >
> > > > > > *as suggested by Thomas ;)
> > > > > >
> > > > > > Initially I thought it made more sense in lcore.h too, however the
> > > application
> > > > > > should only require knowing if core X is a service core if it cares about
> > > > > > services / service-cores, hence I'm fine with rte_service.h too.
> > > > > >
> > > > > > -Harry
> > > > > >
> > > > > Agreed, will spin up a v2.
> > > >
> > > > The most difficult is to find a good name for this function :)
> > >
> > > If not rte_lcore_is_service_core then how about rte_lcore_is_role_service?
> > > But this would need a sibling api rte_lcore_is_role_rte (or a better one) which
> > > is satisfied by rte_lcore_is_enabled :(
> > > IMO when role was limited to RTE & OFF rte_lcore_is_enabled fits now with
> > > new role SERVICE it looks out of place cause even service lcores are
> > > "enabled".
> > > Modifying rte_lcore_is_enabled would be a huge task (API change) as it is used
> > > widely in many places.
> >
> > Hey all,
> >
> > I've been thinking a little, and adding the "is service core" functionality in the
> > rte_service_* namespace might be the wrong place. The function name certainly doesn't
> > roll off the tongue ( rte_service_lcore_has_service_role() ?? )
> >
> > What if we add a new function to rte_lcore.h? The implementation could be in a
> > new file, rte_lcore.c, to avoid "static inline" in a control-path function.
> >
> > In my eyes, this approach is the cleanest as it allows re-use of the same function
> > for various types, including SERVICE, RTE, OFF etc.
> >
> >
> > /** Probes if the calling core has a specific role.
> >  * @retval 1 If the core has role matching the *role* passed in
> >  * @retval 0 If the core's role does not match *role* passed in
> >  */
> > int
> > rte_lcore_has_role(enum rte_lcore_role_t role);
> >
> >
> > Application code becomes pretty self-documenting:
> > if (rte_lcore_has_role(ROLE_SERVICE)) {
> >     // do something
> > }
> >
> > Thoughts? -Harry
>
> OK, no problem
>
Thanks for all the inputs will spin up a v2.
-Pavan

[PATCH v2] eal: add function to check lcore role

[Pavan Nikhilesh]

From: Pavan Bhagavatula <pbhagavatula@caviumnetworks.com>

This function can be used to check the role of a specific lcore.

Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
---

v2 changs:
 - removed ack due to significant changes in patch
 - addressed review comments
 - modified commit title and message

 lib/librte_eal/bsdapp/eal/Makefile              |  1 +
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  6 +++
 lib/librte_eal/common/include/rte_lcore.h       | 14 +++++++
 lib/librte_eal/common/rte_lcore.c               | 49 +++++++++++++++++++++++++
 lib/librte_eal/linuxapp/eal/Makefile            |  1 +
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  6 +++
 6 files changed, 77 insertions(+)
 create mode 100644 lib/librte_eal/common/rte_lcore.c

diff --git a/lib/librte_eal/bsdapp/eal/Makefile b/lib/librte_eal/bsdapp/eal/Makefile
index 005019e..f1263fc 100644
--- a/lib/librte_eal/bsdapp/eal/Makefile
+++ b/lib/librte_eal/bsdapp/eal/Makefile
@@ -88,6 +88,7 @@ SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += malloc_elem.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += malloc_heap.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += rte_keepalive.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += rte_service.c
+SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += rte_lcore.c

 # from arch dir
 SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += rte_cpuflags.c
diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
index aac6fd7..3be3287 100644
--- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
@@ -237,3 +237,9 @@ EXPERIMENTAL {
 	rte_service_unregister;

 } DPDK_17.08;
+
+DPDK_17.11 {
+	global:
+
+	rte_lcore_has_role;
+} DPDK_17.08;
diff --git a/lib/librte_eal/common/include/rte_lcore.h b/lib/librte_eal/common/include/rte_lcore.h
index 50e0d0f..777208d 100644
--- a/lib/librte_eal/common/include/rte_lcore.h
+++ b/lib/librte_eal/common/include/rte_lcore.h
@@ -262,6 +262,20 @@ void rte_thread_get_affinity(rte_cpuset_t *cpusetp);
  */
 int rte_thread_setname(pthread_t id, const char *name);

+/**
+ * Test if the core supplied has a specific role
+ *
+ * @param lcore_id
+ *   The identifier of the lcore, which MUST be between 0 and
+ *   RTE_MAX_LCORE-1.
+ * @param role
+ *   The role to be checked against.
+ * @return
+ *   On success, return 0; otherwise return a negative value.
+ */
+int
+rte_lcore_has_role(unsigned lcore_id, enum rte_lcore_role_t role);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_eal/common/rte_lcore.c b/lib/librte_eal/common/rte_lcore.c
new file mode 100644
index 0000000..66c03f4
--- /dev/null
+++ b/lib/librte_eal/common/rte_lcore.c
@@ -0,0 +1,49 @@
+/*
+ *   BSD LICENSE
+ *
+ *   Copyright (C) Cavium, Inc. 2017.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Cavium, Inc nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include <rte_common.h>
+#include <rte_lcore.h>
+
+
+int
+rte_lcore_has_role(unsigned lcore_id, enum rte_lcore_role_t role)
+{
+	struct rte_config *cfg = rte_eal_get_configuration();
+
+	if (lcore_id >= RTE_MAX_LCORE)
+		return -EINVAL;
+
+	if (cfg->lcore_role[lcore_id] == role)
+		return 0;
+
+	return -EINVAL;
+}
diff --git a/lib/librte_eal/linuxapp/eal/Makefile b/lib/librte_eal/linuxapp/eal/Makefile
index 90bca4d..9596acb 100644
--- a/lib/librte_eal/linuxapp/eal/Makefile
+++ b/lib/librte_eal/linuxapp/eal/Makefile
@@ -100,6 +100,7 @@ SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += malloc_elem.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += malloc_heap.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += rte_keepalive.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += rte_service.c
+SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += rte_lcore.c

 # from arch dir
 SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += rte_cpuflags.c
diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
index 3a8f154..7304a34 100644
--- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
@@ -242,3 +242,9 @@ EXPERIMENTAL {
 	rte_service_unregister;

 } DPDK_17.08;
+
+DPDK_17.11 {
+	global:
+
+	rte_lcore_has_role;
+} DPDK_17.08;
--
2.7.4

Re: [PATCH v2] eal: add function to check lcore role

[Van Haaren, Harry]

> From: Pavan Nikhilesh [mailto:pbhagavatula@caviumnetworks.com]
> Sent: Thursday, September 21, 2017 9:58 AM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>; thomas@monjalon.net
> Cc: dev@dpdk.org; Pavan Bhagavatula <pbhagavatula@caviumnetworks.com>
> Subject: [dpdk-dev] [PATCH v2] eal: add function to check lcore role
> 
> From: Pavan Bhagavatula <pbhagavatula@caviumnetworks.com>
> 
> This function can be used to check the role of a specific lcore.
> 
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
> ---
> 
> v2 changs:
>  - removed ack due to significant changes in patch
>  - addressed review comments
>  - modified commit title and message

Good work - a few final inline comments below;

- whitespace in bsd/linux .map files
- "unsigned" -> "unsigned int" (checkpatch)
- Move function to eal_common_thread.c

With those,

Acked-by: Harry van Haaren <harry.van.haaren@intel.com>


>  lib/librte_eal/bsdapp/eal/Makefile              |  1 +
>  lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  6 +++
>  lib/librte_eal/common/include/rte_lcore.h       | 14 +++++++
>  lib/librte_eal/common/rte_lcore.c               | 49 +++++++++++++++++++++++++
>  lib/librte_eal/linuxapp/eal/Makefile            |  1 +
>  lib/librte_eal/linuxapp/eal/rte_eal_version.map |  6 +++
>  6 files changed, 77 insertions(+)
>  create mode 100644 lib/librte_eal/common/rte_lcore.c
> 
> diff --git a/lib/librte_eal/bsdapp/eal/Makefile
> b/lib/librte_eal/bsdapp/eal/Makefile
> index 005019e..f1263fc 100644
> --- a/lib/librte_eal/bsdapp/eal/Makefile
> +++ b/lib/librte_eal/bsdapp/eal/Makefile
> @@ -88,6 +88,7 @@ SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += malloc_elem.c
>  SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += malloc_heap.c
>  SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += rte_keepalive.c
>  SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += rte_service.c
> +SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += rte_lcore.c
> 
>  # from arch dir
>  SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += rte_cpuflags.c
> diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> index aac6fd7..3be3287 100644
> --- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> +++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> @@ -237,3 +237,9 @@ EXPERIMENTAL {
>  	rte_service_unregister;
> 
>  } DPDK_17.08;
> +
> +DPDK_17.11 {
> +	global:
> +
> +	rte_lcore_has_role;
> +} DPDK_17.08;


I think there's usually a newline between the function and closing brace - nit pick.


> diff --git a/lib/librte_eal/common/include/rte_lcore.h
> b/lib/librte_eal/common/include/rte_lcore.h
> index 50e0d0f..777208d 100644
> --- a/lib/librte_eal/common/include/rte_lcore.h
> +++ b/lib/librte_eal/common/include/rte_lcore.h
> @@ -262,6 +262,20 @@ void rte_thread_get_affinity(rte_cpuset_t *cpusetp);
>   */
>  int rte_thread_setname(pthread_t id, const char *name);
> 
> +/**
> + * Test if the core supplied has a specific role
> + *
> + * @param lcore_id
> + *   The identifier of the lcore, which MUST be between 0 and
> + *   RTE_MAX_LCORE-1.
> + * @param role
> + *   The role to be checked against.
> + * @return
> + *   On success, return 0; otherwise return a negative value.
> + */
> +int
> +rte_lcore_has_role(unsigned lcore_id, enum rte_lcore_role_t role);
> +

Other functions use "unsigned" in this header, however checkpatch complains about it. I think we should use "unsigned int" instead, to make checkpatch happy and use more modern C practice (at the cost of minor deviation from the existing funcs).


>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/lib/librte_eal/common/rte_lcore.c
> b/lib/librte_eal/common/rte_lcore.c
> new file mode 100644
> index 0000000..66c03f4
> --- /dev/null
> +++ b/lib/librte_eal/common/rte_lcore.c
> @@ -0,0 +1,49 @@
> +/*
> + *   BSD LICENSE
> + *
> + *   Copyright (C) Cavium, Inc. 2017.
> + *
> + *   Redistribution and use in source and binary forms, with or without
> + *   modification, are permitted provided that the following conditions
> + *   are met:
> + *
> + *     * Redistributions of source code must retain the above copyright
> + *       notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above copyright
> + *       notice, this list of conditions and the following disclaimer in
> + *       the documentation and/or other materials provided with the
> + *       distribution.
> + *     * Neither the name of Cavium, Inc nor the names of its
> + *       contributors may be used to endorse or promote products derived
> + *       from this software without specific prior written permission.
> + *
> + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include <rte_common.h>
> +#include <rte_lcore.h>
> +
> +
> +int
> +rte_lcore_has_role(unsigned lcore_id, enum rte_lcore_role_t role)
> +{
> +	struct rte_config *cfg = rte_eal_get_configuration();
> +
> +	if (lcore_id >= RTE_MAX_LCORE)
> +		return -EINVAL;
> +
> +	if (cfg->lcore_role[lcore_id] == role)
> +		return 0;
> +
> +	return -EINVAL;
> +}

While reviewing, I found that the rte_socket_id() function is declared in rte_lcore.h, but implemented in eal_common_thread.c
This function should probably move there too - common thread sounds like a good home to this function. Apologies for not noticing this earlier, and suggesting rte_lcore.c


> diff --git a/lib/librte_eal/linuxapp/eal/Makefile
> b/lib/librte_eal/linuxapp/eal/Makefile
> index 90bca4d..9596acb 100644
> --- a/lib/librte_eal/linuxapp/eal/Makefile
> +++ b/lib/librte_eal/linuxapp/eal/Makefile
> @@ -100,6 +100,7 @@ SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += malloc_elem.c
>  SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += malloc_heap.c
>  SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += rte_keepalive.c
>  SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += rte_service.c
> +SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += rte_lcore.c
> 
>  # from arch dir
>  SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += rte_cpuflags.c
> diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> index 3a8f154..7304a34 100644
> --- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> +++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> @@ -242,3 +242,9 @@ EXPERIMENTAL {
>  	rte_service_unregister;
> 
>  } DPDK_17.08;
> +
> +DPDK_17.11 {
> +	global:
> +
> +	rte_lcore_has_role;
> +} DPDK_17.08;
> --
> 2.7.4

Re: [PATCH v2] eal: add function to check lcore role

[Pavan Nikhilesh Bhagavatula]

On Thu, Sep 21, 2017 at 09:41:29AM +0000, Van Haaren, Harry wrote:
> > From: Pavan Nikhilesh [mailto:pbhagavatula@caviumnetworks.com]
> > Sent: Thursday, September 21, 2017 9:58 AM
> > To: Van Haaren, Harry <harry.van.haaren@intel.com>; thomas@monjalon.net
> > Cc: dev@dpdk.org; Pavan Bhagavatula <pbhagavatula@caviumnetworks.com>
> > Subject: [dpdk-dev] [PATCH v2] eal: add function to check lcore role
> >
> > From: Pavan Bhagavatula <pbhagavatula@caviumnetworks.com>
> >
> > This function can be used to check the role of a specific lcore.
> >
> > Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
> > ---
> >
> > v2 changs:
> >  - removed ack due to significant changes in patch
> >  - addressed review comments
> >  - modified commit title and message
>
> Good work - a few final inline comments below;
Thanks Harry
>
> - whitespace in bsd/linux .map files
> - "unsigned" -> "unsigned int" (checkpatch)
> - Move function to eal_common_thread.c
>
For some reason ./devtools/checkpatches.sh reports no error.
Will move the function to eal_common_thread.c and send a v3.
> With those,
>
> Acked-by: Harry van Haaren <harry.van.haaren@intel.com>
>
>
> >  lib/librte_eal/bsdapp/eal/Makefile              |  1 +
> >  lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  6 +++
> >  lib/librte_eal/common/include/rte_lcore.h       | 14 +++++++
> >  lib/librte_eal/common/rte_lcore.c               | 49 +++++++++++++++++++++++++
> >  lib/librte_eal/linuxapp/eal/Makefile            |  1 +
> >  lib/librte_eal/linuxapp/eal/rte_eal_version.map |  6 +++
> >  6 files changed, 77 insertions(+)
> >  create mode 100644 lib/librte_eal/common/rte_lcore.c
> >
> > diff --git a/lib/librte_eal/bsdapp/eal/Makefile
> > b/lib/librte_eal/bsdapp/eal/Makefile
> > index 005019e..f1263fc 100644
> > --- a/lib/librte_eal/bsdapp/eal/Makefile
> > +++ b/lib/librte_eal/bsdapp/eal/Makefile
> > @@ -88,6 +88,7 @@ SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += malloc_elem.c
> >  SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += malloc_heap.c
> >  SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += rte_keepalive.c
> >  SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += rte_service.c
> > +SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += rte_lcore.c
> >
> >  # from arch dir
> >  SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += rte_cpuflags.c
> > diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> > b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> > index aac6fd7..3be3287 100644
> > --- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> > +++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> > @@ -237,3 +237,9 @@ EXPERIMENTAL {
> >  	rte_service_unregister;
> >
> >  } DPDK_17.08;
> > +
> > +DPDK_17.11 {
> > +	global:
> > +
> > +	rte_lcore_has_role;
> > +} DPDK_17.08;
>
>
> I think there's usually a newline between the function and closing brace - nit pick.
>
>
> > diff --git a/lib/librte_eal/common/include/rte_lcore.h
> > b/lib/librte_eal/common/include/rte_lcore.h
> > index 50e0d0f..777208d 100644
> > --- a/lib/librte_eal/common/include/rte_lcore.h
> > +++ b/lib/librte_eal/common/include/rte_lcore.h
> > @@ -262,6 +262,20 @@ void rte_thread_get_affinity(rte_cpuset_t *cpusetp);
> >   */
> >  int rte_thread_setname(pthread_t id, const char *name);
> >
> > +/**
> > + * Test if the core supplied has a specific role
> > + *
> > + * @param lcore_id
> > + *   The identifier of the lcore, which MUST be between 0 and
> > + *   RTE_MAX_LCORE-1.
> > + * @param role
> > + *   The role to be checked against.
> > + * @return
> > + *   On success, return 0; otherwise return a negative value.
> > + */
> > +int
> > +rte_lcore_has_role(unsigned lcore_id, enum rte_lcore_role_t role);
> > +
>
> Other functions use "unsigned" in this header, however checkpatch complains about it. I think we should use "unsigned int" instead, to make checkpatch happy and use more modern C practice (at the cost of minor deviation from the existing funcs).
>
>
> >  #ifdef __cplusplus
> >  }
> >  #endif
> > diff --git a/lib/librte_eal/common/rte_lcore.c
> > b/lib/librte_eal/common/rte_lcore.c
> > new file mode 100644
> > index 0000000..66c03f4
> > --- /dev/null
> > +++ b/lib/librte_eal/common/rte_lcore.c
> > @@ -0,0 +1,49 @@
> > +/*
> > + *   BSD LICENSE
> > + *
> > + *   Copyright (C) Cavium, Inc. 2017.
> > + *
> > + *   Redistribution and use in source and binary forms, with or without
> > + *   modification, are permitted provided that the following conditions
> > + *   are met:
> > + *
> > + *     * Redistributions of source code must retain the above copyright
> > + *       notice, this list of conditions and the following disclaimer.
> > + *     * Redistributions in binary form must reproduce the above copyright
> > + *       notice, this list of conditions and the following disclaimer in
> > + *       the documentation and/or other materials provided with the
> > + *       distribution.
> > + *     * Neither the name of Cavium, Inc nor the names of its
> > + *       contributors may be used to endorse or promote products derived
> > + *       from this software without specific prior written permission.
> > + *
> > + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> > + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> > + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> > + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> > + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> > + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> > + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> > + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> > + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> > + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> > + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> > + */
> > +
> > +#include <rte_common.h>
> > +#include <rte_lcore.h>
> > +
> > +
> > +int
> > +rte_lcore_has_role(unsigned lcore_id, enum rte_lcore_role_t role)
> > +{
> > +	struct rte_config *cfg = rte_eal_get_configuration();
> > +
> > +	if (lcore_id >= RTE_MAX_LCORE)
> > +		return -EINVAL;
> > +
> > +	if (cfg->lcore_role[lcore_id] == role)
> > +		return 0;
> > +
> > +	return -EINVAL;
> > +}
>
> While reviewing, I found that the rte_socket_id() function is declared in rte_lcore.h, but implemented in eal_common_thread.c
> This function should probably move there too - common thread sounds like a good home to this function. Apologies for not noticing this earlier, and suggesting rte_lcore.c
>
>
> > diff --git a/lib/librte_eal/linuxapp/eal/Makefile
> > b/lib/librte_eal/linuxapp/eal/Makefile
> > index 90bca4d..9596acb 100644
> > --- a/lib/librte_eal/linuxapp/eal/Makefile
> > +++ b/lib/librte_eal/linuxapp/eal/Makefile
> > @@ -100,6 +100,7 @@ SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += malloc_elem.c
> >  SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += malloc_heap.c
> >  SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += rte_keepalive.c
> >  SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += rte_service.c
> > +SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += rte_lcore.c
> >
> >  # from arch dir
> >  SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += rte_cpuflags.c
> > diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> > b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> > index 3a8f154..7304a34 100644
> > --- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> > +++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> > @@ -242,3 +242,9 @@ EXPERIMENTAL {
> >  	rte_service_unregister;
> >
> >  } DPDK_17.08;
> > +
> > +DPDK_17.11 {
> > +	global:
> > +
> > +	rte_lcore_has_role;
> > +} DPDK_17.08;
> > --
> > 2.7.4
>

[PATCH v3] eal: add function to check lcore role

[Pavan Nikhilesh]

From: Pavan Bhagavatula <pbhagavatula@caviumnetworks.com>

This function can be used to check the role of a specific lcore.

Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
Acked-by: Harry van Haaren <harry.van.haaren@intel.com>
---

v3 changes:
 - moved changes to eal_common_thread.c
 - addressed coding style issues

v2 changes:
 - removed ack due to significant changes in patch
 - addressed review comments
 - modified commit title and message

 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  7 +++++++
 lib/librte_eal/common/eal_common_thread.c       | 14 ++++++++++++++
 lib/librte_eal/common/include/rte_lcore.h       | 14 ++++++++++++++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  7 +++++++
 4 files changed, 42 insertions(+)

diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
index aac6fd7..19f75d9 100644
--- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
@@ -237,3 +237,10 @@ EXPERIMENTAL {
 	rte_service_unregister;

 } DPDK_17.08;
+
+DPDK_17.11 {
+	global:
+
+	rte_lcore_has_role;
+
+} DPDK_17.08;
diff --git a/lib/librte_eal/common/eal_common_thread.c b/lib/librte_eal/common/eal_common_thread.c
index 2405e93..55e9696 100644
--- a/lib/librte_eal/common/eal_common_thread.c
+++ b/lib/librte_eal/common/eal_common_thread.c
@@ -53,6 +53,20 @@ unsigned rte_socket_id(void)
 	return RTE_PER_LCORE(_socket_id);
 }

+int
+rte_lcore_has_role(unsigned int lcore_id, enum rte_lcore_role_t role)
+{
+	struct rte_config *cfg = rte_eal_get_configuration();
+
+	if (lcore_id >= RTE_MAX_LCORE)
+		return -EINVAL;
+
+	if (cfg->lcore_role[lcore_id] == role)
+		return 0;
+
+	return -EINVAL;
+}
+
 int eal_cpuset_socket_id(rte_cpuset_t *cpusetp)
 {
 	unsigned cpu = 0;
diff --git a/lib/librte_eal/common/include/rte_lcore.h b/lib/librte_eal/common/include/rte_lcore.h
index 50e0d0f..c89e6ba 100644
--- a/lib/librte_eal/common/include/rte_lcore.h
+++ b/lib/librte_eal/common/include/rte_lcore.h
@@ -262,6 +262,20 @@ void rte_thread_get_affinity(rte_cpuset_t *cpusetp);
  */
 int rte_thread_setname(pthread_t id, const char *name);

+/**
+ * Test if the core supplied has a specific role
+ *
+ * @param lcore_id
+ *   The identifier of the lcore, which MUST be between 0 and
+ *   RTE_MAX_LCORE-1.
+ * @param role
+ *   The role to be checked against.
+ * @return
+ *   On success, return 0; otherwise return a negative value.
+ */
+int
+rte_lcore_has_role(unsigned int lcore_id, enum rte_lcore_role_t role);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
index 3a8f154..60c2fbc 100644
--- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
@@ -242,3 +242,10 @@ EXPERIMENTAL {
 	rte_service_unregister;

 } DPDK_17.08;
+
+DPDK_17.11 {
+	global:
+
+	rte_lcore_has_role;
+
+} DPDK_17.08;
--
2.7.4

Re: [PATCH v3] eal: add function to check lcore role

[Thomas Monjalon]

21/09/2017 12:59, Pavan Nikhilesh:
> From: Pavan Bhagavatula <pbhagavatula@caviumnetworks.com>
> 
> This function can be used to check the role of a specific lcore.
> 
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
> Acked-by: Harry van Haaren <harry.van.haaren@intel.com>

Applied, thanks