[dpdk-dev] [PATCH] Add an API to query enabled core index

[dpdk-dev] [PATCH] Add an API to query enabled core index

[Patrick Lu]

EAL -c option allows the user to enable any lcore in the system.
Oftentimes, the user app wants to know 1st enabled core, 2nd
enabled core, etc, rather than phyical core ID (rte_lcore_id().)

The new API rte_lcore_id2() will return an index from enabled lcores
starting from zero.
---
 lib/librte_eal/common/include/rte_lcore.h                | 12 ++++++++++++
 lib/librte_eal/linuxapp/eal/eal.c                        |  1 +
 lib/librte_eal/linuxapp/eal/include/exec-env/rte_lcore.h |  1 +
 3 files changed, 14 insertions(+)

diff --git a/lib/librte_eal/common/include/rte_lcore.h b/lib/librte_eal/common/include/rte_lcore.h
index 3802a28..f0682ce 100644
--- a/lib/librte_eal/common/include/rte_lcore.h
+++ b/lib/librte_eal/common/include/rte_lcore.h
@@ -92,6 +92,18 @@ rte_lcore_count(void)
 #include <exec-env/rte_lcore.h>
 
 /**
+ * Return the index of the enabled lcore starting from zero.
+ *
+ * @return
+ *   the ID of current lcoreid's index
+ */
+static inline unsigned
+rte_lcore_id2(void)
+{
+       return lcore_config[rte_lcore_id()].core_id2;
+}
+
+/**
  * Return the ID of the physical socket of the logical core we are
  * running on.
  * @return
diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index 070bdc9..a9c9e6c 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -444,6 +444,7 @@ eal_parse_coremask(const char *coremask)
 					return -1;
 				}
 				cfg->lcore_role[idx] = ROLE_RTE;
+				lcore_config[idx].core_id2 = count;
 				if(count == 0)
 					cfg->master_lcore = idx;
 				count++;
diff --git a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_lcore.h b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_lcore.h
index e19ab54..9316b05 100644
--- a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_lcore.h
+++ b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_lcore.h
@@ -57,6 +57,7 @@ struct lcore_config {
 	volatile enum rte_lcore_state_t state; /**< lcore state */
 	unsigned socket_id;        /**< physical socket id for this lcore */
 	unsigned core_id;          /**< core number on socket for this lcore */
