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

-----Original Message-----
From: Pavan Nikhilesh Bhagavatula [mailto:pbhagavatula@caviumnetworks.com]
Sent: Friday, September 15, 2017 3:42 PM
To: Van Haaren, Harry <redacted>
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:

quoted

quoted

From: Thomas Monjalon [mailto:thomas@monjalon.net]
Sent: Friday, September 15, 2017 2:53 PM
To: Pavan Nikhilesh Bhagavatula <redacted>; Van

Haaren,

quoted

quoted

Harry [off-list ref]
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:

quoted

On Mon, Aug 28, 2017 at 01:49:37PM +0000, Van Haaren, Harry wrote:

quoted

From: Pavan Nikhilesh Bhagavatula

[mailto:pbhagavatula@caviumnetworks.com]

quoted

quoted

quoted

quoted

quoted

On Mon, Aug 28, 2017 at 10:59:51AM +0000, Van Haaren, Harry wrote:

quoted

From: Pavan Nikhilesh [mailto:pbhagavatula@caviumnetworks.com]

quoted

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

cores library?

quoted

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

but

quoted

quoted

not used in the

quoted

quoted

quoted

library itself.

quoted

The enum rte_lcore_role_t has ROLE_SERVICE which tells that a

particular

quoted

quoted

lcore is

quoted

quoted

quoted

a service lcore as well as an EAL thread some libraries such as

rte_timer

quoted

quoted

allow

quoted

quoted

quoted

specific operations only over EAL threads.

Understood that role of cores is important, and that rte_timer might

require this information.

quoted

quoted

quoted

The rte_timer lib uses the rte_is_lcore_enabled() call to check if a

lcore is an

quoted

quoted

quoted

EAL thread, Which checks if the lcore role is  ROLE_RTE. But it

should

quoted

quoted

also

quoted

quoted

quoted

allow timers to be registered on a service core as processing those

timers can

quoted

quoted

quoted

be done on them.

No problem from me here either - although it's the Timers library

maintainer that should check this.

quoted

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

quoted

quoted

use a public API to retrieve that information. Having "internal" functions
between libraries is not nice.

quoted

quoted

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

function,

quoted

quoted

(add it to the .map files etc) and then call the public function from the
timers library.

quoted

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

quoted

quoted

quoted

file for eal/common and also other functions that are present in

rte_lcore.h

quoted

quoted

quoted

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?

quoted

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