diff mbox

[v2,09/18] Split out platform-specific part of odp_config.h

Message ID 1403027720-9738-10-git-send-email-taras.kondratiuk@linaro.org
State RFC
Headers show

Commit Message

Taras Kondratiuk June 17, 2014, 5:55 p.m. UTC
Signed-off-by: Taras Kondratiuk <taras.kondratiuk@linaro.org>
---
 include/odp_config.h                             |   11 +++---
 platform/linux-generic/include/plat/odp_config.h |   44 ++++++++++++++++++++++
 2 files changed, 50 insertions(+), 5 deletions(-)
 create mode 100644 platform/linux-generic/include/plat/odp_config.h

Comments

Jerin Jacob June 18, 2014, 9:58 a.m. UTC | #1
On Tue, Jun 17, 2014 at 08:55:11PM +0300, Taras Kondratiuk wrote:
> Signed-off-by: Taras Kondratiuk <taras.kondratiuk@linaro.org>
> ---
>  include/odp_config.h                             |   11 +++---
>  platform/linux-generic/include/plat/odp_config.h |   44 ++++++++++++++++++++++

All the parameters in include/odp_config.h are derived from the include/plat/odp_config.h file.
So instead of keeping additional indirection, can we move the config file to per platform(platform/xxxx/include/odp_config.h)
More over, I think config file should not be part of ODP public header file as each platform will have its on constrains on each config items
and number of config items to build the libodp.{a,so}


>  2 files changed, 50 insertions(+), 5 deletions(-)
>  create mode 100644 platform/linux-generic/include/plat/odp_config.h
> 
> diff --git a/include/odp_config.h b/include/odp_config.h
> index 7a15ff9..e6d0cae 100644
> --- a/include/odp_config.h
> +++ b/include/odp_config.h
> @@ -18,31 +18,32 @@
>  extern "C" {
>  #endif
>  
> +#include <plat/odp_config.h>
>  
>  /**
>   * Maximum number of threads
>   */
> -#define ODP_CONFIG_MAX_THREADS  128
> +#define ODP_CONFIG_MAX_THREADS  PLAT_ODP_CONFIG_MAX_THREADS
>  
>  /**
>   * Maximum number of buffer pools
>   */
> -#define ODP_CONFIG_BUFFER_POOLS 16
> +#define ODP_CONFIG_BUFFER_POOLS PLAT_ODP_CONFIG_BUFFER_POOLS
>  
>  /**
>   * Maximum number of queues
>   */
> -#define ODP_CONFIG_QUEUES       1024
> +#define ODP_CONFIG_QUEUES       PLAT_ODP_CONFIG_QUEUES
>  
>  /**
>   * Number of scheduling priorities
>   */
> -#define ODP_CONFIG_SCHED_PRIOS  8
> +#define ODP_CONFIG_SCHED_PRIOS  PLAT_ODP_CONFIG_SCHED_PRIOS
>  
>  /**
>   * Maximum number of packet IO resources
>   */
> -#define ODP_CONFIG_PKTIO_ENTRIES 64
> +#define ODP_CONFIG_PKTIO_ENTRIES PLAT_ODP_CONFIG_PKTIO_ENTRIES
>  
>  #ifdef __cplusplus
>  }
> diff --git a/platform/linux-generic/include/plat/odp_config.h b/platform/linux-generic/include/plat/odp_config.h
> new file mode 100644
> index 0000000..7230925
> --- /dev/null
> +++ b/platform/linux-generic/include/plat/odp_config.h
> @@ -0,0 +1,44 @@
> +/* Copyright (c) 2013, Linaro Limited
> + * All rights reserved.
> + *
> + * SPDX-License-Identifier:     BSD-3-Clause
> + */
> +
> +
> +/**
> + * @file
> + *
> + * ODP configuration
> + */
> +
> +#ifndef ODP_CONFIG_H_
> +#error This file should be included only into corresponding top level header
> +#else
> +
> +/**
> + * Maximum number of threads
> + */
> +#define PLAT_ODP_CONFIG_MAX_THREADS  128
> +
> +/**
> + * Maximum number of buffer pools
> + */
> +#define PLAT_ODP_CONFIG_BUFFER_POOLS 16
> +
> +/**
> + * Maximum number of queues
> + */
> +#define PLAT_ODP_CONFIG_QUEUES       1024
> +
> +/**
> + * Number of scheduling priorities
> + */
> +#define PLAT_ODP_CONFIG_SCHED_PRIOS  8
> +
> +/**
> + * Maximum number of packet IO resources
> + */
> +#define PLAT_ODP_CONFIG_PKTIO_ENTRIES 64
> +
> +
> +#endif
> -- 
> 1.7.9.5
> 
> 
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
Taras Kondratiuk June 18, 2014, 10:37 a.m. UTC | #2
On 06/18/2014 12:58 PM, Jerin Jacob wrote:
> On Tue, Jun 17, 2014 at 08:55:11PM +0300, Taras Kondratiuk wrote:
>> Signed-off-by: Taras Kondratiuk <taras.kondratiuk@linaro.org>
>> ---
>>  include/odp_config.h                             |   11 +++---
>>  platform/linux-generic/include/plat/odp_config.h |   44 ++++++++++++++++++++++
> 
> All the parameters in include/odp_config.h are derived from the include/plat/odp_config.h file.
> So instead of keeping additional indirection, can we move the config file to per platform(platform/xxxx/include/odp_config.h)

