| Message ID | 564C7448.9020601@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Wed, Nov 18, 2015 at 6:51 AM, Zoltan Kiss <zoltan.kiss@linaro.org> wrote: > > > On 18/11/15 12:05, Bill Fischofer wrote: > >> >> >> On Wed, Nov 18, 2015 at 4:58 AM, Zoltan Kiss <zoltan.kiss@linaro.org >> <mailto:zoltan.kiss@linaro.org>> wrote: >> >> >> On 17/11/15 20:46, Bill Fischofer wrote: >> >> I vote for B. The idea is that if the application wishes to >> override it >> can do so otherwise let the implementation take its defaults >> from the >> environment or however else it wishes to support platform-specific >> >> >> I think your analogy fails a bit here because not passing >> odp_platform_init_t doesn't mean you are using "defaults". It just >> means you are using an another way to provide the configuration. >> >> >> That "other way" by definition is a platform-specific default mechanism, >> which might be an environment variable or something else entirely. The >> point is either the application has an application-centric preference or >> it does not. Option B handles those options simply and cleanly. >> > > I see. If you phrase it like this, it explains why we want to have two > ways to do the same things, I like that. My original thought was to > explicitly define ODP_PLATFORM_PARAMS, but it's better to leave it out. > I would phrase option B then like this: > > --- a/include/odp/api/init.h > +++ b/include/odp/api/init.h > @@ -152,6 +152,10 @@ typedef struct odp_platform_init_t { > * > * @see odp_term_global() > * @see odp_init_local() which is required per thread before use. > + * @note The underlying implementation may have an another way to get > typo: an another. Otherwise I like the note. > + * configuration related to platform_params (e.g. environmental variable, > + * configuration file), but if the application passes it, it should always > + * take precedence. > */ > int odp_init_global(const odp_init_t *params, > const odp_platform_init_t *platform_params); > > > > >> >> configuration options. This is in keeping with how we handle >> other >> overridable defaults for things like log and abort functions. >> Specify >> NULL == accept whatever default behavior is in effect, otherwise >> specify >> what you want. Simpler for everyone. >> >> On Tue, Nov 17, 2015 at 1:05 PM, Mike Holmes >> <mike.holmes@linaro.org <mailto:mike.holmes@linaro.org> >> <mailto:mike.holmes@linaro.org <mailto:mike.holmes@linaro.org>>> >> wrote: >> >> >> >> On 17 November 2015 at 13:54, Zoltan Kiss >> <zoltan.kiss@linaro.org <mailto:zoltan.kiss@linaro.org> >> <mailto:zoltan.kiss@linaro.org >> >> <mailto:zoltan.kiss@linaro.org>>> wrote: >> >> Hi all, >> >> Our odp_init_global() has a second parameter with type >> odp_platform_init_t. It's supposed to convey platform >> specific >> init parameters, however neither linux-generic nor >> linux-dpdk >> uses that. In the latter we use instead the >> ODP_PLATFORM_PARAMS >> environment variable. It has the little advantage that >> you don't >> have to modify your application's code, but it limits >> you to >> strings. In case of ODP-OVS we store this in OVSDB and >> retrieve >> it from the startup script (or specify it manually if >> you don't >> use the startup script.) >> I'm tempted to change ODP-OVS and ODP-DPDK to use >> odp_platform_init_t, would be cleaner for OVS and for >> any bigger >> application which have a nice, full-fledged config >> database >> solution. But that would immediately bring us a bigger >> problem: >> none of our unit tests or example applications supports >> passing >> this parameter at all. They are small applications, >> implementing >> a proper config management would be an overkill. I have >> two >> options to solve this: >> >> Apart from keeping the odp_platform_init_t type to be >> passed >> odp_init_global() >> >> A) change our code in linux-generic for examples and >> tests (>20 >> places in the repo) to get the platform parameters from >> ODP_PLATFORM_PARAMS env variable, and pass it down to >> odp_init_global() as a string. >> >> B) Or just allow a platform to use both ways, document >> this, and >> require that if both are present, the >> odp_platform_init_t passed >> as parameter should take precedence. So smaller >> applications >> using simply configurable platforms (like ODP-DPDK) >> don't have >> to figure out a way to deal with this problem. >> >> I have a slight preference towards B), but I could be >> convinced >> that it's a bad idea to have 2 ways to do the same thing. >> >> >> I like A, two ways always feels bad to me. >> I like that it is explicitly passed in and you know what >> you have. >> Env vars can change without the difference being seen in >> the code as >> easily and not all OS'es have env vars. Env vars are a >> great way to >> do system dependent things but I think the application should >> translate them into the odp_platform_init_t so that the >> fudging is >> the apps problem and the app knows more about the portability >> compromises it is making when using platform specifics. >> >> >> Thoughts? >> >> Zoltan >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> <mailto:lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org >> >> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >> >> -- >> Mike Holmes >> Technical Manager - Linaro Networking Group >> Linaro.org <http://www.linaro.org/>***│ *Open source >> software for >> ARM SoCs >> >> __ >> >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> <mailto:lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org >> >> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >>
On 18 November 2015 at 08:08, Savolainen, Petri (Nokia - FI/Espoo) < petri.savolainen@nokia.com> wrote: > > > > --- a/include/odp/api/init.h > > +++ b/include/odp/api/init.h > > @@ -152,6 +152,10 @@ typedef struct odp_platform_init_t { > > * > > * @see odp_term_global() > > * @see odp_init_local() which is required per thread before use. > > + * @note The underlying implementation may have an another way to get > > + * configuration related to platform_params (e.g. environmental > variable, > > + * configuration file), but if the application passes it, it should > > always > > + * take precedence. > > */ > > int odp_init_global(const odp_init_t *params, > > const odp_platform_init_t *platform_params); > > > > Otherwise OK, but add it into the body of the documentation text (instead > of @note). > I like this but I'd like a clarification. If a platform may have another way, implicitly we saying platform must always accept platform_params for all the items that may have been passed another way. > > -Petri > _______________________________________________ > 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 <http://www.linaro.org/> *│ *Open source software for ARM SoCs
--- a/include/odp/api/init.h +++ b/include/odp/api/init.h @@ -152,6 +152,10 @@ typedef struct odp_platform_init_t { * * @see odp_term_global() * @see odp_init_local() which is required per thread before use. + * @note The underlying implementation may have an another way to get + * configuration related to platform_params (e.g. environmental variable, + * configuration file), but if the application passes it, it should always + * take precedence. */ int odp_init_global(const odp_init_t *params, const odp_platform_init_t *platform_params);