| Message ID | 1416935023-30145-1-git-send-email-ola.liljedahl@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On 25 November 2014 at 12:03, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org> > Works on x86 - reported the correct number of cores available http://www.gnu.org/software/libc/manual/html_node/Processor-Resources.html So I assume it fixes the big.LITTLE hang, however the API is called sysconf_core_count and the meager docs say it is "Core count". We might want to name this sysconf_avalable_core_count if the difference is going to matter in the future. I think this patch should improve the docs to clarify that it is the currently available core count even if we don't rename or add a new API. > --- > On ARM big.LITTLE systems with cluster switching, return the actual number > of > currently usable CPU's (e.g. 4, not 8). Trying to bind threads to cores > 4..7 > will just hang those threads. > > platform/linux-generic/odp_system_info.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/platform/linux-generic/odp_system_info.c > b/platform/linux-generic/odp_system_info.c > index 10665bb..60804cb 100644 > --- a/platform/linux-generic/odp_system_info.c > +++ b/platform/linux-generic/odp_system_info.c > @@ -51,7 +51,7 @@ static int sysconf_core_count(void) > { > long ret; > > - ret = sysconf(_SC_NPROCESSORS_CONF); > + ret = sysconf(_SC_NPROCESSORS_ONLN); > if (ret < 0) > return 0; > > -- > 1.9.1 > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
On 25 November 2014 at 21:07, Mike Holmes <mike.holmes@linaro.org> wrote: > > > On 25 November 2014 at 12:03, Ola Liljedahl <ola.liljedahl@linaro.org> > wrote: >> >> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org> > > > > Works on x86 - reported the correct number of cores available > http://www.gnu.org/software/libc/manual/html_node/Processor-Resources.html > > So I assume it fixes the big.LITTLE hang, however the API is called > sysconf_core_count and the meager docs say it is "Core count". We might > want to name this sysconf_avalable_core_count if the difference is going to > matter in the future. I think in this case, the kernel for the ChromeBook2 is somehow configured for 8 cores. That's why _CONF returns 8. But only 4 are ever online at the same time (and they are numbered 0..3). Since binding a thread to one of those cores that are not online doesn't automatically activate the selected core, I don't think the "configured" number of cores matters. Only the cores that are already online can be used from ODP (unless ODP should start bringing offline cores online but I don't think this is ODP's responsibility). > > I think this patch should improve the docs to clarify that it is the > currently available core count even if we don't rename or add a new API. Eventually the value pops out here: odp_system_info.h: /** * Core count * * @return Core count */ int odp_sys_core_count(void); The documentation is not specific at all for what cores that count. -- Ola > >> >> --- >> On ARM big.LITTLE systems with cluster switching, return the actual number >> of >> currently usable CPU's (e.g. 4, not 8). Trying to bind threads to cores >> 4..7 >> will just hang those threads. >> >> platform/linux-generic/odp_system_info.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/platform/linux-generic/odp_system_info.c >> b/platform/linux-generic/odp_system_info.c >> index 10665bb..60804cb 100644 >> --- a/platform/linux-generic/odp_system_info.c >> +++ b/platform/linux-generic/odp_system_info.c >> @@ -51,7 +51,7 @@ static int sysconf_core_count(void) >> { >> long ret; >> >> - ret = sysconf(_SC_NPROCESSORS_CONF); >> + ret = sysconf(_SC_NPROCESSORS_ONLN); >> if (ret < 0) >> return 0; >> >> -- >> 1.9.1 >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> http://lists.linaro.org/mailman/listinfo/lng-odp > > > > > -- > Mike Holmes > Linaro Sr Technical Manager > LNG - ODP
On 25 November 2014 at 21:07, Mike Holmes <mike.holmes@linaro.org> wrote: > > > On 25 November 2014 at 12:03, Ola Liljedahl <ola.liljedahl@linaro.org> > wrote: >> >> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org> > > > > Works on x86 - reported the correct number of cores available > http://www.gnu.org/software/libc/manual/html_node/Processor-Resources.html > > So I assume it fixes the big.LITTLE hang, however the API is called > sysconf_core_count and the meager docs say it is "Core count". We might > want to name this sysconf_avalable_core_count if the difference is going to > matter in the future. > > I think this patch should improve the docs to clarify that it is the > currently available core count even if we don't rename or add a new API. Hasn't this API been better document in some other patch? I just updated my repo and the description seems not to have changed: * @return Core count I think this patch is still relevant. As ODP cannot make cores online (e.g. hotplug cores), it only makes sense to report (the number of) cores already online and available to ODP instead of some potential number of cores. Do I need to resubmit the patch with a description of it reporting "the number of online cores" and update the header file as well? > >> >> --- >> On ARM big.LITTLE systems with cluster switching, return the actual number >> of >> currently usable CPU's (e.g. 4, not 8). Trying to bind threads to cores >> 4..7 >> will just hang those threads. >> >> platform/linux-generic/odp_system_info.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/platform/linux-generic/odp_system_info.c >> b/platform/linux-generic/odp_system_info.c >> index 10665bb..60804cb 100644 >> --- a/platform/linux-generic/odp_system_info.c >> +++ b/platform/linux-generic/odp_system_info.c >> @@ -51,7 +51,7 @@ static int sysconf_core_count(void) >> { >> long ret; >> >> - ret = sysconf(_SC_NPROCESSORS_CONF); >> + ret = sysconf(_SC_NPROCESSORS_ONLN); >> if (ret < 0) >> return 0; >> >> -- >> 1.9.1 >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> http://lists.linaro.org/mailman/listinfo/lng-odp > > > > > -- > Mike Holmes > Linaro Sr Technical Manager > LNG - ODP
On 15 December 2014 at 08:26, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > > On 25 November 2014 at 21:07, Mike Holmes <mike.holmes@linaro.org> wrote: > > > > > > On 25 November 2014 at 12:03, Ola Liljedahl <ola.liljedahl@linaro.org> > > wrote: > >> > >> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org> > > > > > > > > Works on x86 - reported the correct number of cores available > > > http://www.gnu.org/software/libc/manual/html_node/Processor-Resources.html > > > > So I assume it fixes the big.LITTLE hang, however the API is called > > sysconf_core_count and the meager docs say it is "Core count". We might > > want to name this sysconf_avalable_core_count if the difference is going > to > > matter in the future. > > > > I think this patch should improve the docs to clarify that it is the > > currently available core count even if we don't rename or add a new API. > Hasn't this API been better document in some other patch? I just > updated my repo and the description seems not to have changed: > * @return Core count > > I think this patch is still relevant. As ODP cannot make cores online > (e.g. hotplug cores), it only makes sense to report (the number of) > cores already online and available to ODP instead of some potential > number of cores. > > Do I need to resubmit the patch with a description of it reporting > "the number of online cores" and update the header file as well? > As the commenter on the this patch, you and I need to agree on how to get it in. I would like to see this fix in and the docs updating to match because getting the doc updated is proving hard. > > > >> > >> --- > >> On ARM big.LITTLE systems with cluster switching, return the actual > number > >> of > >> currently usable CPU's (e.g. 4, not 8). Trying to bind threads to cores > >> 4..7 > >> will just hang those threads. > >> > >> platform/linux-generic/odp_system_info.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/platform/linux-generic/odp_system_info.c > >> b/platform/linux-generic/odp_system_info.c > >> index 10665bb..60804cb 100644 > >> --- a/platform/linux-generic/odp_system_info.c > >> +++ b/platform/linux-generic/odp_system_info.c > >> @@ -51,7 +51,7 @@ static int sysconf_core_count(void) > >> { > >> long ret; > >> > >> - ret = sysconf(_SC_NPROCESSORS_CONF); > >> + ret = sysconf(_SC_NPROCESSORS_ONLN); > >> if (ret < 0) > >> return 0; > >> > >> -- > >> 1.9.1 > >> > >> > >> _______________________________________________ > >> lng-odp mailing list > >> lng-odp@lists.linaro.org > >> http://lists.linaro.org/mailman/listinfo/lng-odp > > > > > > > > > > -- > > Mike Holmes > > Linaro Sr Technical Manager > > LNG - ODP >
On 15 December 2014 at 19:37, Mike Holmes <mike.holmes@linaro.org> wrote: > > > On 15 December 2014 at 08:26, Ola Liljedahl <ola.liljedahl@linaro.org> > wrote: >> >> On 25 November 2014 at 21:07, Mike Holmes <mike.holmes@linaro.org> wrote: >> > >> > >> > On 25 November 2014 at 12:03, Ola Liljedahl <ola.liljedahl@linaro.org> >> > wrote: >> >> >> >> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org> >> > >> > >> > >> > Works on x86 - reported the correct number of cores available >> > >> > http://www.gnu.org/software/libc/manual/html_node/Processor-Resources.html >> > >> > So I assume it fixes the big.LITTLE hang, however the API is called >> > sysconf_core_count and the meager docs say it is "Core count". We >> > might >> > want to name this sysconf_avalable_core_count if the difference is going >> > to >> > matter in the future. >> > >> > I think this patch should improve the docs to clarify that it is the >> > currently available core count even if we don't rename or add a new API. >> Hasn't this API been better document in some other patch? I just >> updated my repo and the description seems not to have changed: >> * @return Core count >> >> I think this patch is still relevant. As ODP cannot make cores online >> (e.g. hotplug cores), it only makes sense to report (the number of) >> cores already online and available to ODP instead of some potential >> number of cores. >> >> Do I need to resubmit the patch with a description of it reporting >> "the number of online cores" and update the header file as well? > > > As the commenter on the this patch, you and I need to agree on how to get it > in. I make the patch, you approve it, Maxim merges it. > I would like to see this fix in and the docs updating to match because > getting the doc updated is proving hard. The name of the function (odp_core_count) is also wrong. We are interested in what Linux calls CPU's. They map to HW threads if the hardware uses such (and they are enabled). I think we should reuse the term Linux uses since we seem not to be able to find something better. > >> >> > >> >> >> >> --- >> >> On ARM big.LITTLE systems with cluster switching, return the actual >> >> number >> >> of >> >> currently usable CPU's (e.g. 4, not 8). Trying to bind threads to cores >> >> 4..7 >> >> will just hang those threads. >> >> >> >> platform/linux-generic/odp_system_info.c | 2 +- >> >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> >> >> diff --git a/platform/linux-generic/odp_system_info.c >> >> b/platform/linux-generic/odp_system_info.c >> >> index 10665bb..60804cb 100644 >> >> --- a/platform/linux-generic/odp_system_info.c >> >> +++ b/platform/linux-generic/odp_system_info.c >> >> @@ -51,7 +51,7 @@ static int sysconf_core_count(void) >> >> { >> >> long ret; >> >> >> >> - ret = sysconf(_SC_NPROCESSORS_CONF); >> >> + ret = sysconf(_SC_NPROCESSORS_ONLN); >> >> if (ret < 0) >> >> return 0; >> >> >> >> -- >> >> 1.9.1 >> >> >> >> >> >> _______________________________________________ >> >> lng-odp mailing list >> >> lng-odp@lists.linaro.org >> >> http://lists.linaro.org/mailman/listinfo/lng-odp >> > >> > >> > >> > >> > -- >> > Mike Holmes >> > Linaro Sr Technical Manager >> > LNG - ODP > > > > -- > Mike Holmes > Linaro Sr Technical Manager > LNG - ODP
On 15 December 2014 at 20:41, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > On 15 December 2014 at 19:37, Mike Holmes <mike.holmes@linaro.org> wrote: >> >> >> On 15 December 2014 at 08:26, Ola Liljedahl <ola.liljedahl@linaro.org> >> wrote: >>> >>> On 25 November 2014 at 21:07, Mike Holmes <mike.holmes@linaro.org> wrote: >>> > >>> > >>> > On 25 November 2014 at 12:03, Ola Liljedahl <ola.liljedahl@linaro.org> >>> > wrote: >>> >> >>> >> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org> >>> > >>> > >>> > >>> > Works on x86 - reported the correct number of cores available >>> > >>> > http://www.gnu.org/software/libc/manual/html_node/Processor-Resources.html >>> > >>> > So I assume it fixes the big.LITTLE hang, however the API is called >>> > sysconf_core_count and the meager docs say it is "Core count". We >>> > might >>> > want to name this sysconf_avalable_core_count if the difference is going >>> > to >>> > matter in the future. >>> > >>> > I think this patch should improve the docs to clarify that it is the >>> > currently available core count even if we don't rename or add a new API. >>> Hasn't this API been better document in some other patch? I just >>> updated my repo and the description seems not to have changed: >>> * @return Core count >>> >>> I think this patch is still relevant. As ODP cannot make cores online >>> (e.g. hotplug cores), it only makes sense to report (the number of) >>> cores already online and available to ODP instead of some potential >>> number of cores. >>> >>> Do I need to resubmit the patch with a description of it reporting >>> "the number of online cores" and update the header file as well? >> >> >> As the commenter on the this patch, you and I need to agree on how to get it >> in. > I make the patch, you approve it, Maxim merges it. > >> I would like to see this fix in and the docs updating to match because >> getting the doc updated is proving hard. > The name of the function (odp_core_count) is also wrong. We are > interested in what Linux calls CPU's. They map to HW threads if the > hardware uses such (and they are enabled). I think we should reuse the > term Linux uses since we seem not to be able to find something better. This is also strange: printf(" -c, --count <number> core count, core IDs start from 1\n"); Why do we start numbering cores (or CPU's) from 1? And why is this important here, the user can specify the number of cores (CPU's), not which one are actually used. The description is wrong or at least irrelevant. > >> >>> >>> > >>> >> >>> >> --- >>> >> On ARM big.LITTLE systems with cluster switching, return the actual >>> >> number >>> >> of >>> >> currently usable CPU's (e.g. 4, not 8). Trying to bind threads to cores >>> >> 4..7 >>> >> will just hang those threads. >>> >> >>> >> platform/linux-generic/odp_system_info.c | 2 +- >>> >> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >> >>> >> diff --git a/platform/linux-generic/odp_system_info.c >>> >> b/platform/linux-generic/odp_system_info.c >>> >> index 10665bb..60804cb 100644 >>> >> --- a/platform/linux-generic/odp_system_info.c >>> >> +++ b/platform/linux-generic/odp_system_info.c >>> >> @@ -51,7 +51,7 @@ static int sysconf_core_count(void) >>> >> { >>> >> long ret; >>> >> >>> >> - ret = sysconf(_SC_NPROCESSORS_CONF); >>> >> + ret = sysconf(_SC_NPROCESSORS_ONLN); >>> >> if (ret < 0) >>> >> return 0; >>> >> >>> >> -- >>> >> 1.9.1 >>> >> >>> >> >>> >> _______________________________________________ >>> >> lng-odp mailing list >>> >> lng-odp@lists.linaro.org >>> >> http://lists.linaro.org/mailman/listinfo/lng-odp >>> > >>> > >>> > >>> > >>> > -- >>> > Mike Holmes >>> > Linaro Sr Technical Manager >>> > LNG - ODP >> >> >> >> -- >> Mike Holmes >> Linaro Sr Technical Manager >> LNG - ODP
Changing "core" to "cpu":
modified: example/generator/odp_generator.c
modified: example/ipsec/odp_ipsec.c
modified: example/l2fwd/odp_l2fwd.c
modified: example/odp_example/odp_example.c
modified: example/packet/odp_pktio.c
modified: example/timer/odp_timer_test.c
modified: helper/include/odph_linux.h
modified: platform/linux-generic/include/api/odp_system_info.h
modified: platform/linux-generic/include/api/odp_thread.h
modified: platform/linux-generic/odp_linux.c
modified: platform/linux-generic/odp_system_info.c
modified: platform/linux-generic/odp_thread.c
modified: test/api_test/odp_atomic_test.c
modified: test/api_test/odp_common.c
modified: test/api_test/odp_ring_test.c
modified: test/api_test/odp_shm_test.c
Painful but it has to be done.
On 15 December 2014 at 21:18, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > Changing "core" to "cpu": > > modified: example/generator/odp_generator.c > modified: example/ipsec/odp_ipsec.c > modified: example/l2fwd/odp_l2fwd.c > modified: example/odp_example/odp_example.c > modified: example/packet/odp_pktio.c > modified: example/timer/odp_timer_test.c > modified: helper/include/odph_linux.h > modified: platform/linux-generic/include/api/odp_system_info.h > modified: platform/linux-generic/include/api/odp_thread.h > modified: platform/linux-generic/odp_linux.c > modified: platform/linux-generic/odp_system_info.c > modified: platform/linux-generic/odp_thread.c > modified: test/api_test/odp_atomic_test.c > modified: test/api_test/odp_common.c > modified: test/api_test/odp_ring_test.c > modified: test/api_test/odp_shm_test.c > > Painful but it has to be done. Since they all depend on this specific change: /** - * Core count + * CPU count + * Report the number of CPU's available to this ODP program. + * This may be smaller than the number of (online) CPU's in the system. * - * @return Core count + * @return Number of available CPU's */ -int odp_sys_core_count(void); +int odp_sys_cpu_count(void); I assume that all changes have to be part of the same patch. Changing public API's is painful. And it doesn't get easier if you wait.
I agree. Post-v1.0 we'll need to consider a more formal API deprecation process for small changes like this, but for now it's best to just do them and be done with it. On Mon, Dec 15, 2014 at 2:39 PM, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > > On 15 December 2014 at 21:18, Ola Liljedahl <ola.liljedahl@linaro.org> > wrote: > > Changing "core" to "cpu": > > > > modified: example/generator/odp_generator.c > > modified: example/ipsec/odp_ipsec.c > > modified: example/l2fwd/odp_l2fwd.c > > modified: example/odp_example/odp_example.c > > modified: example/packet/odp_pktio.c > > modified: example/timer/odp_timer_test.c > > modified: helper/include/odph_linux.h > > modified: platform/linux-generic/include/api/odp_system_info.h > > modified: platform/linux-generic/include/api/odp_thread.h > > modified: platform/linux-generic/odp_linux.c > > modified: platform/linux-generic/odp_system_info.c > > modified: platform/linux-generic/odp_thread.c > > modified: test/api_test/odp_atomic_test.c > > modified: test/api_test/odp_common.c > > modified: test/api_test/odp_ring_test.c > > modified: test/api_test/odp_shm_test.c > > > > Painful but it has to be done. > Since they all depend on this specific change: > /** > - * Core count > + * CPU count > + * Report the number of CPU's available to this ODP program. > + * This may be smaller than the number of (online) CPU's in the system. > * > - * @return Core count > + * @return Number of available CPU's > */ > -int odp_sys_core_count(void); > +int odp_sys_cpu_count(void); > I assume that all changes have to be part of the same patch. Changing > public API's is painful. And it doesn't get easier if you wait. > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
diff --git a/platform/linux-generic/odp_system_info.c b/platform/linux-generic/odp_system_info.c index 10665bb..60804cb 100644 --- a/platform/linux-generic/odp_system_info.c +++ b/platform/linux-generic/odp_system_info.c @@ -51,7 +51,7 @@ static int sysconf_core_count(void) { long ret; - ret = sysconf(_SC_NPROCESSORS_CONF); + ret = sysconf(_SC_NPROCESSORS_ONLN); if (ret < 0) return 0;
Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org> --- On ARM big.LITTLE systems with cluster switching, return the actual number of currently usable CPU's (e.g. 4, not 8). Trying to bind threads to cores 4..7 will just hang those threads. platform/linux-generic/odp_system_info.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)