It is just a coincidence that all parameters here are platform-specific.
Some configuration functions may be added later.

> More over, I think config file should not be part of ODP public header file as each platform will have its on constrains on each config items
> and number of config items to build the libodp.{a,so}

IMO some of these values are public. Application may want, for example,
to know ODP_CONFIG_MAX_THREADS value to adjust its logic. Any
additional configuration parameters can be added to a platform config
header if needed.
Jerin Jacob June 18, 2014, 12:29 p.m. UTC | #3
On Wed, Jun 18, 2014 at 01:37:59PM +0300, Taras Kondratiuk wrote:
> On 06/18/2014 12:58 PM, Jerin Jacob wrote:
> > On Tue, Jun 17, 2014 at 08:55:11PM +0300, Taras Kondratiuk wrote:
> >> Signed-off-by: Taras Kondratiuk <taras.kondratiuk@linaro.org>
> >> ---
> >>  include/odp_config.h                             |   11 +++---
> >>  platform/linux-generic/include/plat/odp_config.h |   44 ++++++++++++++++++++++
> > 
> > All the parameters in include/odp_config.h are derived from the include/plat/odp_config.h file.
> > So instead of keeping additional indirection, can we move the config file to per platform(platform/xxxx/include/odp_config.h)
> 
> It is just a coincidence that all parameters here are platform-specific.
> Some configuration functions may be added later.
>

I was under the impression that, the value for such configuration always
comes from the platform as we are including only plat/odp_config.h and
if we try to find a default value for any configuration item that works
well across all platform then it may not be a configuration entry.

Anyway, if you foresee any such global configuration in future then lets keep the
indirection.
 
> > More over, I think config file should not be part of ODP public header file as each platform will have its on constrains on each config items
> > and number of config items to build the libodp.{a,so}
> 
> IMO some of these values are public. Application may want, for example,
> to know ODP_CONFIG_MAX_THREADS value to adjust its logic. Any
> additional configuration parameters can be added to a platform config
> header if needed.

IMO Interface between ODP and apps better to be an API, just like ODP system info APIs(example: odp_sys_core_count).

