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

From: Pavan Nikhilesh Bhagavatula [mailto:pbhagavatula@caviumnetworks.com]
Sent: Monday, August 28, 2017 4:43 PM
To: Van Haaren, Harry <redacted>
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:

quoted

quoted

-----Original Message-----
From: Pavan Nikhilesh Bhagavatula [mailto:pbhagavatula@caviumnetworks.com]
Sent: Monday, August 28, 2017 4:10 PM
To: Van Haaren, Harry <redacted>
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:

quoted

quoted

From: Pavan Nikhilesh Bhagavatula [mailto:pbhagavatula@caviumnetworks.com]
Sent: Monday, August 28, 2017 12:33 PM
To: Van Haaren, Harry <redacted>
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:

quoted

quoted

From: Pavan Nikhilesh [mailto:pbhagavatula@caviumnetworks.com]
Sent: Wednesday, August 23, 2017 4:10 PM
To: dev@dpdk.org
Cc: Van Haaren, Harry <redacted>; Pavan Nikhilesh
[off-list ref]
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 <redacted>
---
 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

quoted

quoted

quoted

quoted

cores library?

quoted

If so, the function should actually be used, instead of only added but not used in

the

quoted

quoted

quoted

quoted

library itself.

quoted

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.

quoted

quoted

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.

quoted

quoted

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

quoted

quoted

to retrieve that information. Having "internal" functions between libraries is not nice.

quoted

I think a better design would be to add this function as a public function, (add it to

the

quoted

quoted

.map files etc) and then call the public function from the timers library.

quoted

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

quoted

One technical issue:

quoted

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