| Message ID | 20170530202647.21516-1-bill.fischofer@linaro.org |
|---|---|
| State | New |
| Headers | show |
> diff --git a/include/odp/api/spec/init.h b/include/odp/api/spec/init.h I think it's better to have a new file for this (feature.h). It may be needed also in other parts (of implementation) than init. > index 154cdf8f..ab2ec577 100644 > --- a/include/odp/api/spec/init.h > +++ b/include/odp/api/spec/init.h > @@ -105,6 +105,78 @@ typedef int (*odp_log_func_t)(odp_log_level_t level, > const char *fmt, ...); > /** Replaceable abort function */ > typedef void (*odp_abort_func_t)(void) ODP_NORETURN; > > +/** ODP Feature set. Used to inform implementation which > + * ODP features will not be used by this application. Knowing this may > + * improve implementation efficiency. > + */ > +typedef union odp_unused_feature_t { odp_features_t or odp_feature_list_t or odp_feature_bits_t could be better, since odp_feature_t could be reserved for an enum (single feature). Anyway, used vs unused is variable definition, not type. > + uint64_t all_features; > + struct { > + /** APIs in atomic.h are not used */ > + uint32_t atomic:1; > + /** APIs in barrier.h are not used */ > + uint32_t barrier:1; > + /** APIs in buffer.h are not used */ > + uint32_t buffer:1; > + /** APIs in byteorder.h are not used */ > + uint32_t byteorder:1; > + /** APIs in classification.h are not used */ > + uint32_t classification:1; > + /** APIs in cpu.h are not used */ > + uint32_t cpu:1; > + /** APIs in cpumask.h are not used */ > + uint32_t cpumask:1; > + /** APIs in crypto.h are not used */ > + uint32_t crypto:1; > + /** APIs in errno.h are not used */ > + uint32_t odp_errno:1; > + /** APIs in event.h are not used */ > + uint32_t event:1; > + /** APIs in hash.h are not used */ > + uint32_t hash:1; > + /** APIs in ipsec.h are not used */ > + uint32_t ipsec:1; > + /** APIs in packet_flags.h are not used */ > + uint32_t packet_flags:1; > + /** APIs in packet.h are not used */ > + uint32_t packet:1; > + /** APIs in packet_io.h are not used */ > + uint32_t packet_io:1; > + /** APIs in packet_io_stats.h are not used */ > + uint32_t packet_io_stats:1; > + /** APIs in pool.h are not used */ > + uint32_t pool:1; > + /** APIs in queue.h are not used */ > + uint32_t queue:1; > + /** APIs in random.h are not used */ > + uint32_t random:1; > + /** APIs in rwlock.h are not used */ > + uint32_t rwlock:1; > + /** APIs in rwlock_recursive.h are not used */ > + uint32_t rwlock_recursive:1; > + /** APIs in schedule.h are not used */ > + uint32_t schedule:1; > + /** APIs in std_clib.h are not used */ > + uint32_t std_clib:1; > + /** APIs in sync.h are not used */ > + uint32_t sync:1; > + /** APIs in system_info.h are not used */ > + uint32_t system_info:1; > + /** APIs in thread.h are not used */ > + uint32_t thread:1; > + /** APIs in thrmask.h are not used */ > + uint32_t thrmask:1; > + /** APIs in ticketlock.h are not used */ > + uint32_t ticketlock:1; > + /** APIs in time.h are not used */ > + uint32_t time:1; > + /** APIs in timer.h are not used */ > + uint32_t timer:1; > + /** APIs in traffic_mngr.h are not used */ > + uint32_t traffic_mngr:1; > + }; > +} odp_unused_feature_t; > + The list should be much shorter, at least in the beginning. It's easy to add more later on. Just major/complex features should be enough for now. typedef union odp_features_t { struct { uint32_t classification:1; uint32_t crypto:1; uint32_t ipsec:1; uint32_t packet_io:1; uint32_t pool:1; uint32_t queue:1; uint32_t schedule:1; uint32_t time:1; uint32_t timer:1; uint32_t traffic_mngr:1; } feat; uint32_t all_feat; } odp_features_t; > /** > * ODP initialization data > * > @@ -153,6 +225,10 @@ typedef struct odp_init_t { > odp_log_func_t log_fn; > /** Replacement for the default abort fn */ > odp_abort_func_t abort_fn; > + /** Hints to allow application to tell implementation which ODP > + * features will not be used by this application. > + */ > + odp_unused_feature_t unused_features; odp_features_t not_used; Later on we may add some init time configuration here and the same bit field can be used to indicate which configs we have filled in. odp_features_t config; but that can be left for future. -Petri > } odp_init_t; >
On Wed, May 31, 2017 at 3:33 AM, Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia.com> wrote: > >> diff --git a/include/odp/api/spec/init.h b/include/odp/api/spec/init.h > > I think it's better to have a new file for this (feature.h). It may be needed also in other parts (of implementation) than init. Sounds reasonable, however we still need to decide how this gets passed to odp_init_global. I'm not too happy with this first cut as all of our examples just specify NULL for the odp_init_t to request defaults. Adding this to odp_init_t does seems to call for adding an odp_init_param_init() routine for completeness, so perhaps it's better to have another optional argument to odp_init_global(). But adding a new parameter introduces compatibility issues since C99 doesn't support variadic arguments where optional ones can simply be omitted rather than specified as NULL. > > >> index 154cdf8f..ab2ec577 100644 >> --- a/include/odp/api/spec/init.h >> +++ b/include/odp/api/spec/init.h >> @@ -105,6 +105,78 @@ typedef int (*odp_log_func_t)(odp_log_level_t level, >> const char *fmt, ...); >> /** Replaceable abort function */ >> typedef void (*odp_abort_func_t)(void) ODP_NORETURN; >> >> +/** ODP Feature set. Used to inform implementation which >> + * ODP features will not be used by this application. Knowing this may >> + * improve implementation efficiency. >> + */ >> +typedef union odp_unused_feature_t { > > odp_features_t or odp_feature_list_t or odp_feature_bits_t could be better, since odp_feature_t could be reserved for an enum (single feature). > > Anyway, used vs unused is variable definition, not type. I wanted to make this unused vs. used both for ease of specification (you don't want to have to explicitly say which features you are using as that's burdensome and poses upwards compatibility issues. ODP implementations should assume that applications may use any feature unless the application explicitly says "I'm not going to use feature X". It should not have to state "I'm going to use features A, B, and C, and I forgot to mention D (bug fix)". > >> + uint64_t all_features; >> + struct { >> + /** APIs in atomic.h are not used */ >> + uint32_t atomic:1; >> + /** APIs in barrier.h are not used */ >> + uint32_t barrier:1; >> + /** APIs in buffer.h are not used */ >> + uint32_t buffer:1; >> + /** APIs in byteorder.h are not used */ >> + uint32_t byteorder:1; >> + /** APIs in classification.h are not used */ >> + uint32_t classification:1; >> + /** APIs in cpu.h are not used */ >> + uint32_t cpu:1; >> + /** APIs in cpumask.h are not used */ >> + uint32_t cpumask:1; >> + /** APIs in crypto.h are not used */ >> + uint32_t crypto:1; >> + /** APIs in errno.h are not used */ >> + uint32_t odp_errno:1; >> + /** APIs in event.h are not used */ >> + uint32_t event:1; >> + /** APIs in hash.h are not used */ >> + uint32_t hash:1; >> + /** APIs in ipsec.h are not used */ >> + uint32_t ipsec:1; >> + /** APIs in packet_flags.h are not used */ >> + uint32_t packet_flags:1; >> + /** APIs in packet.h are not used */ >> + uint32_t packet:1; >> + /** APIs in packet_io.h are not used */ >> + uint32_t packet_io:1; >> + /** APIs in packet_io_stats.h are not used */ >> + uint32_t packet_io_stats:1; >> + /** APIs in pool.h are not used */ >> + uint32_t pool:1; >> + /** APIs in queue.h are not used */ >> + uint32_t queue:1; >> + /** APIs in random.h are not used */ >> + uint32_t random:1; >> + /** APIs in rwlock.h are not used */ >> + uint32_t rwlock:1; >> + /** APIs in rwlock_recursive.h are not used */ >> + uint32_t rwlock_recursive:1; >> + /** APIs in schedule.h are not used */ >> + uint32_t schedule:1; >> + /** APIs in std_clib.h are not used */ >> + uint32_t std_clib:1; >> + /** APIs in sync.h are not used */ >> + uint32_t sync:1; >> + /** APIs in system_info.h are not used */ >> + uint32_t system_info:1; >> + /** APIs in thread.h are not used */ >> + uint32_t thread:1; >> + /** APIs in thrmask.h are not used */ >> + uint32_t thrmask:1; >> + /** APIs in ticketlock.h are not used */ >> + uint32_t ticketlock:1; >> + /** APIs in time.h are not used */ >> + uint32_t time:1; >> + /** APIs in timer.h are not used */ >> + uint32_t timer:1; >> + /** APIs in traffic_mngr.h are not used */ >> + uint32_t traffic_mngr:1; >> + }; >> +} odp_unused_feature_t; >> + > > The list should be much shorter, at least in the beginning. It's easy to add more later on. Just major/complex features should be enough for now. I agree. This was just to help frame the discussion. Many of these don't make sense to try to control. Your list seems reasonable. > > typedef union odp_features_t { > > struct { > uint32_t classification:1; > uint32_t crypto:1; > uint32_t ipsec:1; > uint32_t packet_io:1; > uint32_t pool:1; > uint32_t queue:1; > uint32_t schedule:1; > uint32_t time:1; > uint32_t timer:1; > uint32_t traffic_mngr:1; > } feat; I prefer an anonymous struct here to avoid syntactic clutter. not_used.ipsec reads better than not_used.feat.ipsec. > > uint32_t all_feat; > > } odp_features_t; > > > >> /** >> * ODP initialization data >> * >> @@ -153,6 +225,10 @@ typedef struct odp_init_t { >> odp_log_func_t log_fn; >> /** Replacement for the default abort fn */ >> odp_abort_func_t abort_fn; >> + /** Hints to allow application to tell implementation which ODP >> + * features will not be used by this application. >> + */ >> + odp_unused_feature_t unused_features; > > odp_features_t not_used; > > > Later on we may add some init time configuration here and the same bit field can be used to indicate which configs we have filled in. > odp_features_t config; > > but that can be left for future. > > -Petri > > >> } odp_init_t; >>
> -----Original Message----- > From: Bill Fischofer [mailto:bill.fischofer@linaro.org] > Sent: Wednesday, May 31, 2017 2:32 PM > To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia.com> > Cc: lng-odp@lists.linaro.org > Subject: Re: [lng-odp] [RFC API-NEXT PATCH] api: init: add unused feature > hints to init struct > > On Wed, May 31, 2017 at 3:33 AM, Savolainen, Petri (Nokia - FI/Espoo) > <petri.savolainen@nokia.com> wrote: > > > >> diff --git a/include/odp/api/spec/init.h b/include/odp/api/spec/init.h > > > > I think it's better to have a new file for this (feature.h). It may be > needed also in other parts (of implementation) than init. > > Sounds reasonable, however we still need to decide how this gets > passed to odp_init_global. I'm not too happy with this first cut as > all of our examples just specify NULL for the odp_init_t to request > defaults. Adding this to odp_init_t does seems to call for adding an > odp_init_param_init() routine for completeness, so perhaps it's better > to have another optional argument to odp_init_global(). But adding a > new parameter introduces compatibility issues since C99 doesn't > support variadic arguments where optional ones can simply be omitted > rather than specified as NULL. odp_global_init() prototype does not need to change: params == NULL means still "all defaults". Yes, odp_init_param_init() is needed. So that application can init params and update only those fields that are not "default". > > > > > > >> index 154cdf8f..ab2ec577 100644 > >> --- a/include/odp/api/spec/init.h > >> +++ b/include/odp/api/spec/init.h > >> @@ -105,6 +105,78 @@ typedef int (*odp_log_func_t)(odp_log_level_t > level, > >> const char *fmt, ...); > >> /** Replaceable abort function */ > >> typedef void (*odp_abort_func_t)(void) ODP_NORETURN; > >> > >> +/** ODP Feature set. Used to inform implementation which > >> + * ODP features will not be used by this application. Knowing this > may > >> + * improve implementation efficiency. > >> + */ > >> +typedef union odp_unused_feature_t { > > > > odp_features_t or odp_feature_list_t or odp_feature_bits_t could be > better, since odp_feature_t could be reserved for an enum (single > feature). > > > > Anyway, used vs unused is variable definition, not type. > > I wanted to make this unused vs. used both for ease of specification > (you don't want to have to explicitly say which features you are using > as that's burdensome and poses upwards compatibility issues. ODP > implementations should assume that applications may use any feature > unless the application explicitly says "I'm not going to use feature > X". It should not have to state "I'm going to use features A, B, and > C, and I forgot to mention D (bug fix)". That's fine but the type is a list of features. Variable name indicated what the purpose of a list is (use, not use, config, etc). > >> +} odp_unused_feature_t; > >> + > > > > The list should be much shorter, at least in the beginning. It's easy to > add more later on. Just major/complex features should be enough for now. > > I agree. This was just to help frame the discussion. Many of these > don't make sense to try to control. Your list seems reasonable. > > > > > typedef union odp_features_t { > > > > struct { > > uint32_t classification:1; > > uint32_t crypto:1; > > uint32_t ipsec:1; > > uint32_t packet_io:1; > > uint32_t pool:1; > > uint32_t queue:1; > > uint32_t schedule:1; > > uint32_t time:1; > > uint32_t timer:1; > > uint32_t traffic_mngr:1; > > } feat; > > I prefer an anonymous struct here to avoid syntactic clutter. > not_used.ipsec reads better than not_used.feat.ipsec. It's better to have names for structs, since later on odp_features_t may need to be extended with some other struct. I think that happened already with some API type, and struct name saved us with cleaner / backwards compatible definition. Anonymous union saves clutter already. "not_used.feat.ipsec" is quite readable. E.g. this could be then possible later on "not_used.sub_feat.ipsec.inline", or something else which extends the type to contain also other fields. -Petri
On 31 May 2017 at 14:03, Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia.com> wrote: > >> diff --git a/include/odp/api/spec/init.h b/include/odp/api/spec/init.h > > I think it's better to have a new file for this (feature.h). It may be needed also in other parts (of implementation) than init. > > >> index 154cdf8f..ab2ec577 100644 >> --- a/include/odp/api/spec/init.h >> +++ b/include/odp/api/spec/init.h >> @@ -105,6 +105,78 @@ typedef int (*odp_log_func_t)(odp_log_level_t level, >> const char *fmt, ...); >> /** Replaceable abort function */ >> typedef void (*odp_abort_func_t)(void) ODP_NORETURN; >> >> +/** ODP Feature set. Used to inform implementation which >> + * ODP features will not be used by this application. Knowing this may >> + * improve implementation efficiency. >> + */ >> +typedef union odp_unused_feature_t { > > odp_features_t or odp_feature_list_t or odp_feature_bits_t could be better, since odp_feature_t could be reserved for an enum (single feature). > > Anyway, used vs unused is variable definition, not type. > >> + uint64_t all_features; >> + struct { >> + /** APIs in atomic.h are not used */ >> + uint32_t atomic:1; >> + /** APIs in barrier.h are not used */ >> + uint32_t barrier:1; >> + /** APIs in buffer.h are not used */ >> + uint32_t buffer:1; >> + /** APIs in byteorder.h are not used */ >> + uint32_t byteorder:1; >> + /** APIs in classification.h are not used */ >> + uint32_t classification:1; >> + /** APIs in cpu.h are not used */ >> + uint32_t cpu:1; >> + /** APIs in cpumask.h are not used */ >> + uint32_t cpumask:1; >> + /** APIs in crypto.h are not used */ >> + uint32_t crypto:1; >> + /** APIs in errno.h are not used */ >> + uint32_t odp_errno:1; >> + /** APIs in event.h are not used */ >> + uint32_t event:1; >> + /** APIs in hash.h are not used */ >> + uint32_t hash:1; >> + /** APIs in ipsec.h are not used */ >> + uint32_t ipsec:1; >> + /** APIs in packet_flags.h are not used */ >> + uint32_t packet_flags:1; >> + /** APIs in packet.h are not used */ >> + uint32_t packet:1; >> + /** APIs in packet_io.h are not used */ >> + uint32_t packet_io:1; >> + /** APIs in packet_io_stats.h are not used */ >> + uint32_t packet_io_stats:1; >> + /** APIs in pool.h are not used */ >> + uint32_t pool:1; >> + /** APIs in queue.h are not used */ >> + uint32_t queue:1; >> + /** APIs in random.h are not used */ >> + uint32_t random:1; >> + /** APIs in rwlock.h are not used */ >> + uint32_t rwlock:1; >> + /** APIs in rwlock_recursive.h are not used */ >> + uint32_t rwlock_recursive:1; >> + /** APIs in schedule.h are not used */ >> + uint32_t schedule:1; >> + /** APIs in std_clib.h are not used */ >> + uint32_t std_clib:1; >> + /** APIs in sync.h are not used */ >> + uint32_t sync:1; >> + /** APIs in system_info.h are not used */ >> + uint32_t system_info:1; >> + /** APIs in thread.h are not used */ >> + uint32_t thread:1; >> + /** APIs in thrmask.h are not used */ >> + uint32_t thrmask:1; >> + /** APIs in ticketlock.h are not used */ >> + uint32_t ticketlock:1; >> + /** APIs in time.h are not used */ >> + uint32_t time:1; >> + /** APIs in timer.h are not used */ >> + uint32_t timer:1; >> + /** APIs in traffic_mngr.h are not used */ >> + uint32_t traffic_mngr:1; >> + }; >> +} odp_unused_feature_t; >> + > > The list should be much shorter, at least in the beginning. It's easy to add more later on. Just major/complex features should be enough for now. > > typedef union odp_features_t { > > struct { > uint32_t classification:1; > uint32_t crypto:1; > uint32_t ipsec:1; > uint32_t packet_io:1; > uint32_t pool:1; IMO packet_io and pool should not be part of this list. Since they are very basic for dataplane application. -Bala > uint32_t queue:1; > uint32_t schedule:1; > uint32_t time:1; > uint32_t timer:1; > uint32_t traffic_mngr:1; > } feat; > > uint32_t all_feat; > > } odp_features_t; > > > >> /** >> * ODP initialization data >> * >> @@ -153,6 +225,10 @@ typedef struct odp_init_t { >> odp_log_func_t log_fn; >> /** Replacement for the default abort fn */ >> odp_abort_func_t abort_fn; >> + /** Hints to allow application to tell implementation which ODP >> + * features will not be used by this application. >> + */ >> + odp_unused_feature_t unused_features; > > odp_features_t not_used; > > > Later on we may add some init time configuration here and the same bit field can be used to indicate which configs we have filled in. > odp_features_t config; > > but that can be left for future. > > -Petri > > >> } odp_init_t; >>
diff --git a/include/odp/api/spec/init.h b/include/odp/api/spec/init.h index 154cdf8f..ab2ec577 100644 --- a/include/odp/api/spec/init.h +++ b/include/odp/api/spec/init.h @@ -105,6 +105,78 @@ typedef int (*odp_log_func_t)(odp_log_level_t level, const char *fmt, ...); /** Replaceable abort function */ typedef void (*odp_abort_func_t)(void) ODP_NORETURN; +/** ODP Feature set. Used to inform implementation which + * ODP features will not be used by this application. Knowing this may + * improve implementation efficiency. + */ +typedef union odp_unused_feature_t { + uint64_t all_features; + struct { + /** APIs in atomic.h are not used */ + uint32_t atomic:1; + /** APIs in barrier.h are not used */ + uint32_t barrier:1; + /** APIs in buffer.h are not used */ + uint32_t buffer:1; + /** APIs in byteorder.h are not used */ + uint32_t byteorder:1; + /** APIs in classification.h are not used */ + uint32_t classification:1; + /** APIs in cpu.h are not used */ + uint32_t cpu:1; + /** APIs in cpumask.h are not used */ + uint32_t cpumask:1; + /** APIs in crypto.h are not used */ + uint32_t crypto:1; + /** APIs in errno.h are not used */ + uint32_t odp_errno:1; + /** APIs in event.h are not used */ + uint32_t event:1; + /** APIs in hash.h are not used */ + uint32_t hash:1; + /** APIs in ipsec.h are not used */ + uint32_t ipsec:1; + /** APIs in packet_flags.h are not used */ + uint32_t packet_flags:1; + /** APIs in packet.h are not used */ + uint32_t packet:1; + /** APIs in packet_io.h are not used */ + uint32_t packet_io:1; + /** APIs in packet_io_stats.h are not used */ + uint32_t packet_io_stats:1; + /** APIs in pool.h are not used */ + uint32_t pool:1; + /** APIs in queue.h are not used */ + uint32_t queue:1; + /** APIs in random.h are not used */ + uint32_t random:1; + /** APIs in rwlock.h are not used */ + uint32_t rwlock:1; + /** APIs in rwlock_recursive.h are not used */ + uint32_t rwlock_recursive:1; + /** APIs in schedule.h are not used */ + uint32_t schedule:1; + /** APIs in std_clib.h are not used */ + uint32_t std_clib:1; + /** APIs in sync.h are not used */ + uint32_t sync:1; + /** APIs in system_info.h are not used */ + uint32_t system_info:1; + /** APIs in thread.h are not used */ + uint32_t thread:1; + /** APIs in thrmask.h are not used */ + uint32_t thrmask:1; + /** APIs in ticketlock.h are not used */ + uint32_t ticketlock:1; + /** APIs in time.h are not used */ + uint32_t time:1; + /** APIs in timer.h are not used */ + uint32_t timer:1; + /** APIs in traffic_mngr.h are not used */ + uint32_t traffic_mngr:1; + }; +} odp_unused_feature_t; + /** * ODP initialization data * @@ -153,6 +225,10 @@ typedef struct odp_init_t { odp_log_func_t log_fn; /** Replacement for the default abort fn */ odp_abort_func_t abort_fn; + /** Hints to allow application to tell implementation which ODP + * features will not be used by this application. + */ + odp_unused_feature_t unused_features; } odp_init_t; /** @@ -165,6 +241,12 @@ typedef struct odp_init_t { * passing any required platform specific data. */ +/** + * Initialize the odp_init_t to default values for all fields. + * + * @param[out] param Address of the odp_init_t to be initialized + */ +void odp_init_param_init(odp_init_t *param); /** * Global ODP initialization
RFC based on discussions we had today regarding feature hints. Add an optimization hint, allow applications to indicate which ODP feature(s) they will not use. This may permit implementations to behave more efficiently if it knows that certain features will not be used by the application. Also add the new API odp_init_param_init() to initialize the odp_init_t to default values. Alternative: Should this be a separate parameter to odp_init_global()? Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> --- include/odp/api/spec/init.h | 82 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) -- 2.11.0