+	unsigned core_id2;         /**< DPDK core index, starting from 0 */
 };
 
 /**
-- 
2.0.0

Re: [dpdk-dev] [PATCH] Add an API to query enabled core index

[Thomas Monjalon]

Hi,

2014-06-11 13:45, Patrick Lu:
> EAL -c option allows the user to enable any lcore in the system.
> Oftentimes, the user app wants to know 1st enabled core, 2nd
> enabled core, etc, rather than phyical core ID (rte_lcore_id().)
> 
> The new API rte_lcore_id2() will return an index from enabled lcores
> starting from zero.

I think core_id2 is not a representative name.
What do you think of renaming core_id as lcore_hwid and core_id2 as lcore_index?

-- 
Thomas

Re: [dpdk-dev] [PATCH] Add an API to query enabled core index

[Richardson, Bruce]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Wednesday, June 11, 2014 2:51 PM
> To: Lu, Patrick
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] Add an API to query enabled core index
> 
> Hi,
> 
> 2014-06-11 13:45, Patrick Lu:
> > EAL -c option allows the user to enable any lcore in the system.
> > Oftentimes, the user app wants to know 1st enabled core, 2nd
> > enabled core, etc, rather than phyical core ID (rte_lcore_id().)
> >
> > The new API rte_lcore_id2() will return an index from enabled lcores
> > starting from zero.
> 
> I think core_id2 is not a representative name.
> What do you think of renaming core_id as lcore_hwid and core_id2 as
> lcore_index?
> 
> --
I like lcore_index as the name for the new function. However, I'm not sure in that case that we want/need to rename the old one.

Re: [dpdk-dev] [PATCH] Add an API to query enabled core index

[Thomas Monjalon]

2014-06-11 21:57, Richardson, Bruce:
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> > 2014-06-11 13:45, Patrick Lu:
> > > The new API rte_lcore_id2() will return an index from enabled lcores
> > > starting from zero.
> > 
> > I think core_id2 is not a representative name.
> > What do you think of renaming core_id as lcore_hwid and core_id2 as
> > lcore_index?
> 
> I like lcore_index as the name for the new function. However, I'm not sure
> in that case that we want/need to rename the old one.

I think it would be not easy to distinguish id and index. So I prefer 
hwid/index. And lcore is more precise than core.

-- 
Thomas

Re: [dpdk-dev] [PATCH] Add an API to query enabled core index

[Richardson, Bruce]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Wednesday, June 11, 2014 3:50 PM
> To: Richardson, Bruce
> Cc: Lu, Patrick; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] Add an API to query enabled core index
> 
> 2014-06-11 21:57, Richardson, Bruce:
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> > > 2014-06-11 13:45, Patrick Lu:
> > > > The new API rte_lcore_id2() will return an index from enabled lcores
> > > > starting from zero.
> > >
> > > I think core_id2 is not a representative name.
> > > What do you think of renaming core_id as lcore_hwid and core_id2 as
> > > lcore_index?
> >
> > I like lcore_index as the name for the new function. However, I'm not sure
> > in that case that we want/need to rename the old one.
> 
> I think it would be not easy to distinguish id and index. So I prefer
> hwid/index. And lcore is more precise than core.
> 

The function is already called "rte_lcore_id()" so there is no need to change it to make it an "lcore" function. That function has been around for a long time and is commonly used, so I'd prefer it not be changed unless it really is necessary. "rte_lcore_index" is a sufficiently different function name, in my opinion. The API documentation should clear up any confusion for the user anyway.

Re: [dpdk-dev] [PATCH] Add an API to query enabled core index

[Dumitrescu, Cristian]

Maybe we could simplify this discussion by simply creating a new function to return the mask of all enabled cores (as provided through -c coremask EAL option) and have the user utilize this mask to derive whatever info it needs?

Right now, to get the mask of enabled cores, a for loop is required to test each core index one by one and re-create the mask.

In several instances, I needed to know just the number of enabled cores (i.e. number of bits set in -c coremask), and there was no alternative to the for loop above. But given such a function, we can quickly do:
	uint64_t coremask = rte_eal_coremask();
	n_lcores = __builtin_popcountll(coremask);

For what Patrick needs: 
	uint32_t lcore_enabled_pos = __builtin_popcountll(coremask & RTE_LEN2MASK(lcore_index));

Regards,
Cristian

-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Richardson, Bruce
Sent: Thursday, June 12, 2014 12:28 AM
To: Thomas Monjalon
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] Add an API to query enabled core index

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Wednesday, June 11, 2014 3:50 PM
> To: Richardson, Bruce
> Cc: Lu, Patrick; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] Add an API to query enabled core index
> 
> 2014-06-11 21:57, Richardson, Bruce:
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> > > 2014-06-11 13:45, Patrick Lu:
> > > > The new API rte_lcore_id2() will return an index from enabled lcores
> > > > starting from zero.
> > >
> > > I think core_id2 is not a representative name.
> > > What do you think of renaming core_id as lcore_hwid and core_id2 as
> > > lcore_index?
> >
> > I like lcore_index as the name for the new function. However, I'm not sure
> > in that case that we want/need to rename the old one.
> 
> I think it would be not easy to distinguish id and index. So I prefer
> hwid/index. And lcore is more precise than core.
> 

The function is already called "rte_lcore_id()" so there is no need to change it to make it an "lcore" function. That function has been around for a long time and is commonly used, so I'd prefer it not be changed unless it really is necessary. "rte_lcore_index" is a sufficiently different function name, in my opinion. The API documentation should clear up any confusion for the user anyway.
--------------------------------------------------------------
Intel Shannon Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263
Business address: Dromore House, East Park, Shannon, Co. Clare

This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.

Re: [dpdk-dev] [PATCH] Add an API to query enabled core index

[Olivier MATZ]

Hello,

On 06/11/2014 11:57 PM, Richardson, Bruce wrote:
>> I think core_id2 is not a representative name.
>> What do you think of renaming core_id as lcore_hwid and core_id2 as
>> lcore_index?
>>
>> --
> I like lcore_index as the name for the new function. However, I'm not sure in that case that we want/need to rename the old one.

What about lcore_rank ?
It may avoid confusion between "id" and "index", which are quite
close visually and phonetically.

I agree that we should not change the old lcore_id, its name is already
appropriate.

Regards,
Olivier

Re: [dpdk-dev] [PATCH] Add an API to query enabled core index

[Richardson, Bruce]

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Thursday, June 12, 2014 1:20 AM
> To: Richardson, Bruce; Thomas Monjalon; Lu, Patrick
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] Add an API to query enabled core index
> 
> Hello,
> 
> On 06/11/2014 11:57 PM, Richardson, Bruce wrote:
> >> I think core_id2 is not a representative name.
> >> What do you think of renaming core_id as lcore_hwid and core_id2 as
> >> lcore_index?
> >>
> >> --
> > I like lcore_index as the name for the new function. However, I'm not sure in
> that case that we want/need to rename the old one.
> 
> What about lcore_rank ?
> It may avoid confusion between "id" and "index", which are quite
> close visually and phonetically.

Not sure about rank, index is more correct. How about making it "app_index" or "app_idx", to indicate that it's not a global id but rather the idx that's local to the running app instance.

Other alternative approach would be rte_lcore_position() API that takes a hardware lcore id, and tells you it's "position" in the coremask for the application, i.e. lcore 6 is in position 2 (of e.g. 5) lcores, for instance. [It would obviously return -1 on non-active cores.]

> 
> I agree that we should not change the old lcore_id, its name is already
> appropriate.
> 
And it's so widely used that changing it would break the code of probably every single Intel DPDK application ever written!

Re: [dpdk-dev] [PATCH] Add an API to query enabled core index

[Patrick Lu]

On Thu, Jun 12, 2014 at 08:54:11AM -0700, Richardson, Bruce wrote:
> 
> 
> > -----Original Message-----
> > From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> > Sent: Thursday, June 12, 2014 1:20 AM
> > To: Richardson, Bruce; Thomas Monjalon; Lu, Patrick
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] Add an API to query enabled core index
> > 
> > Hello,
> > 
> > On 06/11/2014 11:57 PM, Richardson, Bruce wrote:
> > >> I think core_id2 is not a representative name.
> > >> What do you think of renaming core_id as lcore_hwid and core_id2 as
> > >> lcore_index?
> > >>
> > >> --
> > > I like lcore_index as the name for the new function. However, I'm not sure in
> > that case that we want/need to rename the old one.
> > 
> > What about lcore_rank ?
> > It may avoid confusion between "id" and "index", which are quite
> > close visually and phonetically.
> 
> Not sure about rank, index is more correct. How about making it "app_index" or "app_idx", to indicate that it's not a global id but rather the idx that's local to the running app instance.
> 
> Other alternative approach would be rte_lcore_position() API that takes a hardware lcore id, and tells you it's "position" in the coremask for the application, i.e. lcore 6 is in position 2 (of e.g. 5) lcores, for instance. [It would obviously return -1 on non-active cores.]
The main purpose of this API is for a running thread know its relative
index in all enabled core, so it can access the shared data structure
with correct index. I don't know if we necessarily need to pass in a
hardware lcore id, I suggest the API will implicit call rte_lcore_id.

I think either position or index is a much appropriated name for this
API.
> 
> > 
> > I agree that we should not change the old lcore_id, its name is already
> > appropriate.
> > 
> And it's so widely used that changing it would break the code of probably every single Intel DPDK application ever written!

Re: [dpdk-dev] [PATCH] Add an API to query enabled core index

[Richardson, Bruce]

> -----Original Message-----
> From: Lu, Patrick
> Sent: Friday, June 13, 2014 9:58 AM
> To: Richardson, Bruce
> Cc: Olivier MATZ; Thomas Monjalon; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] Add an API to query enabled core index
> 
> On Thu, Jun 12, 2014 at 08:54:11AM -0700, Richardson, Bruce wrote:
> >
> >
> > > -----Original Message-----
> > > From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> > > Sent: Thursday, June 12, 2014 1:20 AM
> > > To: Richardson, Bruce; Thomas Monjalon; Lu, Patrick
> > > Cc: dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH] Add an API to query enabled core index
> > >
> > > Hello,
> > >
> > > On 06/11/2014 11:57 PM, Richardson, Bruce wrote:
> > > >> I think core_id2 is not a representative name.
> > > >> What do you think of renaming core_id as lcore_hwid and core_id2 as
> > > >> lcore_index?
> > > >>
> > > >> --
> > > > I like lcore_index as the name for the new function. However, I'm not sure
> in
> > > that case that we want/need to rename the old one.
> > >
> > > What about lcore_rank ?
> > > It may avoid confusion between "id" and "index", which are quite
> > > close visually and phonetically.
> >
> > Not sure about rank, index is more correct. How about making it "app_index"
> or "app_idx", to indicate that it's not a global id but rather the idx that's local to
> the running app instance.
> >
> > Other alternative approach would be rte_lcore_position() API that takes a
> hardware lcore id, and tells you it's "position" in the coremask for the
> application, i.e. lcore 6 is in position 2 (of e.g. 5) lcores, for instance. [It would
> obviously return -1 on non-active cores.]
> The main purpose of this API is for a running thread know its relative
> index in all enabled core, so it can access the shared data structure
> with correct index. I don't know if we necessarily need to pass in a
> hardware lcore id, I suggest the API will implicit call rte_lcore_id.

Yes, having the API call rte_lcore_id internally is simpler. However, the advantage of having the API take the hardware core id means that it then becomes possible for one thread to query the position of another. However, how likely this scenario is to be useful, I'm not sure. 

> 
> I think either position or index is a much appropriated name for this
> API.

What do you think of "rte_lcore_app_idx()" then as the name? This hopefully is descriptive enough and should allow us to leave the existing API unmodified.

Re: [dpdk-dev] [PATCH] Add an API to query enabled core index

[Lu, Patrick]

-----Original Message-----
From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] 
Sent: Wednesday, June 11, 2014 2:51 PM
To: Lu, Patrick
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] Add an API to query enabled core index

Hi,

2014-06-11 13:45, Patrick Lu:
> EAL -c option allows the user to enable any lcore in the system.
> Oftentimes, the user app wants to know 1st enabled core, 2nd enabled 
> core, etc, rather than phyical core ID (rte_lcore_id().)
> 
> The new API rte_lcore_id2() will return an index from enabled lcores 
> starting from zero.

I think core_id2 is not a representative name.
What do you think of renaming core_id as lcore_hwid and core_id2 as lcore_index?

--
Thomas

I think this is a good idea. Except core_id is used in 13 other places. Should I resubmit the patch with core_id renamed it lcore_hwid?

Patrick

Re: [dpdk-dev] [PATCH] Add an API to query enabled core index

[Thomas Monjalon]

2014-06-11 21:58, Lu, Patrick:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] 
> > 2014-06-11 13:45, Patrick Lu:
> > > The new API rte_lcore_id2() will return an index from enabled lcores
> > > starting from zero.
> > 
> > I think core_id2 is not a representative name.
> > What do you think of renaming core_id as lcore_hwid and core_id2 as
> > lcore_index? 
> 
> I think this is a good idea. Except core_id is used in 13 other places.
> Should I resubmit the patch with core_id renamed it lcore_hwid?

It should be in a separated patch. A patch-serie would be appreciated.

By the way, I don't see any reason to integrate this change in DPDK 1.7.0
as we are in feature freeze phase.

-- 
Thomas