| Message ID | 1417029652-31887-1-git-send-email-mike.holmes@linaro.org |
|---|---|
| State | Rejected |
| Headers | show |
I disprove of this patch. On 26 November 2014 at 20:20, Mike Holmes <mike.holmes@linaro.org> wrote: > Improve the documentation regarding possible return values. > > Signed-off-by: Mike Holmes <mike.holmes@linaro.org> > --- > platform/linux-generic/include/api/odp_system_info.h | 19 ++++++++++++++----- > 1 file changed, 14 insertions(+), 5 deletions(-) > > diff --git a/platform/linux-generic/include/api/odp_system_info.h b/platform/linux-generic/include/api/odp_system_info.h > index bcd08d7..28ce669 100644 > --- a/platform/linux-generic/include/api/odp_system_info.h > +++ b/platform/linux-generic/include/api/odp_system_info.h > @@ -28,28 +28,34 @@ extern "C" { > /** > * CPU frequency in Hz > * > - * @return CPU frequency in Hz > + * @retval CPU frequency in Hz Normally called "CPU clock frequency". Is this current clock frequency or max (peak) frequency (for "turbo" mode) or max steady state frequency? As ODP applications normally busy wait, the kernel DVFS (voltage and frequency scaling) support will likely rack up the clock frequency (the threads never block so the system must be really busy, better to try to complete processing as quickly as possible). But at application start when this function is called, the clock frequency might still be rather low. Power management and frequency scaling is something we need to look closer at in the future. The current support in Linux is probably not designed for applications that busy wait all the time and never complete. The kernel has no understanding of the load and thus performance requirements of the ODP applications. > + * @retval 0 on error or we failed to detect the CPU frequency > */ > uint64_t odp_sys_cpu_hz(void); > > /** > * Huge page size in bytes > * > - * @return Huge page size in bytes > + * @retval Huge page size in bytes > + * @retval 0 on error > */ > uint64_t odp_sys_huge_page_size(void); > > /** > * Page size in bytes > * > - * @return Page size in bytes > + * This is set at compile time via ODP_PAGE_SIZE > + * @retval Page size in bytes > + * > */ > uint64_t odp_sys_page_size(void); > > /** > * CPU model name > * > - * @return Pointer to CPU model name string > + * @note max size is 127 chars + null termination > + * @retval Pointer to CPU model name string > + * @retval null teminated string on failure > */ > const char *odp_sys_cpu_model_str(void); > > @@ -63,7 +69,10 @@ int odp_sys_cache_line_size(void); > /** > * Core count > * > - * @return Core count > + * @note The total physical number of cores, the current number avalable might be less. I think this function should rather return the number of available cores, not the total number of cores in the system (as handled by the kernel). Not all cores might be available to the ODP application (for different reasons, e.g. not online or the application has been bound to a certain subset of cores). It's meaningless for the ODP application to "know" about resources that it cannot (and should not) use. > + * > + * @retval Core count > + * @retval 0 on error > */ > int odp_sys_core_count(void); > > -- > 2.1.0 > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp
On 26 November 2014 at 15:23, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > I disprove of this patch. > > On 26 November 2014 at 20:20, Mike Holmes <mike.holmes@linaro.org> wrote: > > Improve the documentation regarding possible return values. > > > > Signed-off-by: Mike Holmes <mike.holmes@linaro.org> > > --- > > platform/linux-generic/include/api/odp_system_info.h | 19 > ++++++++++++++----- > > 1 file changed, 14 insertions(+), 5 deletions(-) > > > > diff --git a/platform/linux-generic/include/api/odp_system_info.h > b/platform/linux-generic/include/api/odp_system_info.h > > index bcd08d7..28ce669 100644 > > --- a/platform/linux-generic/include/api/odp_system_info.h > > +++ b/platform/linux-generic/include/api/odp_system_info.h > > @@ -28,28 +28,34 @@ extern "C" { > > /** > > * CPU frequency in Hz > > * > > - * @return CPU frequency in Hz > > + * @retval CPU frequency in Hz > Normally called "CPU clock frequency". > Is this current clock frequency or max (peak) frequency (for "turbo" > mode) or max steady state frequency? > Linux generic gets the information from /proc/cpuinfo - I assume that is the max frequency, but we need to clarify > As ODP applications normally busy wait, the kernel DVFS (voltage and > frequency scaling) support will likely rack up the clock frequency > (the threads never block so the system must be really busy, better to > try to complete processing as quickly as possible). But at application > start when this function is called, the clock frequency might still be > rather low. > > Power management and frequency scaling is something we need to look > closer at in the future. The current support in Linux is probably not > designed for applications that busy wait all the time and never > complete. The kernel has no understanding of the load and thus > performance requirements of the ODP applications. > > > + * @retval 0 on error > or we failed to detect the CPU frequency > This is populated at init time, it should fail there, if it does not it will be 0 if the speed is not grepped accurately. As above I hope this is max as it is not a dynamic value. > > */ > > uint64_t odp_sys_cpu_hz(void); > > > > /** > > * Huge page size in bytes > > * > > - * @return Huge page size in bytes > > + * @retval Huge page size in bytes > > + * @retval 0 on error > > */ > > uint64_t odp_sys_huge_page_size(void); > > > > /** > > * Page size in bytes > > * > > - * @return Page size in bytes > > + * This is set at compile time via ODP_PAGE_SIZE > > + * @retval Page size in bytes > > + * > > */ > > uint64_t odp_sys_page_size(void); > > > > /** > > * CPU model name > > * > > - * @return Pointer to CPU model name string > > + * @note max size is 127 chars + null termination > > + * @retval Pointer to CPU model name string > > + * @retval null teminated string on failure > > */ > > const char *odp_sys_cpu_model_str(void); > > > > @@ -63,7 +69,10 @@ int odp_sys_cache_line_size(void); > > /** > > * Core count > > * > > - * @return Core count > > + * @note The total physical number of cores, the current number > available might be less. > I think this function should rather return the number of available > cores, not the total number of > cores in the system (as handled by the kernel). Not all cores might be > available to the ODP > application (for different reasons, e.g. not online or the application > has been bound to a certain > subset of cores). It's meaningless for the ODP application to "know" > about resources that it > cannot (and should not) use. > Don't disagree, but that is not how it is currently coded, I think you have a patch floating that will fix the code and it can amend the docs too. > > > + * > > + * @retval Core count > > + * @retval 0 on error > > */ > > int odp_sys_core_count(void); > > > > -- > > 2.1.0 > > > > > > _______________________________________________ > > lng-odp mailing list > > lng-odp@lists.linaro.org > > http://lists.linaro.org/mailman/listinfo/lng-odp >
diff --git a/platform/linux-generic/include/api/odp_system_info.h b/platform/linux-generic/include/api/odp_system_info.h index bcd08d7..28ce669 100644 --- a/platform/linux-generic/include/api/odp_system_info.h +++ b/platform/linux-generic/include/api/odp_system_info.h @@ -28,28 +28,34 @@ extern "C" { /** * CPU frequency in Hz * - * @return CPU frequency in Hz + * @retval CPU frequency in Hz + * @retval 0 on error */ uint64_t odp_sys_cpu_hz(void); /** * Huge page size in bytes * - * @return Huge page size in bytes + * @retval Huge page size in bytes + * @retval 0 on error */ uint64_t odp_sys_huge_page_size(void); /** * Page size in bytes * - * @return Page size in bytes + * This is set at compile time via ODP_PAGE_SIZE + * @retval Page size in bytes + * */ uint64_t odp_sys_page_size(void); /** * CPU model name * - * @return Pointer to CPU model name string + * @note max size is 127 chars + null termination + * @retval Pointer to CPU model name string + * @retval null teminated string on failure */ const char *odp_sys_cpu_model_str(void); @@ -63,7 +69,10 @@ int odp_sys_cache_line_size(void); /** * Core count * - * @return Core count + * @note The total physical number of cores, the current number avalable might be less. + * + * @retval Core count + * @retval 0 on error */ int odp_sys_core_count(void);
Improve the documentation regarding possible return values. Signed-off-by: Mike Holmes <mike.holmes@linaro.org> --- platform/linux-generic/include/api/odp_system_info.h | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)