| Message ID | 1418248839-47292-1-git-send-email-mike.holmes@linaro.org |
|---|---|
| State | Not Applicable |
| Headers | show |
On 11 December 2014 at 07:13, Savolainen, Petri (NSN - FI/Espoo) < petri.savolainen@nsn.com> wrote: > > > -----Original Message----- > > From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp- > > bounces@lists.linaro.org] On Behalf Of ext Mike Holmes > > Sent: Thursday, December 11, 2014 12:01 AM > > To: lng-odp@lists.linaro.org > > Subject: [lng-odp] [PATCH] api: doxygen: test_odp_sys_cpu_model_str > > > > Specify more clearly the behaviour of the function. > > > > Signed-off-by: Mike Holmes <mike.holmes@linaro.org> > > --- > > platform/linux-generic/include/api/odp_system_info.h | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/platform/linux-generic/include/api/odp_system_info.h > > b/platform/linux-generic/include/api/odp_system_info.h > > index bcd08d7..bf5896c 100644 > > --- a/platform/linux-generic/include/api/odp_system_info.h > > +++ b/platform/linux-generic/include/api/odp_system_info.h > > @@ -48,6 +48,8 @@ uint64_t odp_sys_page_size(void); > > > > /** > > * CPU model name > > + * The string is 128 chars long and will be NULL terminatied. > > + * The pointer will never be NULL, but it may be an empty string. > > * > > * @return Pointer to CPU model name string > > */ > > > While 128 chars would be reasonable limit, I think we need not restrict > the length. Application can copy or print only subset of that (into a log > entry, etc). I'd change it like this: > For 1.0 ? If so there are a lot more details we need to look at in system_into besides this I think. Is it better to document the reality of what we have now and make changes later? Possibly better still delete the system_info APIs altogether for 1.0 because they have some strong assumptions that are not clear in the API definition and ODP should abstract network acceleration functionality and not OS calls - maybe these should be in odph if they are there at all ? Back to this case Are you proposing that if you need to copy the string as you mention above you have to call the function, then get the string length, malloc space and then copy it ? feels like a lot of effort for a string used in debug messages that is not likely to be very long and impacted very much by a limit. Also can we assume the string never changes ? What if you start on CPU 0 and then run on CPU7 on big/little is the string constant ? I don't know big/little behaviour, for some of the other system_into APIs this is more serious like CPU freq. > > > /** > * CPU model name > * > * Returns pointer to a null terminated string describing the CPU > * or SoC model. The pointer will never be NULL, but the string > * may be empty. > * > * @return Pointer to CPU model name string > */ > > > -Petri > > > > > -- > > 2.1.0 > > > > > > _______________________________________________ > > lng-odp mailing list > > lng-odp@lists.linaro.org > > http://lists.linaro.org/mailman/listinfo/lng-odp >
From: ext Mike Holmes [mailto:mike.holmes@linaro.org] Sent: Thursday, December 11, 2014 9:03 PM To: Savolainen, Petri (NSN - FI/Espoo) Cc: lng-odp@lists.linaro.org Subject: Re: [lng-odp] [PATCH] api: doxygen: test_odp_sys_cpu_model_str On 11 December 2014 at 07:13, Savolainen, Petri (NSN - FI/Espoo) <petri.savolainen@nsn.com<mailto:petri.savolainen@nsn.com>> wrote: > -----Original Message----- > From: lng-odp-bounces@lists.linaro.org<mailto:lng-odp-bounces@lists.linaro.org> [mailto:lng-odp-<mailto:lng-odp-> > bounces@lists.linaro.org<mailto:bounces@lists.linaro.org>] On Behalf Of ext Mike Holmes > Sent: Thursday, December 11, 2014 12:01 AM > To: lng-odp@lists.linaro.org<mailto:lng-odp@lists.linaro.org> > Subject: [lng-odp] [PATCH] api: doxygen: test_odp_sys_cpu_model_str > > Specify more clearly the behaviour of the function. > > Signed-off-by: Mike Holmes <mike.holmes@linaro.org<mailto:mike.holmes@linaro.org>> > --- > platform/linux-generic/include/api/odp_system_info.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/platform/linux-generic/include/api/odp_system_info.h > b/platform/linux-generic/include/api/odp_system_info.h > index bcd08d7..bf5896c 100644 > --- a/platform/linux-generic/include/api/odp_system_info.h > +++ b/platform/linux-generic/include/api/odp_system_info.h > @@ -48,6 +48,8 @@ uint64_t odp_sys_page_size(void); > > /** > * CPU model name > + * The string is 128 chars long and will be NULL terminatied. > + * The pointer will never be NULL, but it may be an empty string. > * > * @return Pointer to CPU model name string > */ While 128 chars would be reasonable limit, I think we need not restrict the length. Application can copy or print only subset of that (into a log entry, etc). I'd change it like this: For 1.0 ? If so there are a lot more details we need to look at in system_into besides this I think. Is it better to document the reality of what we have now and make changes later? Possibly better still delete the system_info APIs altogether for 1.0 because they have some strong assumptions that are not clear in the API definition and ODP should abstract network acceleration functionality and not OS calls - maybe these should be in odph if they are there at all ? It’s good information for error/performance test/crash log reporting. Which Soc or cpu model/revision/core speed/memory speed/etc. Some vendors may have good reasons to output 129 chars… Back to this case Are you proposing that if you need to copy the string as you mention above you have to call the function, then get the string length, malloc space and then copy it ? feels like a lot of effort for a string used in debug messages that is not likely to be very long and impacted very much by a limit. For example, if application would not care about the string length it would just … printf(“%s”, odp_sys_cpu_model_str()); and if it would care about the length … char model[MAX_LEN]; snprintf(model, MAX_LEN, “%s”, odp_sys_cpu_model_str()); The first example would print out all available information, the second would clip it if needed. Also can we assume the string never changes ? What if you start on CPU 0 and then run on CPU7 on big/little is the string constant ? I don't know big/little behaviour, for some of the other system_into APIs this is more serious like CPU freq. It depends on the implementation. I wouldn’t change it on the fly. It would just confuse when comparing logs, etc. -Petri /** * CPU model name * * Returns pointer to a null terminated string describing the CPU * or SoC model. The pointer will never be NULL, but the string * may be empty. * * @return Pointer to CPU model name string */ -Petri > -- > 2.1.0 > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org<mailto:lng-odp@lists.linaro.org> > http://lists.linaro.org/mailman/listinfo/lng-odp -- Mike Holmes Linaro Sr Technical Manager 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..bf5896c 100644 --- a/platform/linux-generic/include/api/odp_system_info.h +++ b/platform/linux-generic/include/api/odp_system_info.h @@ -48,6 +48,8 @@ uint64_t odp_sys_page_size(void); /** * CPU model name + * The string is 128 chars long and will be NULL terminatied. + * The pointer will never be NULL, but it may be an empty string. * * @return Pointer to CPU model name string */
Specify more clearly the behaviour of the function. Signed-off-by: Mike Holmes <mike.holmes@linaro.org> --- platform/linux-generic/include/api/odp_system_info.h | 2 ++ 1 file changed, 2 insertions(+)