[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 at 6wind.com] > > Sent: Thursday, June 12, 2014 1:20 AM > > To: Richardson, Bruce; Thomas Monjalon; Lu, Patrick > > Cc: dev at 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!
[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. I agree that we should not change the old lcore_id, its name is already appropriate. Regards, Olivier
[dpdk-dev] [PATCH] Add an API to query enabled core index
2014-06-11 21:58, Lu, Patrick: > From: Thomas Monjalon [mailto:thomas.monjalon at 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
[dpdk-dev] [PATCH] Add an API to query enabled core index
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-boun...@dpdk.org] On Behalf Of Richardson, Bruce Sent: Thursday, June 12, 2014 12:28 AM To: Thomas Monjalon Cc: dev at dpdk.org Subject: Re: [dpdk-dev] [PATCH] Add an API to query enabled core index > -Original Message- > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] > Sent: Wednesday, June 11, 2014 3:50 PM > To: Richardson, Bruce > Cc: Lu, Patrick; dev at 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 at 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.
[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
[dpdk-dev] [PATCH] Add an API to query enabled core index
> -Original Message- > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] > Sent: Wednesday, June 11, 2014 3:50 PM > To: Richardson, Bruce > Cc: Lu, Patrick; dev at 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 at 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.
[dpdk-dev] [PATCH] Add an API to query enabled core index
-Original Message- From: Thomas Monjalon [mailto:thomas.monja...@6wind.com] Sent: Wednesday, June 11, 2014 2:51 PM To: Lu, Patrick Cc: dev at 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
[dpdk-dev] [PATCH] Add an API to query enabled core index
> -Original Message- > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Thomas Monjalon > Sent: Wednesday, June 11, 2014 2:51 PM > To: Lu, Patrick > Cc: dev at 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.
[dpdk-dev] [PATCH] Add an API to query enabled core index
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
/**
+ * 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
