Message ID | 20170123194608.4552-1-mike.holmes@linaro.org |
---|---|
Headers | show |
Series | introduce odph_api.h and clean up public helper API | expand |
Reviewed-and-tested-by: Brian Brooks <brian.brooks@linaro.org> On 01/23 14:46:04, Mike Holmes wrote: > Greatly reduce the proliferation of helper includes that every app needs > Make the public helper API very obvious > Fix recent inclusion of table APIs that were not in the helper include dir and > were not exported during install. > > Mike Holmes (4): > helper: add odph_api.h for existing exported headers > helper: use odph_api.h for test include for unexported files > test: use odph_api.h > examples: use odph_api.h > > example/classifier/odp_classifier.c | 4 +-- > example/generator/odp_generator.c | 6 +--- > example/ipsec/odp_ipsec.c | 6 +--- > example/ipsec/odp_ipsec_fwd_db.h | 1 - > example/ipsec/odp_ipsec_loop_db.h | 1 - > example/ipsec/odp_ipsec_misc.h | 4 +-- > example/ipsec/odp_ipsec_stream.c | 4 +-- > example/l2fwd_simple/odp_l2fwd_simple.c | 4 +-- > example/l3fwd/odp_l3fwd.c | 6 +--- > example/l3fwd/odp_l3fwd_db.h | 2 +- > example/packet/odp_pktio.c | 4 +-- > example/switch/odp_switch.c | 4 +-- > example/time/time_global_test.c | 2 +- > example/timer/odp_timer_test.c | 2 +- > helper/Makefile.am | 11 +++--- > helper/cuckootable.c | 2 +- > helper/hashtable.c | 2 +- > helper/include/odp/helper/odph_api.h | 39 ++++++++++++++++++++++ > helper/{ => include/odp/helper}/odph_cuckootable.h | 0 > helper/{ => include/odp/helper}/odph_hashtable.h | 0 > .../{ => include/odp/helper}/odph_iplookuptable.h | 0 > helper/{ => include/odp/helper}/odph_lineartable.h | 0 > helper/iplookuptable.c | 2 +- > helper/lineartable.c | 2 +- > helper/test/chksum.c | 4 +-- > helper/test/cuckootable.c | 2 +- > helper/test/iplookuptable.c | 2 +- > helper/test/odpthreads.c | 2 +- > helper/test/parse.c | 3 +- > helper/test/table.c | 3 +- > test/common_plat/common/odp_cunit_common.c | 2 +- > test/common_plat/performance/odp_crypto.c | 2 +- > test/common_plat/performance/odp_l2fwd.c | 4 +-- > test/common_plat/performance/odp_pktio_perf.c | 5 +-- > test/common_plat/performance/odp_sched_latency.c | 2 +- > test/common_plat/performance/odp_scheduling.c | 2 +- > .../api/classification/odp_classification_common.c | 4 --- > .../classification/odp_classification_test_pmr.c | 4 --- > .../api/classification/odp_classification_tests.c | 4 --- > .../classification/odp_classification_testsuites.h | 1 + > test/common_plat/validation/api/pktio/pktio.c | 4 +-- > test/common_plat/validation/api/timer/timer.c | 2 +- > .../validation/api/traffic_mngr/traffic_mngr.c | 6 +--- > test/linux-generic/mmap_vlan_ins/mmap_vlan_ins.c | 4 +-- > test/linux-generic/pktio_ipc/ipc_common.h | 5 +-- > test/linux-generic/ring/ring_stress.c | 2 +- > 46 files changed, 80 insertions(+), 97 deletions(-) > create mode 100644 helper/include/odp/helper/odph_api.h > rename helper/{ => include/odp/helper}/odph_cuckootable.h (100%) > rename helper/{ => include/odp/helper}/odph_hashtable.h (100%) > rename helper/{ => include/odp/helper}/odph_iplookuptable.h (100%) > rename helper/{ => include/odp/helper}/odph_lineartable.h (100%) > > -- > 2.9.3
> -----Original Message----- > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of Mike > Holmes > Sent: Monday, January 23, 2017 9:46 PM > To: lng-odp@lists.linaro.org > Subject: [lng-odp] [PATCH 0/4] introduce odph_api.h and clean up public > helper API > > Greatly reduce the proliferation of helper includes that every app needs > Make the public helper API very obvious > Fix recent inclusion of table APIs that were not in the helper include dir > and > were not exported during install. I think we should not do this (combine all helpers into an "API"). First, "odph_api.h" gives the impression that helper definitions are part of ODP API. Those are not, since e.g. Ethernet header struct definition is not needed for HW acceleration - it's needed (reused) for writing tests and examples. We could decide to create and maintain an "ODP protocol suite" which would follow RFC's closely and provide those struct definitions for tens of protocols, all modes, options, etc included. Anyway, helper is not that today. Secondly, when application does this ... #include <odp/helper/eth.h> #include <odp/helper/icmp.h> #include <odp/helper/ip.h> ... instead of this. #include <odp/helper/odph_api.h> It is very easy to see what is happening, and what (extra) dependencies application has. -Petri
On 24 January 2017 at 03:23, Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia-bell-labs.com> wrote: > > >> -----Original Message----- >> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of Mike >> Holmes >> Sent: Monday, January 23, 2017 9:46 PM >> To: lng-odp@lists.linaro.org >> Subject: [lng-odp] [PATCH 0/4] introduce odph_api.h and clean up public >> helper API >> >> Greatly reduce the proliferation of helper includes that every app needs >> Make the public helper API very obvious >> Fix recent inclusion of table APIs that were not in the helper include dir >> and >> were not exported during install. > > I think we should not do this (combine all helpers into an "API"). > > First, "odph_api.h" gives the impression that helper definitions are part of ODP API. I dont think it is confusing, but we can use odp_helper_ap.h rather than odph_api and we do need a stable helper API so that apps are portable Those are not, since e.g. Ethernet header struct definition is not needed for HW acceleration Agree that is why they are helpers - it's needed (reused) for writing tests and examples. We could decide to create and maintain an "ODP protocol suite" which would follow RFC's closely and provide those struct definitions for tens of protocols, all modes, options, etc included. Anyway, helper is not that today. We can in future make better helper distinctions, but to date it has all been in one, and it has been in a confusing state for a long time, just look at the latest table stuff that was incorrectly accepted. I can see OS_helpers, protocol_helpers, algorithm_helpers (tables) etc all being their own mini lib that is ABI portable and installed once. > > Secondly, when application does this ... > > #include <odp/helper/eth.h> > #include <odp/helper/icmp.h> > #include <odp/helper/ip.h> > > ... instead of this. > > #include <odp/helper/odph_api.h> > > It is very easy to see what is happening, and what (extra) dependencies application has. The same for ODP, we need to pick one mechanism or another, I am happy to agree if ODP also includes everything in parts, but having it in one file makes the portable API for helper very concrete, it is need to make the helpers be ABI compatible which is needed for the cloud. > > > -Petri > > > > -- Mike Holmes Program Manager - Linaro Networking Group Linaro.org │ Open source software for ARM SoCs "Work should be fun and collaborative, the rest follows"
On 24 January 2017 at 16:51, Mike Holmes <mike.holmes@linaro.org> wrote: > On 24 January 2017 at 03:23, Savolainen, Petri (Nokia - FI/Espoo) > <petri.savolainen@nokia-bell-labs.com> wrote: >> >> >>> -----Original Message----- >>> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of Mike >>> Holmes >>> Sent: Monday, January 23, 2017 9:46 PM >>> To: lng-odp@lists.linaro.org >>> Subject: [lng-odp] [PATCH 0/4] introduce odph_api.h and clean up public >>> helper API >>> >>> Greatly reduce the proliferation of helper includes that every app needs >>> Make the public helper API very obvious >>> Fix recent inclusion of table APIs that were not in the helper include dir >>> and >>> were not exported during install. >> >> I think we should not do this (combine all helpers into an "API"). >> >> First, "odph_api.h" gives the impression that helper definitions are part of ODP API. > > I dont think it is confusing, but we can use odp_helper_ap.h rather > than odph_api and we do need a stable helper API so that apps are > portable > > Those are not, since e.g. Ethernet header struct definition is not > needed for HW acceleration > > Agree that is why they are helpers > > - it's needed (reused) for writing tests and examples. We could > decide to create and maintain an "ODP protocol suite" which would > follow RFC's closely and provide those struct definitions for tens of > protocols, all modes, options, etc included. Anyway, helper is not > that today. > > We can in future make better helper distinctions, but to date it has > all been in one, and it has been in a confusing state for a long time, > just look at the latest table stuff that was incorrectly accepted. > I can see OS_helpers, protocol_helpers, algorithm_helpers (tables) etc > all being their own mini lib that is ABI portable and installed once. That is what I like "odph_api": i.e. <block>_<interface>. If ODP helpers is to be splitted into different parts, this kind of naming structure make sense. like future odph_ipheaders, odph_os ... Not sure about the "api" part, but that can be changed when the different blocks get clear. > >> >> Secondly, when application does this ... >> >> #include <odp/helper/eth.h> >> #include <odp/helper/icmp.h> >> #include <odp/helper/ip.h> >> >> ... instead of this. >> >> #include <odp/helper/odph_api.h> >> >> It is very easy to see what is happening, and what (extra) dependencies application has. > > The same for ODP, we need to pick one mechanism or another, I am happy > to agree if ODP also includes everything in parts, but having it in > one file makes the portable API for helper very concrete, it is need > to make the helpers be ABI compatible which is needed for the cloud. Agree with the consistency: if we accept a single odp_api.h why not the same for helpers? I am rather positive to these changes: they gather the different helper interfaces in one spot, making clearer what it the helper api and what is not. The different API "subblocks" are not clear here, that could be improved later. Christophe. > >> >> >> -Petri >> >> >> >> > > > > -- > Mike Holmes > Program Manager - Linaro Networking Group > Linaro.org │ Open source software for ARM SoCs > "Work should be fun and collaborative, the rest follows"
For the series: Reviewed-by: Christophe Milard <christophe.milard@linaro.org> On 23 January 2017 at 20:46, Mike Holmes <mike.holmes@linaro.org> wrote: > Greatly reduce the proliferation of helper includes that every app needs > Make the public helper API very obvious > Fix recent inclusion of table APIs that were not in the helper include dir and > were not exported during install. > > Mike Holmes (4): > helper: add odph_api.h for existing exported headers > helper: use odph_api.h for test include for unexported files > test: use odph_api.h > examples: use odph_api.h > > example/classifier/odp_classifier.c | 4 +-- > example/generator/odp_generator.c | 6 +--- > example/ipsec/odp_ipsec.c | 6 +--- > example/ipsec/odp_ipsec_fwd_db.h | 1 - > example/ipsec/odp_ipsec_loop_db.h | 1 - > example/ipsec/odp_ipsec_misc.h | 4 +-- > example/ipsec/odp_ipsec_stream.c | 4 +-- > example/l2fwd_simple/odp_l2fwd_simple.c | 4 +-- > example/l3fwd/odp_l3fwd.c | 6 +--- > example/l3fwd/odp_l3fwd_db.h | 2 +- > example/packet/odp_pktio.c | 4 +-- > example/switch/odp_switch.c | 4 +-- > example/time/time_global_test.c | 2 +- > example/timer/odp_timer_test.c | 2 +- > helper/Makefile.am | 11 +++--- > helper/cuckootable.c | 2 +- > helper/hashtable.c | 2 +- > helper/include/odp/helper/odph_api.h | 39 ++++++++++++++++++++++ > helper/{ => include/odp/helper}/odph_cuckootable.h | 0 > helper/{ => include/odp/helper}/odph_hashtable.h | 0 > .../{ => include/odp/helper}/odph_iplookuptable.h | 0 > helper/{ => include/odp/helper}/odph_lineartable.h | 0 > helper/iplookuptable.c | 2 +- > helper/lineartable.c | 2 +- > helper/test/chksum.c | 4 +-- > helper/test/cuckootable.c | 2 +- > helper/test/iplookuptable.c | 2 +- > helper/test/odpthreads.c | 2 +- > helper/test/parse.c | 3 +- > helper/test/table.c | 3 +- > test/common_plat/common/odp_cunit_common.c | 2 +- > test/common_plat/performance/odp_crypto.c | 2 +- > test/common_plat/performance/odp_l2fwd.c | 4 +-- > test/common_plat/performance/odp_pktio_perf.c | 5 +-- > test/common_plat/performance/odp_sched_latency.c | 2 +- > test/common_plat/performance/odp_scheduling.c | 2 +- > .../api/classification/odp_classification_common.c | 4 --- > .../classification/odp_classification_test_pmr.c | 4 --- > .../api/classification/odp_classification_tests.c | 4 --- > .../classification/odp_classification_testsuites.h | 1 + > test/common_plat/validation/api/pktio/pktio.c | 4 +-- > test/common_plat/validation/api/timer/timer.c | 2 +- > .../validation/api/traffic_mngr/traffic_mngr.c | 6 +--- > test/linux-generic/mmap_vlan_ins/mmap_vlan_ins.c | 4 +-- > test/linux-generic/pktio_ipc/ipc_common.h | 5 +-- > test/linux-generic/ring/ring_stress.c | 2 +- > 46 files changed, 80 insertions(+), 97 deletions(-) > create mode 100644 helper/include/odp/helper/odph_api.h > rename helper/{ => include/odp/helper}/odph_cuckootable.h (100%) > rename helper/{ => include/odp/helper}/odph_hashtable.h (100%) > rename helper/{ => include/odp/helper}/odph_iplookuptable.h (100%) > rename helper/{ => include/odp/helper}/odph_lineartable.h (100%) > > -- > 2.9.3
I also think this cleanup is a step in the right direction. If the helpers are to be an adjunct ODP package that can be distributed alongside ODP then this is what's needed. Otherwise they are just a disjoint set of unrelated individual files that we should not try to package at all. But that would mean their use would be restricted to embedded ODP users, and the expectation is that most ODP users going forward are going to be working from a distribution in more of a cloud environment rather than the ODP repo. So +1 from me on this. These things can always be improved, but that can be done in stages building on this work. On Wed, Jan 25, 2017 at 8:30 AM, Christophe Milard <christophe.milard@linaro.org> wrote: > For the series: > > Reviewed-by: Christophe Milard <christophe.milard@linaro.org> > > On 23 January 2017 at 20:46, Mike Holmes <mike.holmes@linaro.org> wrote: >> Greatly reduce the proliferation of helper includes that every app needs >> Make the public helper API very obvious >> Fix recent inclusion of table APIs that were not in the helper include dir and >> were not exported during install. >> >> Mike Holmes (4): >> helper: add odph_api.h for existing exported headers >> helper: use odph_api.h for test include for unexported files >> test: use odph_api.h >> examples: use odph_api.h >> >> example/classifier/odp_classifier.c | 4 +-- >> example/generator/odp_generator.c | 6 +--- >> example/ipsec/odp_ipsec.c | 6 +--- >> example/ipsec/odp_ipsec_fwd_db.h | 1 - >> example/ipsec/odp_ipsec_loop_db.h | 1 - >> example/ipsec/odp_ipsec_misc.h | 4 +-- >> example/ipsec/odp_ipsec_stream.c | 4 +-- >> example/l2fwd_simple/odp_l2fwd_simple.c | 4 +-- >> example/l3fwd/odp_l3fwd.c | 6 +--- >> example/l3fwd/odp_l3fwd_db.h | 2 +- >> example/packet/odp_pktio.c | 4 +-- >> example/switch/odp_switch.c | 4 +-- >> example/time/time_global_test.c | 2 +- >> example/timer/odp_timer_test.c | 2 +- >> helper/Makefile.am | 11 +++--- >> helper/cuckootable.c | 2 +- >> helper/hashtable.c | 2 +- >> helper/include/odp/helper/odph_api.h | 39 ++++++++++++++++++++++ >> helper/{ => include/odp/helper}/odph_cuckootable.h | 0 >> helper/{ => include/odp/helper}/odph_hashtable.h | 0 >> .../{ => include/odp/helper}/odph_iplookuptable.h | 0 >> helper/{ => include/odp/helper}/odph_lineartable.h | 0 >> helper/iplookuptable.c | 2 +- >> helper/lineartable.c | 2 +- >> helper/test/chksum.c | 4 +-- >> helper/test/cuckootable.c | 2 +- >> helper/test/iplookuptable.c | 2 +- >> helper/test/odpthreads.c | 2 +- >> helper/test/parse.c | 3 +- >> helper/test/table.c | 3 +- >> test/common_plat/common/odp_cunit_common.c | 2 +- >> test/common_plat/performance/odp_crypto.c | 2 +- >> test/common_plat/performance/odp_l2fwd.c | 4 +-- >> test/common_plat/performance/odp_pktio_perf.c | 5 +-- >> test/common_plat/performance/odp_sched_latency.c | 2 +- >> test/common_plat/performance/odp_scheduling.c | 2 +- >> .../api/classification/odp_classification_common.c | 4 --- >> .../classification/odp_classification_test_pmr.c | 4 --- >> .../api/classification/odp_classification_tests.c | 4 --- >> .../classification/odp_classification_testsuites.h | 1 + >> test/common_plat/validation/api/pktio/pktio.c | 4 +-- >> test/common_plat/validation/api/timer/timer.c | 2 +- >> .../validation/api/traffic_mngr/traffic_mngr.c | 6 +--- >> test/linux-generic/mmap_vlan_ins/mmap_vlan_ins.c | 4 +-- >> test/linux-generic/pktio_ipc/ipc_common.h | 5 +-- >> test/linux-generic/ring/ring_stress.c | 2 +- >> 46 files changed, 80 insertions(+), 97 deletions(-) >> create mode 100644 helper/include/odp/helper/odph_api.h >> rename helper/{ => include/odp/helper}/odph_cuckootable.h (100%) >> rename helper/{ => include/odp/helper}/odph_hashtable.h (100%) >> rename helper/{ => include/odp/helper}/odph_iplookuptable.h (100%) >> rename helper/{ => include/odp/helper}/odph_lineartable.h (100%) >> >> -- >> 2.9.3
Merged, Maxim. On 01/25/17 18:59, Bill Fischofer wrote: > I also think this cleanup is a step in the right direction. If the > helpers are to be an adjunct ODP package that can be distributed > alongside ODP then this is what's needed. Otherwise they are just a > disjoint set of unrelated individual files that we should not try to > package at all. But that would mean their use would be restricted to > embedded ODP users, and the expectation is that most ODP users going > forward are going to be working from a distribution in more of a cloud > environment rather than the ODP repo. > > So +1 from me on this. These things can always be improved, but that > can be done in stages building on this work. > > On Wed, Jan 25, 2017 at 8:30 AM, Christophe Milard > <christophe.milard@linaro.org> wrote: >> For the series: >> >> Reviewed-by: Christophe Milard <christophe.milard@linaro.org> >> >> On 23 January 2017 at 20:46, Mike Holmes <mike.holmes@linaro.org> wrote: >>> Greatly reduce the proliferation of helper includes that every app needs >>> Make the public helper API very obvious >>> Fix recent inclusion of table APIs that were not in the helper include dir and >>> were not exported during install. >>> >>> Mike Holmes (4): >>> helper: add odph_api.h for existing exported headers >>> helper: use odph_api.h for test include for unexported files >>> test: use odph_api.h >>> examples: use odph_api.h >>> >>> example/classifier/odp_classifier.c | 4 +-- >>> example/generator/odp_generator.c | 6 +--- >>> example/ipsec/odp_ipsec.c | 6 +--- >>> example/ipsec/odp_ipsec_fwd_db.h | 1 - >>> example/ipsec/odp_ipsec_loop_db.h | 1 - >>> example/ipsec/odp_ipsec_misc.h | 4 +-- >>> example/ipsec/odp_ipsec_stream.c | 4 +-- >>> example/l2fwd_simple/odp_l2fwd_simple.c | 4 +-- >>> example/l3fwd/odp_l3fwd.c | 6 +--- >>> example/l3fwd/odp_l3fwd_db.h | 2 +- >>> example/packet/odp_pktio.c | 4 +-- >>> example/switch/odp_switch.c | 4 +-- >>> example/time/time_global_test.c | 2 +- >>> example/timer/odp_timer_test.c | 2 +- >>> helper/Makefile.am | 11 +++--- >>> helper/cuckootable.c | 2 +- >>> helper/hashtable.c | 2 +- >>> helper/include/odp/helper/odph_api.h | 39 ++++++++++++++++++++++ >>> helper/{ => include/odp/helper}/odph_cuckootable.h | 0 >>> helper/{ => include/odp/helper}/odph_hashtable.h | 0 >>> .../{ => include/odp/helper}/odph_iplookuptable.h | 0 >>> helper/{ => include/odp/helper}/odph_lineartable.h | 0 >>> helper/iplookuptable.c | 2 +- >>> helper/lineartable.c | 2 +- >>> helper/test/chksum.c | 4 +-- >>> helper/test/cuckootable.c | 2 +- >>> helper/test/iplookuptable.c | 2 +- >>> helper/test/odpthreads.c | 2 +- >>> helper/test/parse.c | 3 +- >>> helper/test/table.c | 3 +- >>> test/common_plat/common/odp_cunit_common.c | 2 +- >>> test/common_plat/performance/odp_crypto.c | 2 +- >>> test/common_plat/performance/odp_l2fwd.c | 4 +-- >>> test/common_plat/performance/odp_pktio_perf.c | 5 +-- >>> test/common_plat/performance/odp_sched_latency.c | 2 +- >>> test/common_plat/performance/odp_scheduling.c | 2 +- >>> .../api/classification/odp_classification_common.c | 4 --- >>> .../classification/odp_classification_test_pmr.c | 4 --- >>> .../api/classification/odp_classification_tests.c | 4 --- >>> .../classification/odp_classification_testsuites.h | 1 + >>> test/common_plat/validation/api/pktio/pktio.c | 4 +-- >>> test/common_plat/validation/api/timer/timer.c | 2 +- >>> .../validation/api/traffic_mngr/traffic_mngr.c | 6 +--- >>> test/linux-generic/mmap_vlan_ins/mmap_vlan_ins.c | 4 +-- >>> test/linux-generic/pktio_ipc/ipc_common.h | 5 +-- >>> test/linux-generic/ring/ring_stress.c | 2 +- >>> 46 files changed, 80 insertions(+), 97 deletions(-) >>> create mode 100644 helper/include/odp/helper/odph_api.h >>> rename helper/{ => include/odp/helper}/odph_cuckootable.h (100%) >>> rename helper/{ => include/odp/helper}/odph_hashtable.h (100%) >>> rename helper/{ => include/odp/helper}/odph_iplookuptable.h (100%) >>> rename helper/{ => include/odp/helper}/odph_lineartable.h (100%) >>> >>> -- >>> 2.9.3