> 
> -- 
> Taras Kondratiuk
Taras Kondratiuk June 18, 2014, 12:48 p.m. UTC | #4
On 06/18/2014 03:29 PM, Jerin Jacob wrote:
> On Wed, Jun 18, 2014 at 01:37:59PM +0300, Taras Kondratiuk wrote:
>> On 06/18/2014 12:58 PM, Jerin Jacob wrote:
>>> On Tue, Jun 17, 2014 at 08:55:11PM +0300, Taras Kondratiuk wrote:
>>>> Signed-off-by: Taras Kondratiuk <taras.kondratiuk@linaro.org>
>>>> ---
>>>>  include/odp_config.h                             |   11 +++---
>>>>  platform/linux-generic/include/plat/odp_config.h |   44 ++++++++++++++++++++++
>>>
>>> All the parameters in include/odp_config.h are derived from the include/plat/odp_config.h file.
>>> So instead of keeping additional indirection, can we move the config file to per platform(platform/xxxx/include/odp_config.h)
>>
>> It is just a coincidence that all parameters here are platform-specific.
>> Some configuration functions may be added later.
>>
> 
> I was under the impression that, the value for such configuration always
> comes from the platform as we are including only plat/odp_config.h and
> if we try to find a default value for any configuration item that works
> well across all platform then it may not be a configuration entry.
> 
> Anyway, if you foresee any such global configuration in future then lets keep the
> indirection.
>  
>>> More over, I think config file should not be part of ODP public header file as each platform will have its on constrains on each config items
>>> and number of config items to build the libodp.{a,so}
>>
>> IMO some of these values are public. Application may want, for example,
>> to know ODP_CONFIG_MAX_THREADS value to adjust its logic. Any
>> additional configuration parameters can be added to a platform config
>> header if needed.
> 
> IMO Interface between ODP and apps better to be an API, just like ODP system info APIs(example: odp_sys_core_count).

Agree, but in this case a function prototypes like
odp_max_thread_count() will anyway go to odp_config.h, so no sense to
remove it. We need to agree on a basic approach to more forward. These
details can be polished later.
Taras Kondratiuk June 18, 2014, 12:48 p.m. UTC | #5
On 06/18/2014 03:29 PM, Jerin Jacob wrote:
> On Wed, Jun 18, 2014 at 01:37:59PM +0300, Taras Kondratiuk wrote:
>> On 06/18/2014 12:58 PM, Jerin Jacob wrote:
>>> On Tue, Jun 17, 2014 at 08:55:11PM +0300, Taras Kondratiuk wrote:
>>>> Signed-off-by: Taras Kondratiuk <taras.kondratiuk@linaro.org>
>>>> ---
>>>>  include/odp_config.h                             |   11 +++---
>>>>  platform/linux-generic/include/plat/odp_config.h |   44 ++++++++++++++++++++++
>>>
>>> All the parameters in include/odp_config.h are derived from the include/plat/odp_config.h file.
>>> So instead of keeping additional indirection, can we move the config file to per platform(platform/xxxx/include/odp_config.h)
>>
>> It is just a coincidence that all parameters here are platform-specific.
>> Some configuration functions may be added later.
>>
> 
> I was under the impression that, the value for such configuration always
> comes from the platform as we are including only plat/odp_config.h and
> if we try to find a default value for any configuration item that works
> well across all platform then it may not be a configuration entry.
> 
> Anyway, if you foresee any such global configuration in future then lets keep the
> indirection.
>  
>>> More over, I think config file should not be part of ODP public header file as each platform will have its on constrains on each config items
>>> and number of config items to build the libodp.{a,so}
>>
>> IMO some of these values are public. Application may want, for example,
>> to know ODP_CONFIG_MAX_THREADS value to adjust its logic. Any
>> additional configuration parameters can be added to a platform config
>> header if needed.
> 
> IMO Interface between ODP and apps better to be an API, just like ODP system info APIs(example: odp_sys_core_count).

