On 05.11.15 12:29, Maxim Uvarov wrote:
Hello Hongbo,
are you looking to Ivan's solutions?
If will take time then please do:
1. Open bug for that issue describing problem and possible solutions.
2. Add ODP_DBG("") with description and bug bug link, that dummy files are used.
api-next branch fails in CI, and I would like to see there at least some
temporary solution.
Thanks,
Maxim.
There was a proposition to make APIs like:
odp_cpu_hz_max(), odp_cpu_hz_max_id(), odp_cpu_hz_id()
to be deprecated as they doesn't reflect correct information any way and
after adding time API used only for not accurate information.
In place of linux-generic when it tries to emulate CPU cycles with
odp_cpu_hz_max()
it can be made as local function.
On 11/02/2015 16:40, Ivan Khoronzhuk wrote:
Just forgot to mention. I've added several patches some time ago
to linux kernel to present SMBIOS tables to userspace via sysfs.
Currently this sysfs entries are used by dmidecode util to retrieve
system h/w information. For ARM64 it's planned to support UEFI in the
future and these tables are filled in UEFI firmware. For x86 it's not
a problem for now. So you can use dmidecode sources as example how to
read DMI tables and retrieve maximum and current cpufrequency.
For my PC it looks like:
Processor Information
Socket Designation: CPU 1
Type: Central Processor
Family: Core i5
Manufacturer: Intel
ID: A7 06 02 00 FF FB EB BF
Signature: Type 0, Family 6, Model 42, Stepping 7
Flags:
FPU (Floating-point unit on-chip)
VME (Virtual mode extension)
DE (Debugging extension)
PSE (Page size extension)
TSC (Time stamp counter)
MSR (Model specific registers)
PAE (Physical address extension)
MCE (Machine check exception)
CX8 (CMPXCHG8 instruction supported)
APIC (On-chip APIC hardware supported)
SEP (Fast system call)
MTRR (Memory type range registers)
PGE (Page global enable)
MCA (Machine check architecture)
CMOV (Conditional move instruction supported)
PAT (Page attribute table)
PSE-36 (36-bit page size extension)
CLFSH (CLFLUSH instruction supported)
DS (Debug store)
ACPI (ACPI supported)
MMX (MMX technology supported)
FXSR (FXSAVE and FXSTOR instructions supported)
SSE (Streaming SIMD extensions)
SSE2 (Streaming SIMD extensions 2)
SS (Self-snoop)
HTT (Multi-threading)
TM (Thermal monitor supported)
PBE (Pending break enabled)
Version: Intel(R) Core(TM) i5-2520M CPU @ 2.50GHz
Voltage: 1.1 V
External Clock: 100 MHz
Max Speed: 2500 MHz
Current Speed: 2500 MHz
Status: Populated, Enabled
Upgrade: Other
L1 Cache Handle: 0x0005
L2 Cache Handle: 0x0006
L3 Cache Handle: 0x0007
Serial Number: To Be Filled By O.E.M.
Asset Tag: To Be Filled By O.E.M.
Part Number: To Be Filled By O.E.M.
Core Count: 2
Core Enabled: 1
Thread Count: 2
Characteristics:
64-bit capable
You need strings:
"
Max Speed: 2500 MHz
Current Speed: 2500 MHz
"
Last time I looked at it target was to support it for aarch64 but seems only
with UEFI.
Any way, you can also check if there sysfs entries are exist, and if so,
retrieve this information.
Actually dmidecode util retrieves this information with sysfs entries
(/sys/firmware/dmi) starting from kernel v4.2.
See patches in the kernel:
863ef5b Documentation: ABI: sysfs-firmware-dmi: add -entries suffix to file name
d7f96f97 firmware: dmi_scan: add SBMIOS entry and DMI tables
Read Documentation/ABI/testing/sysfs-firmware-dmi-tables how to use them.
They are binary sysfs entries.
Before LK v4.2 dmidecode retrieved this information with /dev/mem.
Again, as example use dmidecode sources that are currently present in git:
"git://git.savannah.nongnu.org/dmidecode.git"
But it requires /dev/mem to be enabled by the system.
Also all of that requires root rights.
Equally as /sys/kernel/debug/clk in previous reply.
To get SMBIOS (DMI) reference specification you can use:
http://www.dmtf.org/standards/smbios
It should work for Windows also in some way.
Oh, just remember.
You can retrieve dmi tables in ASCII view if dmi-sysfs is enable in the kernel.
But it required to be switched on in kernel config before build. It's not
standard
feature. But /sys/firmware/dmi are absolutely reliable, as they are always
compiled
in the kernel. Only exception, if bootloader (like BIOS, UEFI ..etc) don't
present
dmi tables, then they are simply absent and you can use another approach.
Again, for ARM, it should be retrieved from cpufreq first and only if it's off
-> dmi tables,
...then other approaches.
On 02.11.15 13:57, Ivan Khoronzhuk wrote:
Device tree has node for CPU that contains clock-frequency.
(ePAPR v1.1)
But it can be used only if cpufreq is off. And it also doesn't guarantee
that frequency of the CPU was not changed by the kernel in some place.
By a big account it can be changed only by cpufreq, so maybe used, I think.
There is also can be that it's not defined in the device-tree /proc/device-tree,
and it's frequently situation.
It also can be retrieved in /sys/kernel/debug/clk/clk_dump
or /sys/kernel/debug/clk/clk_summary. But I'm not sure if it's
always present, it prints clock states on the SoC in view of tree.
Also, IMHO, we should use different approaches for different archs and don't
try to make it common.
On 02.11.15 11:23, Maxim Uvarov wrote:
Doesn’t device tree provide that value?
Maxim.
On 11/02/2015 12:07, Hongbo Zhang wrote:
Advantage of acquiring max freq from /sys/devices/system/cpu*/cpufreq/
is it's much easier, but this requires freq-scaling should be turned
on. what's more, this sysfs is identical for all platforms, if we use
if on ARM, we should use it on the other platforms too.
Upstream max freq into /proc/cpuinfo may be harder, there are many
variants of ARM, not sure if one patch can cover them, not sure if the
maintainer likes such patch.
On 2 November 2015 at 16:44, Hongbo Zhang <[email protected]> wrote:
I should clarify in my previous mails, I used sys fs instead of proc
of sometimes.
On x86 the max freq is acquired from /proc/cpuinfo, on my ARM
platform, this file exists too, but no freq info included there. This
file should always exist, whatever freq-scaling is turned on or not,
so max freq should be acquired here.
If freq-scaling feature is turned on, the sysfs entry
/sys/devices/system/cpu*/cpufreq/ is created, we can acquire current
freq here. If this feature isn't enabled, no such file created, and no
concept of "current freq" at all, because CPU always run at max speed.
We shouldn't acquire max freq here because this sysfs isn't always
there, so I think we should add more info into /proc/cpuinfo for ARM
for max freq.
Let's wait for Stuart to see if he has more findings.
On 2 November 2015 at 15:10, Maxim Uvarov <[email protected]> wrote:
On 11/02/2015 09:27, Hongbo Zhang wrote:
Maxim,
Do you like my idea that merging this patch first and then fixing
other issues in separate patches in future? since this patch itself
has no problem and can eliminate system validation failure.
Hi Hongbo, I talked with Stuart, he has some ideas how to implement max hz
on arm.
Might be on your LAVA box cpus freq is not turned on in kernel so that you
do not see
sysfs files. I will wait for his feedback.
Maxim.
On 30 October 2015 at 20:48, Hongbo Zhang <[email protected]> wrote:
On 30 October 2015 at 18:18, Maxim Uvarov <[email protected]>
wrote:
On 10/30/2015 13:10, Hongbo Zhang wrote:
My "0 does not work" meas: if 0 returns, the odp may crrupt.
for example, odp_cpu_cycles() calls odp_cpu_hz_max()
uint64_t odp_cpu_cycles(void)
{
struct timespec time;
uint64_t sec, ns, hz, cycles;
int ret;
ret = clock_gettime(CLOCK_MONOTONIC_RAW, &time);
if (ret != 0)
ODP_ABORT("clock_gettime failed\n");
hz = odp_cpu_hz_max();
sec = (uint64_t)time.tv_sec;
ns = (uint64_t)time.tv_nsec;
cycles = sec * hz;
cycles += (ns * hz) / GIGA;
return cycles;
Ok, but if you put here some dummy value that means odp_cpus_cycles() is
corrupted. But that call is used
to calculate performance metrics for for functions execution. And in
case
with dummy number output number
will be useless.
Dummy value cannot be avoided at this stage, then this patch should be
no problem, it only pad the array to avoid validation failure.
Other problem of test code you pointed out can be fixed in other patches
later.
And whats more we should do on odp_system_info man include:
a) Implement ARM sysfs to report to user space the CPU info, and let
ODP to use real data instead of dummy one ASAP.
b) I only implemented x86 per-CPU interface, other platforms should be
updated/implemented in this pattern
c) Separate platform specific implementation into their own platform
file if possible.
Maxim.
}
Then odp_cpu_cycles() is called by odp_scheduling.c, even I didn't
check into code deeply, but if odp_cpu_hz_max() returns 0, then odp
cannot run I think, that is why a dumy 1400000000 is returned
(original implementation, not by me), without this dummy data, we
cannot run odp on such a platform.
So a dummy data is needed currently, and a final solution should be
implemented ASAP, eg mimic sysfs interfaces as x86 platforms.
In fact there are much things to clean up in odp_sysinfo.c, such as
separating each platform specific functions into there own platform
files.
On 30 October 2015 at 17:31, Maxim Uvarov <[email protected]>
wrote:
On 10/30/2015 11:47, Hongbo Zhang wrote:
On 30 October 2015 at 15:17, Maxim Uvarov <[email protected]>
wrote:
On 10/30/2015 09:24, Hongbo Zhang wrote:
On 29 October 2015 at 21:25, Maxim Uvarov <[email protected]>
wrote:
On 10/29/2015 13:51, Hongbo Zhang wrote:
This patch is for https://bugs.linaro.org/show_bug.cgi?id=1870,
and
has been tested on Juno.
On 29 October 2015 at 18:45, <[email protected]> wrote:
From: Hongbo Zhang <[email protected]>
In the default dummy function systemcpu(), only cpu_hz[0] and
model_str[0]
are set to dummy values, then in the validation code if iterate
each
CPU,
cores other than core 0 report failure, this patchs pad all the
arrays
to
default values to pass validation.
Signed-off-by: Hongbo Zhang <[email protected]>
---
platform/linux-generic/odp_system_info.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/platform/linux-generic/odp_system_info.c
b/platform/linux-generic/odp_system_info.c
index 8bd1584..5dbf68a 100644
--- a/platform/linux-generic/odp_system_info.c
+++ b/platform/linux-generic/odp_system_info.c
@@ -358,7 +358,7 @@ static int systemcpu(odp_system_info_t
*sysinfo)
static int systemcpu(odp_system_info_t *sysinfo)
{
- int ret;
+ int ret, i;
ret = sysconf_cpu_count();
if (ret == 0) {
@@ -371,10 +371,12 @@ static int systemcpu(odp_system_info_t
*sysinfo)
sysinfo->huge_page_size = huge_page_size();
/* Dummy values */
- sysinfo->cpu_hz[0] = 1400000000;
sysinfo->cache_line_size = 64;
- strncpy(sysinfo->model_str[0], "UNKNOWN",
sizeof(sysinfo->model_str));
+ for (i = 0; i < MAX_CPU_NUMBER; i++) {
+ sysinfo->cpu_hz[i] = 1400000000;
+ strcpy(sysinfo->model_str[i], "UNKNOWN");
why 1400000 and not 0.
In fact, this systemcpu() function is pseudo code, it is for ARM
platform, because there is no sufficient sysfs interface for such
info, this should be implemented to return real value sooner or
later.
140000 is legacy data, and if 0 the validation code returns
failure.
For the test, all the defects you pointed did exist there, I just
followed the previous code pattern. It is OK for me to take this
chance to fix them since I touched this part of code.
Thanks.
How about changing api and say that if returned hz is 0 that means
function
can not determine hz of current cpus.
And make 0 as valid number. I don't like some temporary fixes with
dummy
numbers due to other people can relay
on that number in their apps.
For the _current_ hz, it is OK to return 0 as valid number, currently
no app relies on it.
but for the _max_ hz, we have to make some dummy number, other odp
codes rely on it, 0 doesn't work even if we treat it as valid.
That if 0 does not work, call to get max hz should return error if you
can
not return real max hz value.
Maxim.
Maxim.
Also I think test is wrong. At least there is missmatch between
test
and
implementation:
void system_test_odp_cpu_hz(void)
{
uint64_t hz;
hz = odp_cpu_hz();
CU_ASSERT(0 < hz); <-- unsigned less then 0???
}
uint64_t odp_cpu_hz(void)
{
int id = sched_getcpu();
return arch_cpu_hz_current(id);
}
And finally:
static uint64_t arch_cpu_hz_current(int id ODP_UNUSED)
{
return -1;
}
So you return -1 for unsigned function.
In my understanding:
odp_cpu_hz(); on error has to return 0 and odp_err has to be set.
Btw, you have the same mismatches for all other functions:
odp_cpu_hz_id(cpu);
odp_cpu_hz_max_id(cpu);
All of that has to be fixed.
Thank you,
Maxim.
+ }
return 0;
}
--
2.1.4
_______________________________________________
lng-odp mailing list
[email protected]
https://lists.linaro.org/mailman/listinfo/lng-odp
_______________________________________________
lng-odp mailing list
[email protected]
https://lists.linaro.org/mailman/listinfo/lng-odp
_______________________________________________
lng-odp mailing list
[email protected]
https://lists.linaro.org/mailman/listinfo/lng-odp
--
Regards,
Ivan Khoronzhuk
_______________________________________________
lng-odp mailing list
[email protected]
https://lists.linaro.org/mailman/listinfo/lng-odp
