[API-NEXT] api: init: allow implementation to use private ways for its own configuration

This could help the existing configuration methods to be used if the
application prefers that. The platform_params should always supersede that
though.

Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org>

Could anyone review this? Petri?

On 18/11/15 16:22, Zoltan Kiss wrote:
> This could help the existing configuration methods to be used if the
> application prefers that. The platform_params should always supersede that
> though.
>
> Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org>
>
> diff --git a/include/odp/api/init.h b/include/odp/api/init.h
> index 737ff6d..24f4f3a 100644
> --- a/include/odp/api/init.h
> +++ b/include/odp/api/init.h
> @@ -141,6 +141,9 @@ typedef struct odp_platform_init_t {
>    *
>    * This function must be called once before calling any other ODP API
>    * functions.
> + * The underlying implementation may have another way to get configuration
> + * related to platform_params (e.g. environmental variable, configuration
> + * file), but if the application passes it, it should always supersede.
>    *
>    * @param params          Those parameters that are interpreted by the ODP API.
>    *                        Use NULL to set all parameters to their defaults.
>

On 26/11/15 09:25, Savolainen, Petri (Nokia - FI/Espoo) wrote:
>
>
>> -----Original Message-----
>> From: EXT Zoltan Kiss [mailto:zoltan.kiss@linaro.org]
>> Sent: Tuesday, November 24, 2015 4:45 PM
>> To: lng-odp@lists.linaro.org
>> Cc: bill.fischofer@linaro.org; Savolainen, Petri (Nokia - FI/Espoo);
>> mike.holmes@linaro.org
>> Subject: Re: [API-NEXT PATCH] api: init: allow implementation to use
>> private ways for its own configuration
>>
>> Could anyone review this? Petri?
>>
>> On 18/11/15 16:22, Zoltan Kiss wrote:
>>> This could help the existing configuration methods to be used if the
>>> application prefers that. The platform_params should always supersede
>> that
>>> though.
>>>
>>> Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org>
>>>
>>> diff --git a/include/odp/api/init.h b/include/odp/api/init.h
>>> index 737ff6d..24f4f3a 100644
>>> --- a/include/odp/api/init.h
>>> +++ b/include/odp/api/init.h
>>> @@ -141,6 +141,9 @@ typedef struct odp_platform_init_t {
>>>     *
>>>     * This function must be called once before calling any other ODP API
>>>     * functions.
>>> + * The underlying implementation may have another way to get
>> configuration
>>> + * related to platform_params (e.g. environmental variable,
>> configuration
>>> + * file), but if the application passes it, it should always supersede.
>>>     *
>>>     * @param params          Those parameters that are interpreted by the
>> ODP API.
>>>     *                        Use NULL to set all parameters to their
>> defaults.
>>>
>
> Which configuration should supersede which?

"but if the application passes it, it should always supersede."

As this is in the comment of odp_platform_init_t, sed 
s/"it"/"odp_platform_init_t"/g


> Isn't it that way around that config files and env params are usually used to override the hard coded configuration. So, you'd build your application with a default config, but would use extra methods to override the hard coded default config. If hard coded config all ways overrides, you cannot try other configs without rebuilding (which may not be even possible if you just have received the app binary).

Yes, but noone talks about hardcoded configuration here. Every sensible 
application should have a sensible way to get configured, including ODP 
platform parameters of the user's choice to be passed to the actual 
platform. Of course the application can choose to detect the platform 
runtime and apply a default if nothing else configured to it, but it 
would be still more relevant than some platform defaults.

>
> It may vary per parameter and application, which parameters are possible to override (without application 100% controlling it).

I'm not sure I understand that. A parameter is something which is 
configurable, by definition. In other words, it's possible to override 
it. "always supersede" implies to me that, but you can suggest a better 
phrasing.

> Can we say anything else than platform configuration is platform specific
Even platform config is platform specific, it also seems obvious to me.

> and may include usage of  platform_params ?
My aim here is that if the app provide platform_params, then the 
platform MUST use that instead of other possible configuration ways (if 
any). So the application can enforce (if it wants) config, which is 
probably coming from the user through some way of app configuration.

>
> -Petri

On 30/11/15 12:02, Savolainen, Petri (Nokia - FI/Espoo) wrote:
>
>
>> -----Original Message-----
>> From: EXT Zoltan Kiss [mailto:zoltan.kiss@linaro.org]
>> Sent: Thursday, November 26, 2015 4:35 PM
>> To: Savolainen, Petri (Nokia - FI/Espoo); lng-odp@lists.linaro.org
>> Cc: bill.fischofer@linaro.org; mike.holmes@linaro.org
>> Subject: Re: [API-NEXT PATCH] api: init: allow implementation to use
>> private ways for its own configuration
>>
>>
>>
>> On 26/11/15 09:25, Savolainen, Petri (Nokia - FI/Espoo) wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: EXT Zoltan Kiss [mailto:zoltan.kiss@linaro.org]
>>>> Sent: Tuesday, November 24, 2015 4:45 PM
>>>> To: lng-odp@lists.linaro.org
>>>> Cc: bill.fischofer@linaro.org; Savolainen, Petri (Nokia - FI/Espoo);
>>>> mike.holmes@linaro.org
>>>> Subject: Re: [API-NEXT PATCH] api: init: allow implementation to use
>>>> private ways for its own configuration
>>>>
>>>> Could anyone review this? Petri?
>>>>
>>>> On 18/11/15 16:22, Zoltan Kiss wrote:
>>>>> This could help the existing configuration methods to be used if the
>>>>> application prefers that. The platform_params should always supersede
>>>> that
>>>>> though.
>>>>>
>>>>> Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org>
>>>>>
>>>>> diff --git a/include/odp/api/init.h b/include/odp/api/init.h
>>>>> index 737ff6d..24f4f3a 100644
>>>>> --- a/include/odp/api/init.h
>>>>> +++ b/include/odp/api/init.h
>>>>> @@ -141,6 +141,9 @@ typedef struct odp_platform_init_t {
>>>>>      *
>>>>>      * This function must be called once before calling any other ODP
>> API
>>>>>      * functions.
>>>>> + * The underlying implementation may have another way to get
>>>> configuration
>>>>> + * related to platform_params (e.g. environmental variable,
>>>> configuration
>>>>> + * file), but if the application passes it, it should always
>> supersede.
>>>>>      *
>>>>>      * @param params          Those parameters that are interpreted by
>> the
>>>> ODP API.
>>>>>      *                        Use NULL to set all parameters to their
>>>> defaults.
>>>>>
>>>
>>> Which configuration should supersede which?
>>
>> "but if the application passes it, it should always supersede."
>>
>> As this is in the comment of odp_platform_init_t, sed
>> s/"it"/"odp_platform_init_t"/g
>
>
> + * The underlying implementation may have another way to get configuration
> + * related to platform_params (e.g. environmental variable, configuration
> + * file), but if the application passes it, it should always supersede.
>
> It's not clear from this sentence if application:
> - passes "another way" (e.g. config file) which supersedes "platform_params" ,or
> - passes "platform_params" which supersedes "another way"
>
> Both can be understood as "it".

Ok, I'll rephrase it as "but if the application passes platform_params"

>
>
>>
>>
>>> Isn't it that way around that config files and env params are usually
>> used to override the hard coded configuration. So, you'd build your
>> application with a default config, but would use extra methods to override
>> the hard coded default config. If hard coded config all ways overrides,
>> you cannot try other configs without rebuilding (which may not be even
>> possible if you just have received the app binary).
>>
>> Yes, but noone talks about hardcoded configuration here. Every sensible
>> application should have a sensible way to get configured, including ODP
>> platform parameters of the user's choice to be passed to the actual
>> platform. Of course the application can choose to detect the platform
>> runtime and apply a default if nothing else configured to it, but it
>> would be still more relevant than some platform defaults.
>
> For example, if an (3rd party) application has been built before a new (implementation specific) parameter was introduced, the app would init all params to defaults (with odp_xxx_params_init()) and set those that it cares about. Any new params would be in defaults unless you rebuild the app, which may not be always possible.

This is an example of a badly written application. As I said above, 
platform_params shouldn't be used for passing hardcoded default parameters.
I can add a sentence to remind people about having a sensible way of 
configuration, and not trying to figure out the impossible at the time 
of development.

>
> Another example, optimal configuration is not known at build time. The system is composed from multiple ODP apps, any single developer does not know which ODP configuration is optimal or even possible when apps are integrated together.

Yes, that's why your app should have a way to get its configuration. 
E.g. command line options, a config file. OVS has OVSDB, at the moment 
you can also store a string called odp_platform_params there, which will 
be passed to the platform in platform_params.
If there is a platform which needs something else than a string, then 
OVS can provide a way for that. In any case, that's the implementation's 
internal detail, the application may or may not know how to handle that, 
and how to handle if it changes.
But it's not ODP's business, that's why we are providing only an opaque 
type here. This patch just tries to encourage apps and platforms to pass 
it here, because IF the app is able to configure the platform (e.g. OVS 
at the moment is able to pass a string given by the user), then it's 
better to pass the config here than forcing the user to use another way 
(e.g. OVS used a gross hack in the init script to get the config from 
OVSDB and set the env var)

>
>
>>
>>>
>>> It may vary per parameter and application, which parameters are possible
>> to override (without application 100% controlling it).
>>
>> I'm not sure I understand that. A parameter is something which is
>> configurable, by definition. In other words, it's possible to override
>> it. "always supersede" implies to me that, but you can suggest a better
>> phrasing.
>
> For example, maximum number of CPUs may be hard coded in an app,

I think that's the applications trouble, it has to handle that.

> but any CPU MHz goes.
>
> int my_cpu[32];
>
> // crash if plat_config.max_cpu > 32
> my_cpu[odp_cpu_id()].foo = 1;
>
> // any value is OK
> printf("CPU MHz %u", odp_cpu_mhz());
>
>
> // Fill defaults (from config file)
> // max_cpu = 64, max_mhz = 1000 (on this SoC)
> plat_xyz_config_init_params(&plat_config);

Again, I strongly oppose this way. It doesn't make any sense. Any decent 
applications should get the platform parameters through it's own config 
from the user, and not try to hardcode these things.

> plat_config.max_cpu = 32;
> plat_config.max_mhz = 500; // 500MHz was max on the old SoC, could be overridden with some plat specific way
>
>
>>
>>> Can we say anything else than platform configuration is platform
>> specific
>> Even platform config is platform specific, it also seems obvious to me.
>>
>>> and may include usage of  platform_params ?
>> My aim here is that if the app provide platform_params, then the
>> platform MUST use that instead of other possible configuration ways (if
>> any). So the application can enforce (if it wants) config, which is
>> probably coming from the user through some way of app configuration.
>
> I think the key here is that the plat_xyz_config_init_params(&plat_config) reads the config file, so that those things application does not set in platform_paras, can be still modified through a config file. But currently  plat_xyz_config_init_params() is platform specific (may not even exist).
>
> -Petri
>
>>
>>>
>>> -Petri

On 01/12/15 08:38, Savolainen, Petri (Nokia - FI/Espoo) wrote:
>>>
>>> // Fill defaults (from config file)
>>> // max_cpu = 64, max_mhz = 1000 (on this SoC)
>>> plat_xyz_config_init_params(&plat_config);
>>
>> Again, I strongly oppose this way. It doesn't make any sense. Any decent
>> applications should get the platform parameters through it's own config
>> from the user, and not try to hardcode these things.
>
> How of those application parameters would need to be updated?
By the user, when configuring through the app's configuration interface. 
Not our concern how that happens.

> Every time any of its target platforms change or add a parameter? Platforms may have many tuning parameters, which the application is *not interested* - default is fine, now and in the future.
Yes, e.g. DPDK has a lot of parameters, but it only mandates -n and -c 
(number of memory channels and cpu mask), and the latter is actually 
calculated by ODP-DPDK, so our apps need to specify -n only.

>
> Now when application wants to change only one param trough plat_params, how it sets all other (100) parameters in the struct?

It really depends on platforms (that's why it's platform dependent), but 
struct is definitely not the best solution for that. Passing a string, 
or a linked list of struct, if you want something more elegant. That's 
really not our concern again, but generic application/API interfacing 
questions.
I think you're concerns are mostly rooted on the thought that 
platform_params should be a struct. It's not.

>
> -Petri
>
>
>>
>>> plat_config.max_cpu = 32;
>>> plat_config.max_mhz = 500; // 500MHz was max on the old SoC, could be
>> overridden with some plat specific way

Hi Petri,

Did you have time to think on this? Mike added it to tomorrow's arch 
call to discuss, unless you are happy with what I've proposed.

Zoli

On 01/12/15 11:30, Zoltan Kiss wrote:
>
>
> On 01/12/15 08:38, Savolainen, Petri (Nokia - FI/Espoo) wrote:
>>>>
>>>> // Fill defaults (from config file)
>>>> // max_cpu = 64, max_mhz = 1000 (on this SoC)
>>>> plat_xyz_config_init_params(&plat_config);
>>>
>>> Again, I strongly oppose this way. It doesn't make any sense. Any 
>>> decent
>>> applications should get the platform parameters through it's own config
>>> from the user, and not try to hardcode these things.
>>
>> How of those application parameters would need to be updated?
> By the user, when configuring through the app's configuration 
> interface. Not our concern how that happens.
>
>> Every time any of its target platforms change or add a parameter? 
>> Platforms may have many tuning parameters, which the application is 
>> *not interested* - default is fine, now and in the future.
> Yes, e.g. DPDK has a lot of parameters, but it only mandates -n and -c 
> (number of memory channels and cpu mask), and the latter is actually 
> calculated by ODP-DPDK, so our apps need to specify -n only.
>
>>
>> Now when application wants to change only one param trough 
>> plat_params, how it sets all other (100) parameters in the struct?
>
> It really depends on platforms (that's why it's platform dependent), 
> but struct is definitely not the best solution for that. Passing a 
> string, or a linked list of struct, if you want something more 
> elegant. That's really not our concern again, but generic 
> application/API interfacing questions.
> I think you're concerns are mostly rooted on the thought that 
> platform_params should be a struct. It's not.
>
>>
>> -Petri
>>
>>
>>>
>>>> plat_config.max_cpu = 32;
>>>> plat_config.max_mhz = 500; // 500MHz was max on the old SoC, could be
>>> overridden with some plat specific way