Agree, but in this case a function prototypes like
odp_max_thread_count() will anyway go to odp_config.h, so no sense to
remove it. We need to agree on a basic approach to more forward. These
details can be polished later.
Jerin Jacob June 18, 2014, 1:16 p.m. UTC | #6
On Wed, Jun 18, 2014 at 03:48:34PM +0300, Taras Kondratiuk wrote:
> On 06/18/2014 03:29 PM, Jerin Jacob wrote:
> > On Wed, Jun 18, 2014 at 01:37:59PM +0300, Taras Kondratiuk wrote:
> >> On 06/18/2014 12:58 PM, Jerin Jacob wrote:
> >>> On Tue, Jun 17, 2014 at 08:55:11PM +0300, Taras Kondratiuk wrote:
> >>>> Signed-off-by: Taras Kondratiuk <taras.kondratiuk@linaro.org>
> >>>> ---
> >>>>  include/odp_config.h                             |   11 +++---
> >>>>  platform/linux-generic/include/plat/odp_config.h |   44 ++++++++++++++++++++++
> >>>
> >>> All the parameters in include/odp_config.h are derived from the include/plat/odp_config.h file.
> >>> So instead of keeping additional indirection, can we move the config file to per platform(platform/xxxx/include/odp_config.h)
> >>
> >> It is just a coincidence that all parameters here are platform-specific.
> >> Some configuration functions may be added later.
> >>
> > 
> > I was under the impression that, the value for such configuration always
> > comes from the platform as we are including only plat/odp_config.h and
> > if we try to find a default value for any configuration item that works
> > well across all platform then it may not be a configuration entry.
> > 
> > Anyway, if you foresee any such global configuration in future then lets keep the
> > indirection.
> >  
> >>> More over, I think config file should not be part of ODP public header file as each platform will have its on constrains on each config items
> >>> and number of config items to build the libodp.{a,so}
> >>
> >> IMO some of these values are public. Application may want, for example,
> >> to know ODP_CONFIG_MAX_THREADS value to adjust its logic. Any
> >> additional configuration parameters can be added to a platform config
> >> header if needed.
> > 
> > IMO Interface between ODP and apps better to be an API, just like ODP system info APIs(example: odp_sys_core_count).
> 
> Agree, but in this case a function prototypes like
> odp_max_thread_count() will anyway go to odp_config.h, so no sense to
> remove it. We need to agree on a basic approach to more forward. These
> details can be polished later.

I agree and I liked your approach towards introducing indirection to enable platform abstraction.
I was trying to see is it really required for odp_config.h file or not(as its different from all other header file)?. 
If you think its really required then please keep the indirection for odp_config.h.

Thanks,
Jerin

> 
> -- 
> Taras Kondratiuk
Savolainen, Petri (NSN - FI/Espoo) June 19, 2014, 12:14 p.m. UTC | #7
> -----Original Message-----
> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp-
> bounces@lists.linaro.org] On Behalf Of ext Jerin Jacob
> Sent: Wednesday, June 18, 2014 4:17 PM
> To: Taras Kondratiuk
> Cc: lng-odp@lists.linaro.org
> Subject: Re: [lng-odp] [PATCH v2 09/18] Split out platform-specific
> part of odp_config.h
> 
> On Wed, Jun 18, 2014 at 03:48:34PM +0300, Taras Kondratiuk wrote:
> > On 06/18/2014 03:29 PM, Jerin Jacob wrote:
> > > On Wed, Jun 18, 2014 at 01:37:59PM +0300, Taras Kondratiuk wrote:
> > >> On 06/18/2014 12:58 PM, Jerin Jacob wrote:
> > >>> On Tue, Jun 17, 2014 at 08:55:11PM +0300, Taras Kondratiuk wrote:
> > >>>> Signed-off-by: Taras Kondratiuk <taras.kondratiuk@linaro.org>
> > >>>> ---
> > >>>>  include/odp_config.h                             |   11 +++---
> > >>>>  platform/linux-generic/include/plat/odp_config.h |   44
> ++++++++++++++++++++++
> > >>>
> > >>> All the parameters in include/odp_config.h are derived from the
> include/plat/odp_config.h file.
> > >>> So instead of keeping additional indirection, can we move the
> config file to per platform(platform/xxxx/include/odp_config.h)
> > >>
> > >> It is just a coincidence that all parameters here are platform-
> specific.
> > >> Some configuration functions may be added later.
> > >>
> > >
> > > I was under the impression that, the value for such configuration
> always
> > > comes from the platform as we are including only plat/odp_config.h
> and
> > > if we try to find a default value for any configuration item that
> works
> > > well across all platform then it may not be a configuration entry.
> > >
> > > Anyway, if you foresee any such global configuration in future then
> lets keep the
> > > indirection.
> > >
> > >>> More over, I think config file should not be part of ODP public
> header file as each platform will have its on constrains on each config
> items
> > >>> and number of config items to build the libodp.{a,so}
> > >>
> > >> IMO some of these values are public. Application may want, for
> example,
> > >> to know ODP_CONFIG_MAX_THREADS value to adjust its logic. Any
> > >> additional configuration parameters can be added to a platform
> config
> > >> header if needed.
> > >
> > > IMO Interface between ODP and apps better to be an API, just like
> ODP system info APIs(example: odp_sys_core_count).
> >
> > Agree, but in this case a function prototypes like
> > odp_max_thread_count() will anyway go to odp_config.h, so no sense to
> > remove it. We need to agree on a basic approach to more forward.
> These
> > details can be polished later.
> 
> I agree and I liked your approach towards introducing indirection to
> enable platform abstraction.
> I was trying to see is it really required for odp_config.h file or
> not(as its different from all other header file)?.
> If you think its really required then please keep the indirection for
> odp_config.h.
> 

