| Message ID | 1435223104-15434-2-git-send-email-hongbo.zhang@freescale.com |
|---|---|
| State | New |
| Headers | show |
Need to send this with API-NEXT in the subject because it touches include/odp/api AND in the subject, make it a patch series for the two separate ideas AND is a warning that you tried to do more than one thing in a patch. Also you added a new API to API-NEXT with an implementation but you did not update the validation tests suite to cover the new API On 25 June 2015 at 05:05, <hongbo.zhang@freescale.com> wrote: > From: Hongbo Zhang <hongbo.zhang@linaro.org> > > For AMP system such as ARM big.LITTLE, cores are heterogeneous, so cpu_hz > and model_str should be different too, so this patch changes variable > cpu_hz and model_str to data array to contain data for each different > core, while for the common SMP system, we can simply use the cpu_hz[0] and > model[0] to contain data for all cores because they are all same, but if > like, we can use each item in the data array too. > > Accordingly, odp_sys_cpu_hz_amp(cpu) and odp_sys_cpu_model_str_amp(cpu) > are added for AMP system, while the original version interfaces without > any parameter are kept for SMP systems. > > Currently, all the platforms implemented are all SMP, so only cpu_hz[0] > and model[0] are used, but when cpuinfo_arm() is implemented, this per-CPU > feature should not be missed because of its big.LITTLE. > > Signed-off-by: Hongbo Zhang <hongbo.zhang@linaro.org> > --- > include/odp/api/system_info.h | 4 ++ > platform/linux-generic/include/odp_internal.h | 6 ++- > platform/linux-generic/odp_system_info.c | 55 > +++++++++++++++++---------- > 3 files changed, 43 insertions(+), 22 deletions(-) > > diff --git a/include/odp/api/system_info.h b/include/odp/api/system_info.h > index 1f3294b..55a4180 100644 > --- a/include/odp/api/system_info.h > +++ b/include/odp/api/system_info.h > @@ -30,6 +30,8 @@ extern "C" { > */ > uint64_t odp_sys_cpu_hz(void); > > +uint64_t odp_sys_cpu_hz_amp(int cpu); > need to add a doxygen description to this new API element. > + > /** > * Huge page size in bytes > * > @@ -51,6 +53,8 @@ uint64_t odp_sys_page_size(void); > */ > const char *odp_sys_cpu_model_str(void); > > +const char *odp_sys_cpu_model_str_amp(int cpu); > + > need to add a doxygen description to this new API element. > /** > * Cache line size in bytes > * > diff --git a/platform/linux-generic/include/odp_internal.h > b/platform/linux-generic/include/odp_internal.h > index 8c5d339..7388128 100644 > --- a/platform/linux-generic/include/odp_internal.h > +++ b/platform/linux-generic/include/odp_internal.h > @@ -22,13 +22,15 @@ extern "C" { > > extern __thread int __odp_errno; > > +#define MAX_CPU_NUMBER 1024 > Is this a good number? It feels large for a normal case. > + > typedef struct { > - uint64_t cpu_hz; > + uint64_t cpu_hz[MAX_CPU_NUMBER]; > uint64_t huge_page_size; > uint64_t page_size; > int cache_line_size; > int cpu_count; > - char model_str[128]; > + char model_str[MAX_CPU_NUMBER][128]; > } odp_system_info_t; > > struct odp_global_data_s { > diff --git a/platform/linux-generic/odp_system_info.c > b/platform/linux-generic/odp_system_info.c > index 31df29e..c9f6be1 100644 > --- a/platform/linux-generic/odp_system_info.c > +++ b/platform/linux-generic/odp_system_info.c > @@ -36,7 +36,6 @@ typedef struct { > > #define HUGE_PAGE_DIR "/sys/kernel/mm/hugepages" > > - > /* > * Report the number of CPUs in the affinity mask of the main thread > */ > @@ -139,17 +138,17 @@ static int cpuinfo_x86(FILE *file, odp_system_info_t > *sysinfo) > if (pos) { > int len; > pos = strchr(str, ':'); > - strncpy(sysinfo->model_str, pos+2, > - sizeof(sysinfo->model_str)); > - len = strlen(sysinfo->model_str); > - sysinfo->model_str[len - 1] = 0; > + strncpy(sysinfo->model_str[0], pos + 2, > + sizeof(sysinfo->model_str[0])); > + len = strlen(sysinfo->model_str[0]); > + sysinfo->model_str[0][len - 1] = 0; > model = 1; > count--; > } > } > } > > - sysinfo->cpu_hz = (uint64_t) (mhz * 1000000.0); > + sysinfo->cpu_hz[0] = (uint64_t)(mhz * 1000000.0); > > return 0; > } > @@ -188,10 +187,10 @@ static int cpuinfo_octeon(FILE *file, > odp_system_info_t *sysinfo) > if (pos) { > int len; > pos = strchr(str, ':'); > - strncpy(sysinfo->model_str, pos+2, > - sizeof(sysinfo->model_str)); > - len = strlen(sysinfo->model_str); > - sysinfo->model_str[len - 1] = 0; > + strncpy(sysinfo->model_str[0], pos + 2, > + sizeof(sysinfo->model_str[0])); > + len = strlen(sysinfo->model_str[0]); > + sysinfo->model_str[0][len - 1] = 0; > model = 1; > count--; > } > @@ -199,7 +198,7 @@ static int cpuinfo_octeon(FILE *file, > odp_system_info_t *sysinfo) > } > > /* bogomips seems to be 2x freq */ > - sysinfo->cpu_hz = (uint64_t) (mhz * 1000000.0 / 2.0); > + sysinfo->cpu_hz[0] = (uint64_t)(mhz * 1000000.0 / 2.0); > > return 0; > } > @@ -228,16 +227,16 @@ static int cpuinfo_powerpc(FILE *file, > odp_system_info_t *sysinfo) > if (pos) { > int len; > pos = strchr(str, ':'); > - strncpy(sysinfo->model_str, pos+2, > - sizeof(sysinfo->model_str)); > - len = strlen(sysinfo->model_str); > - sysinfo->model_str[len - 1] = 0; > + strncpy(sysinfo->model_str[0], pos + 2, > + sizeof(sysinfo->model_str[0])); > + len = strlen(sysinfo->model_str[0]); > + sysinfo->model_str[0][len - 1] = 0; > model = 1; > count--; > } > } > > - sysinfo->cpu_hz = (uint64_t) (mhz * 1000000.0); > + sysinfo->cpu_hz[0] = (uint64_t)(mhz * 1000000.0); > } > > > @@ -330,10 +329,10 @@ static int systemcpu(odp_system_info_t *sysinfo) > sysinfo->huge_page_size = huge_page_size(); > > /* Dummy values */ > - sysinfo->cpu_hz = 1400000000; > + sysinfo->cpu_hz[0] = 1400000000; The "=" no longer line up > sysinfo->cache_line_size = 64; > > - strncpy(sysinfo->model_str, "UNKNOWN", sizeof(sysinfo->model_str)); > + strncpy(sysinfo->model_str[0], "UNKNOWN", > sizeof(sysinfo->model_str)); > > return 0; > } > @@ -376,7 +375,15 @@ int odp_system_info_init(void) > */ > uint64_t odp_sys_cpu_hz(void) > { > - return odp_global_data.system_info.cpu_hz; > + return odp_global_data.system_info.cpu_hz[0]; > +} > + > odp_sys_cpu_hz should call odp_sys_cpu_hz_amp(0) as its implementation so that there is no possibility of forgetting to modify something when updating the structures. E.g. the specific case calls the generic case with a specific argument. > +uint64_t odp_sys_cpu_hz_amp(int cpu) > +{ > + if (cpu >= 0 && cpu < MAX_CPU_NUMBER) > + return odp_global_data.system_info.cpu_hz[cpu]; > + else > + return -1; > } > uint64_t odp_sys_huge_page_size(void) > @@ -391,7 +398,15 @@ uint64_t odp_sys_page_size(void) > > const char *odp_sys_cpu_model_str(void) > { > - return odp_global_data.system_info.model_str; > + return odp_global_data.system_info.model_str[0]; > +} > + > the specific case should call the generic case with a specific argument. > +const char *odp_sys_cpu_model_str_amp(int cpu) > +{ > + if (cpu >= 0 && cpu < MAX_CPU_NUMBER) > + return odp_global_data.system_info.model_str[cpu]; > + else > + return NULL; > } > > int odp_sys_cache_line_size(void) > -- > 1.9.1 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
On 30 June 2015 at 04:15, Mike Holmes <mike.holmes@linaro.org> wrote: > Need to send this with API-NEXT in the subject because it touches > include/odp/api > OK, I see. > AND in the subject, make it a patch series for the two separate ideas AND is > a warning that you tried to do more than one thing in a patch. > I think you meant separating cpu_hz and model_str, yes it is OK to do so. > Also you added a new API to API-NEXT with an implementation but you did not > update the validation tests suite to cover the new API > Forgot the validation, will cover it. > On 25 June 2015 at 05:05, <hongbo.zhang@freescale.com> wrote: >> >> From: Hongbo Zhang <hongbo.zhang@linaro.org> >> >> For AMP system such as ARM big.LITTLE, cores are heterogeneous, so cpu_hz >> and model_str should be different too, so this patch changes variable >> cpu_hz and model_str to data array to contain data for each different >> core, while for the common SMP system, we can simply use the cpu_hz[0] and >> model[0] to contain data for all cores because they are all same, but if >> like, we can use each item in the data array too. >> >> Accordingly, odp_sys_cpu_hz_amp(cpu) and odp_sys_cpu_model_str_amp(cpu) >> are added for AMP system, while the original version interfaces without >> any parameter are kept for SMP systems. >> >> Currently, all the platforms implemented are all SMP, so only cpu_hz[0] >> and model[0] are used, but when cpuinfo_arm() is implemented, this per-CPU >> feature should not be missed because of its big.LITTLE. >> >> Signed-off-by: Hongbo Zhang <hongbo.zhang@linaro.org> >> --- >> include/odp/api/system_info.h | 4 ++ >> platform/linux-generic/include/odp_internal.h | 6 ++- >> platform/linux-generic/odp_system_info.c | 55 >> +++++++++++++++++---------- >> 3 files changed, 43 insertions(+), 22 deletions(-) >> >> diff --git a/include/odp/api/system_info.h b/include/odp/api/system_info.h >> index 1f3294b..55a4180 100644 >> --- a/include/odp/api/system_info.h >> +++ b/include/odp/api/system_info.h >> @@ -30,6 +30,8 @@ extern "C" { >> */ >> uint64_t odp_sys_cpu_hz(void); >> >> +uint64_t odp_sys_cpu_hz_amp(int cpu); > > > need to add a doxygen description to this new API element. > >> >> + >> /** >> * Huge page size in bytes >> * >> @@ -51,6 +53,8 @@ uint64_t odp_sys_page_size(void); >> */ >> const char *odp_sys_cpu_model_str(void); >> >> +const char *odp_sys_cpu_model_str_amp(int cpu); >> + > > > need to add a doxygen description to this new API element. > >> >> /** >> * Cache line size in bytes >> * >> diff --git a/platform/linux-generic/include/odp_internal.h >> b/platform/linux-generic/include/odp_internal.h >> index 8c5d339..7388128 100644 >> --- a/platform/linux-generic/include/odp_internal.h >> +++ b/platform/linux-generic/include/odp_internal.h >> @@ -22,13 +22,15 @@ extern "C" { >> >> extern __thread int __odp_errno; >> >> +#define MAX_CPU_NUMBER 1024 > > > Is this a good number? It feels large for a normal case. > Then let's change it to 128, Freescale already has 24-core as far as I konw. >> >> + >> typedef struct { >> - uint64_t cpu_hz; >> + uint64_t cpu_hz[MAX_CPU_NUMBER]; >> uint64_t huge_page_size; >> uint64_t page_size; >> int cache_line_size; >> int cpu_count; >> - char model_str[128]; >> + char model_str[MAX_CPU_NUMBER][128]; >> } odp_system_info_t; >> >> struct odp_global_data_s { >> diff --git a/platform/linux-generic/odp_system_info.c >> b/platform/linux-generic/odp_system_info.c >> index 31df29e..c9f6be1 100644 >> --- a/platform/linux-generic/odp_system_info.c >> +++ b/platform/linux-generic/odp_system_info.c >> @@ -36,7 +36,6 @@ typedef struct { >> >> #define HUGE_PAGE_DIR "/sys/kernel/mm/hugepages" >> >> - >> /* >> * Report the number of CPUs in the affinity mask of the main thread >> */ >> @@ -139,17 +138,17 @@ static int cpuinfo_x86(FILE *file, odp_system_info_t >> *sysinfo) >> if (pos) { >> int len; >> pos = strchr(str, ':'); >> - strncpy(sysinfo->model_str, pos+2, >> - sizeof(sysinfo->model_str)); >> - len = strlen(sysinfo->model_str); >> - sysinfo->model_str[len - 1] = 0; >> + strncpy(sysinfo->model_str[0], pos + 2, >> + sizeof(sysinfo->model_str[0])); >> + len = strlen(sysinfo->model_str[0]); >> + sysinfo->model_str[0][len - 1] = 0; >> model = 1; >> count--; >> } >> } >> } >> >> - sysinfo->cpu_hz = (uint64_t) (mhz * 1000000.0); >> + sysinfo->cpu_hz[0] = (uint64_t)(mhz * 1000000.0); >> >> return 0; >> } >> @@ -188,10 +187,10 @@ static int cpuinfo_octeon(FILE *file, >> odp_system_info_t *sysinfo) >> if (pos) { >> int len; >> pos = strchr(str, ':'); >> - strncpy(sysinfo->model_str, pos+2, >> - sizeof(sysinfo->model_str)); >> - len = strlen(sysinfo->model_str); >> - sysinfo->model_str[len - 1] = 0; >> + strncpy(sysinfo->model_str[0], pos + 2, >> + sizeof(sysinfo->model_str[0])); >> + len = strlen(sysinfo->model_str[0]); >> + sysinfo->model_str[0][len - 1] = 0; >> model = 1; >> count--; >> } >> @@ -199,7 +198,7 @@ static int cpuinfo_octeon(FILE *file, >> odp_system_info_t *sysinfo) >> } >> >> /* bogomips seems to be 2x freq */ >> - sysinfo->cpu_hz = (uint64_t) (mhz * 1000000.0 / 2.0); >> + sysinfo->cpu_hz[0] = (uint64_t)(mhz * 1000000.0 / 2.0); >> >> return 0; >> } >> @@ -228,16 +227,16 @@ static int cpuinfo_powerpc(FILE *file, >> odp_system_info_t *sysinfo) >> if (pos) { >> int len; >> pos = strchr(str, ':'); >> - strncpy(sysinfo->model_str, pos+2, >> - sizeof(sysinfo->model_str)); >> - len = strlen(sysinfo->model_str); >> - sysinfo->model_str[len - 1] = 0; >> + strncpy(sysinfo->model_str[0], pos + 2, >> + sizeof(sysinfo->model_str[0])); >> + len = strlen(sysinfo->model_str[0]); >> + sysinfo->model_str[0][len - 1] = 0; >> model = 1; >> count--; >> } >> } >> >> - sysinfo->cpu_hz = (uint64_t) (mhz * 1000000.0); >> + sysinfo->cpu_hz[0] = (uint64_t)(mhz * 1000000.0); >> } >> >> >> @@ -330,10 +329,10 @@ static int systemcpu(odp_system_info_t *sysinfo) >> sysinfo->huge_page_size = huge_page_size(); >> >> /* Dummy values */ >> - sysinfo->cpu_hz = 1400000000; >> + sysinfo->cpu_hz[0] = 1400000000; > > > The "=" no longer line up > >> >> sysinfo->cache_line_size = 64; >> >> - strncpy(sysinfo->model_str, "UNKNOWN", >> sizeof(sysinfo->model_str)); >> + strncpy(sysinfo->model_str[0], "UNKNOWN", >> sizeof(sysinfo->model_str)); >> >> return 0; >> } >> @@ -376,7 +375,15 @@ int odp_system_info_init(void) >> */ >> uint64_t odp_sys_cpu_hz(void) >> { >> - return odp_global_data.system_info.cpu_hz; >> + return odp_global_data.system_info.cpu_hz[0]; >> +} >> + > > > odp_sys_cpu_hz should call odp_sys_cpu_hz_amp(0) as its implementation so > that there is no possibility of forgetting to modify something when updating > the structures. E.g. the specific case calls the generic case with a > specific argument. > Good idea. >> >> +uint64_t odp_sys_cpu_hz_amp(int cpu) >> +{ >> + if (cpu >= 0 && cpu < MAX_CPU_NUMBER) >> + return odp_global_data.system_info.cpu_hz[cpu]; >> + else >> + return -1; >> } >> >> uint64_t odp_sys_huge_page_size(void) >> @@ -391,7 +398,15 @@ uint64_t odp_sys_page_size(void) >> >> const char *odp_sys_cpu_model_str(void) >> { >> - return odp_global_data.system_info.model_str; >> + return odp_global_data.system_info.model_str[0]; >> +} >> + > > > the specific case should call the generic case with a specific argument. > >> >> +const char *odp_sys_cpu_model_str_amp(int cpu) >> +{ >> + if (cpu >= 0 && cpu < MAX_CPU_NUMBER) >> + return odp_global_data.system_info.model_str[cpu]; >> + else >> + return NULL; >> } >> >> int odp_sys_cache_line_size(void) >> -- >> 1.9.1 >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> https://lists.linaro.org/mailman/listinfo/lng-odp > > > > > -- > Mike Holmes > Technical Manager - Linaro Networking Group > Linaro.org │ Open source software for ARM SoCs > > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
diff --git a/include/odp/api/system_info.h b/include/odp/api/system_info.h index 1f3294b..55a4180 100644 --- a/include/odp/api/system_info.h +++ b/include/odp/api/system_info.h @@ -30,6 +30,8 @@ extern "C" { */ uint64_t odp_sys_cpu_hz(void); +uint64_t odp_sys_cpu_hz_amp(int cpu); + /** * Huge page size in bytes * @@ -51,6 +53,8 @@ uint64_t odp_sys_page_size(void); */ const char *odp_sys_cpu_model_str(void); +const char *odp_sys_cpu_model_str_amp(int cpu); + /** * Cache line size in bytes * diff --git a/platform/linux-generic/include/odp_internal.h b/platform/linux-generic/include/odp_internal.h index 8c5d339..7388128 100644 --- a/platform/linux-generic/include/odp_internal.h +++ b/platform/linux-generic/include/odp_internal.h @@ -22,13 +22,15 @@ extern "C" { extern __thread int __odp_errno; +#define MAX_CPU_NUMBER 1024 + typedef struct { - uint64_t cpu_hz; + uint64_t cpu_hz[MAX_CPU_NUMBER]; uint64_t huge_page_size; uint64_t page_size; int cache_line_size; int cpu_count; - char model_str[128]; + char model_str[MAX_CPU_NUMBER][128]; } odp_system_info_t; struct odp_global_data_s { diff --git a/platform/linux-generic/odp_system_info.c b/platform/linux-generic/odp_system_info.c index 31df29e..c9f6be1 100644 --- a/platform/linux-generic/odp_system_info.c +++ b/platform/linux-generic/odp_system_info.c @@ -36,7 +36,6 @@ typedef struct { #define HUGE_PAGE_DIR "/sys/kernel/mm/hugepages" - /* * Report the number of CPUs in the affinity mask of the main thread */ @@ -139,17 +138,17 @@ static int cpuinfo_x86(FILE *file, odp_system_info_t *sysinfo) if (pos) { int len; pos = strchr(str, ':'); - strncpy(sysinfo->model_str, pos+2, - sizeof(sysinfo->model_str)); - len = strlen(sysinfo->model_str); - sysinfo->model_str[len - 1] = 0; + strncpy(sysinfo->model_str[0], pos + 2, + sizeof(sysinfo->model_str[0])); + len = strlen(sysinfo->model_str[0]); + sysinfo->model_str[0][len - 1] = 0; model = 1; count--; } } } - sysinfo->cpu_hz = (uint64_t) (mhz * 1000000.0); + sysinfo->cpu_hz[0] = (uint64_t)(mhz * 1000000.0); return 0; } @@ -188,10 +187,10 @@ static int cpuinfo_octeon(FILE *file, odp_system_info_t *sysinfo) if (pos) { int len; pos = strchr(str, ':'); - strncpy(sysinfo->model_str, pos+2, - sizeof(sysinfo->model_str)); - len = strlen(sysinfo->model_str); - sysinfo->model_str[len - 1] = 0; + strncpy(sysinfo->model_str[0], pos + 2, + sizeof(sysinfo->model_str[0])); + len = strlen(sysinfo->model_str[0]); + sysinfo->model_str[0][len - 1] = 0; model = 1; count--; } @@ -199,7 +198,7 @@ static int cpuinfo_octeon(FILE *file, odp_system_info_t *sysinfo) } /* bogomips seems to be 2x freq */ - sysinfo->cpu_hz = (uint64_t) (mhz * 1000000.0 / 2.0); + sysinfo->cpu_hz[0] = (uint64_t)(mhz * 1000000.0 / 2.0); return 0; } @@ -228,16 +227,16 @@ static int cpuinfo_powerpc(FILE *file, odp_system_info_t *sysinfo) if (pos) { int len; pos = strchr(str, ':'); - strncpy(sysinfo->model_str, pos+2, - sizeof(sysinfo->model_str)); - len = strlen(sysinfo->model_str); - sysinfo->model_str[len - 1] = 0; + strncpy(sysinfo->model_str[0], pos + 2, + sizeof(sysinfo->model_str[0])); + len = strlen(sysinfo->model_str[0]); + sysinfo->model_str[0][len - 1] = 0; model = 1; count--; } } - sysinfo->cpu_hz = (uint64_t) (mhz * 1000000.0); + sysinfo->cpu_hz[0] = (uint64_t)(mhz * 1000000.0); } @@ -330,10 +329,10 @@ static int systemcpu(odp_system_info_t *sysinfo) sysinfo->huge_page_size = huge_page_size(); /* Dummy values */ - sysinfo->cpu_hz = 1400000000; + sysinfo->cpu_hz[0] = 1400000000; sysinfo->cache_line_size = 64; - strncpy(sysinfo->model_str, "UNKNOWN", sizeof(sysinfo->model_str)); + strncpy(sysinfo->model_str[0], "UNKNOWN", sizeof(sysinfo->model_str)); return 0; } @@ -376,7 +375,15 @@ int odp_system_info_init(void) */ uint64_t odp_sys_cpu_hz(void) { - return odp_global_data.system_info.cpu_hz; + return odp_global_data.system_info.cpu_hz[0]; +} + +uint64_t odp_sys_cpu_hz_amp(int cpu) +{ + if (cpu >= 0 && cpu < MAX_CPU_NUMBER) + return odp_global_data.system_info.cpu_hz[cpu]; + else + return -1; } uint64_t odp_sys_huge_page_size(void) @@ -391,7 +398,15 @@ uint64_t odp_sys_page_size(void) const char *odp_sys_cpu_model_str(void) { - return odp_global_data.system_info.model_str; + return odp_global_data.system_info.model_str[0]; +} + +const char *odp_sys_cpu_model_str_amp(int cpu) +{ + if (cpu >= 0 && cpu < MAX_CPU_NUMBER) + return odp_global_data.system_info.model_str[cpu]; + else + return NULL; } int odp_sys_cache_line_size(void)