A set of defines (like ODP_CONFIG_MAX_THREADS) are needed for controlling size of  application data structures (e.g. an array of per thread statistics, per queue context data, ...).

Also user should be able to modify some of these (to optimize memory ODP implementation memory usage, etc). Those should be collected into a common header file (odp_config.h, which would be overridable on application level ?) 
 
- Petri
diff mbox

Patch

diff --git a/include/odp_config.h b/include/odp_config.h
index 7a15ff9..e6d0cae 100644
--- a/include/odp_config.h
+++ b/include/odp_config.h
@@ -18,31 +18,32 @@ 
 extern "C" {
 #endif
 
+#include <plat/odp_config.h>
 
 /**
  * Maximum number of threads
  */
-#define ODP_CONFIG_MAX_THREADS  128
+#define ODP_CONFIG_MAX_THREADS  PLAT_ODP_CONFIG_MAX_THREADS
 
 /**
  * Maximum number of buffer pools
  */
-#define ODP_CONFIG_BUFFER_POOLS 16
+#define ODP_CONFIG_BUFFER_POOLS PLAT_ODP_CONFIG_BUFFER_POOLS
 
 /**
  * Maximum number of queues
  */
-#define ODP_CONFIG_QUEUES       1024
+#define ODP_CONFIG_QUEUES       PLAT_ODP_CONFIG_QUEUES
 
 /**
  * Number of scheduling priorities
  */
-#define ODP_CONFIG_SCHED_PRIOS  8
+#define ODP_CONFIG_SCHED_PRIOS  PLAT_ODP_CONFIG_SCHED_PRIOS
 
 /**
  * Maximum number of packet IO resources
  */
-#define ODP_CONFIG_PKTIO_ENTRIES 64
+#define ODP_CONFIG_PKTIO_ENTRIES PLAT_ODP_CONFIG_PKTIO_ENTRIES
 
 #ifdef __cplusplus
 }
diff --git a/platform/linux-generic/include/plat/odp_config.h b/platform/linux-generic/include/plat/odp_config.h
new file mode 100644
index 0000000..7230925
--- /dev/null
+++ b/platform/linux-generic/include/plat/odp_config.h
@@ -0,0 +1,44 @@ 
+/* Copyright (c) 2013, Linaro Limited
+ * All rights reserved.
+ *
+ * SPDX-License-Identifier:     BSD-3-Clause
+ */
+
+
+/**
+ * @file
+ *
+ * ODP configuration
+ */
+
+#ifndef ODP_CONFIG_H_
+#error This file should be included only into corresponding top level header
+#else
+
+/**
+ * Maximum number of threads
+ */
+#define PLAT_ODP_CONFIG_MAX_THREADS  128
+
+/**
+ * Maximum number of buffer pools
+ */
+#define PLAT_ODP_CONFIG_BUFFER_POOLS 16
+
+/**
+ * Maximum number of queues
+ */
+#define PLAT_ODP_CONFIG_QUEUES       1024
+
+/**
+ * Number of scheduling priorities
+ */
+#define PLAT_ODP_CONFIG_SCHED_PRIOS  8
+
+/**
+ * Maximum number of packet IO resources
+ */
+#define PLAT_ODP_CONFIG_PKTIO_ENTRIES 64
+
+
+#endif