> -----Original Message-----
> From: Christophe Milard [mailto:christophe.mil...@linaro.org]
> Sent: Wednesday, February 15, 2017 11:40 AM
> To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia-bell-
> labs.com>
> Cc: Mike Holmes <mike.hol...@linaro.org>; lng-odp <lng-
> o...@lists.linaro.org>
> Subject: Re: [lng-odp] [PATCH 1/2] helper: linux: renamed threads_extn to
> linux helpers
>
> On 15 February 2017 at 09:34, Savolainen, Petri (Nokia - FI/Espoo)
> <petri.savolai...@nokia-bell-labs.com> wrote:
> >
> >
> >> -----Original Message-----
> >> From: Christophe Milard [mailto:christophe.mil...@linaro.org]
> >> Sent: Monday, February 13, 2017 5:41 PM
> >> To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia-bell-
> >> labs.com>
> >> Cc: Mike Holmes <mike.hol...@linaro.org>; lng-odp <lng-
> >> o...@lists.linaro.org>
> >> Subject: Re: [lng-odp] [PATCH 1/2] helper: linux: renamed threads_extn
> to
> >> linux helpers
> >>
> >> This is getting very confusing now: why helper/<OS>? do we have
> >> odp/<OS>?: shouldn't these be symetric?
> >
> >
> > Application (like OFP today) which wants to use e.g. Linux pthread
> helpers include:
> >
> > #include <odp_api.h>
> > #include <odp/helper/linux/pthread.h>
> >
> > odph_linux_pthread_create()
> > ...
> > odph_linux_pthread_join()
> >
> >
> > It explicitly uses Linux pthreads through a helper function. There's no
> need to provide these helpers and functions on a non-Linux implementation
> (because there is no Linux).
> >
> > Linux helpers are totally different thing than ODP implementation
> internal directory structure.
>
> Different: Yes. But related:
> We are back, sadly, to my first question: "what is a platform".
> To that, you, Petri, answered: "platform == implementation". And I agree.
Linux helpers use Linux API and ODP API calls. Those are not aware how an
implementation is split into files and folders. So, what is platform discussion
does not belong into this thread. This patch fixes the build issue under and
renames --enable-helper-thread-extn to --enable-helper-linux, which is more
logical and explicit about linux helpers.
>
> Now, can we agree that the above platform definition implies:
> different OS => different platform? Or are you saying that a OSE ODP
> and linux ODP are the same implementation???.
Each vendor decide their folder structure. If they re-use ours you can either
find e.g.
odp/platform/nxp-ls, which includes code for all supported OSes, or
odp/platform/odp-nxp-ls-linux
odp/platform/odp-nxp-ls-ose
odp/platform/odp-nxp-ls-vxworks
I really don't care as long as everything builds and tests run for each OS. I'd
prefer platform per vendor + SoC family, so that in the *unlikely case* that
everything is installed into the same directory you would see
odp/platform/linux-generic
odp/platform/odp-dpdk
odp/platform/odp-nxp-ls
odp/platform/odp-cavium-thunder
...
But, as said every vendor organize their repo as they like.
>
> If we can agree that, for ODP implementation, different OSes means
> different platforms, do you feel comfortable calling these variation
> differently for the helpers?
Repo/directory structure is own by vendor/implementation/platform. Helpers
depend on APIs. There's no direct link. Helpers are sorted based on the API(s)
they depend on.
>
> What if some helper function requires to be different for a given OS?
> Say for instance, some company wants to implement ODP threads using
> libdill or libmill? (on linux). Or some helper function would depend
> on memory available on a given HW?
odp/helper/linux/pthread.h
odp/helper/linux/dill.h
odp/helper/linux/mill.h
odp/helper/ose/interrupt.h
odp/helper/ose/xyz.h
Helper code uses ODP API exactly the same way as application does: it uses
capability APIs if it's concerned about some capability. Helper code is part of
application code.
>
> Yes, ODP implementation and helpers are 2 differents things, but they
> are facing the same issue: They try to implement a given interface
> (the ODP API for ODP and the helper API for the helpers) on different
> HW, OS, ... I'd say on different platform. I cannot see why helpers
> variants would only be limited to OSes.
No helpers do not have different implementation for different ODP
implementation.
Even the "abstract threads" implement (today) abstraction over linux pthread
and process, but not e.g. OSE threads. You'd need to add some #ifdef __OSE
there to enable OSE support. Maybe first add odp/helper/ose/thread.c and then
#ifdef abtract thread to call those functions if application (and helpers) are
built against OSE (instead of Linux).
>
> If we can agree on the above, we should agree that a common solution
> to tackle the same problem would be nice.
> On the ODP implementation side, we agreed that a platform was just a
> way to diverge for the main delivery.
> I still think it makes sense to have the same approach for the helpers:
> If we deliver ODP implementation "linux-gen" and "ODP-dpdk" we should
> deliver 2 variant of helper for these as well (even if they happen to
> be the same).
No, since helper code should be always the same.
Pthread system calls in Linux work the same way, regardless if it's called with
odp-linux or odp-dpdk lib installed.
-Petri
>
> A constructor willing to have his own ODP (whatever the reason) would
> have the framework to diverge: a divergence is a new platform. Both
> for helpers and ODP
> The interesting question is how do we write the code so that these
> divergence would be easy to do and so that maximum code can be reuse.
> That is where the next challenge is. But first we need to agree on
> what a platform is and how we handle helpers.
>
> Christophe.
>
> >
> >
> >>
> >> My view is getting clearer and clearer:
> >> On the test side, was have made tests for {OS, HW}= {linux/PC} and
> >> given the possibility to diverge from these (for any reason, e.g.
> >> difference of HW or OS) with the use of "platform":
> >> If a test defined on the common validation test is inappropriate for a
> >> ODP developer (whatever the reason), that developer can overwrite this
> >> test (or part of it, such as its setup) with an appropriate one on the
> >> platform side.
> >> Kallray, AFAIK, if using the platform trick for both OS and HW
> divergence.
> >
> > If the system under test is not a Linux system. You cannot run linux
> helper code there and thus you just skip the linux helper tests.
> >
> >
> >>
> >> Why not the same approach on the helper side? We give helpers for
> >> {linux/PC} and provide a way to redefine these function for those who
> >> need to, on the platform side (e.g. using a weak symbol on the common
> >> helpers and letting those who need define the same function as
> >> "strong" on the helper platform side?... or any other better trick).
> >
> > Why a vendor which do not support Linux would rewrite e.g.
> odph_linux_pthread_create() to emulate Linux pthreads? If the system is
> not Linux, there is no point to run Linux system calls there and
> application would not call odph_linux_pthread_create() there.
> >
> > Remember, helper code is application code. Application would not call
> Linux system calls in an non-Linux system. So, no need for a non-Linux
> based ODP implementation to emulate any Linux system calls.
> >
> >
> >>
> >> Then, any implementer knows where to place their hacks: on the
> >> "platform side". Maybe not best, but at least consistent and clear.
> >>
> >> That would just give helpers, with different platform (for any reason)
> >> implementations.
> >>
> >> The linux-specific helper would of course not fit on the common helper
> >> part as they are linux specific and cannot be implementation on
> >> different OS (platform). Once again, I don't understand why we needed
> >> to do wrappers around well known Linux system calls: IMHO, helpers
> >> should contains functions that we expect to be delivered on any
> >> implementation (OS/HW) helpers, so these linux wrappers don't fit
> >> there.
> >
> > Linux helpers can be used in Linux based systems - may be 90 - 95% of
> all data plane systems today support or can support Linux.
> >
> > Non-Linux systems do not need to run Linux applications (helpers).
> >
> >
> >> If we really want to keep that (I'd vote against), let's create
> >> another directory name for what it is: "linux-wrappers"... and
> >> separate them from function we'd expect to see everywhere (the rest of
> >> the helpers, even if different platform dependent implementations for
> >> those might exist)
> >
> > This patch uses odp/helper/linux/xx.h which is pretty explicit about the
> dependency to Linux. Anything under odp/helper/linux is Linux dependent
> and would not build/run on non-Linux systems. Clear?
> >
> >
> > -Petri
> >
> >
> >
> >
> >
> >>
> >> Christophe
> >>
> >> On 13 February 2017 at 16:09, Savolainen, Petri (Nokia - FI/Espoo)
> >> <petri.savolai...@nokia-bell-labs.com> wrote:
> >> >
> >> >
> >> > From: Mike Holmes [mailto:mike.hol...@linaro.org]
> >> > Sent: Monday, February 13, 2017 5:02 PM
> >> > To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia-
> bell-
> >> labs.com>
> >> > Cc: lng-odp <lng-odp@lists.linaro.org>
> >> > Subject: Re: [lng-odp] [PATCH 1/2] helper: linux: renamed
> threads_extn
> >> to linux helpers
> >> >
> >> >
> >> >
> >> > On 13 February 2017 at 09:41, Savolainen, Petri (Nokia - FI/Espoo)
> >> <mailto:petri.savolai...@nokia-bell-labs.com> wrote:
> >> >
> >> >
> >> > From: Mike Holmes [mailto:mailto:mike.hol...@linaro.org]
> >> > Sent: Friday, February 10, 2017 5:02 PM
> >> > To: Petri Savolainen <mailto:petri.savolai...@linaro.org>
> >> > Cc: lng-odp <mailto:lng-odp@lists.linaro.org>
> >> > Subject: Re: [lng-odp] [PATCH 1/2] helper: linux: renamed
> threads_extn
> >> to linux helpers
> >> >
> >> >
> >> >
> >> > On 3 February 2017 at 06:23, Petri Savolainen
> >> <mailto:mailto:petri.savolai...@linaro.org> wrote:
> >> > There's no platform specific helpers. Helpers may depend on
> >> > Linux and make it easier to do common series of Linux system
> >> > calls. These kind of helpers are grouped into helper/linux
> >> > directory.
> >> >
> >> > Use --enable-helper-linux configuration option to enable
> >> > support for Linux helpers.
> >> >
> >> > Signed-off-by: Petri Savolainen
> >> <mailto:mailto:petri.savolai...@linaro.org>
> >> >
> >> > Reviewed-by: Mike Holmes <mailto:mailto:mike.hol...@linaro.org>
> >> >
> >> > required 3 way apply
> >> >
> >> >
> >> >
> >> > This should be merged. It moves linux dependent helper code under
> linux
> >> folder. If there would be helper code dependent on some other OS, that
> >> would go to their own helper/OS_foo/ folder. All Linux based ODP
> >> implementations can run this helper code - there is no need to have N
> >> different linux helpers for N different Linux based implementations.
> >> >
> >> >
> >> > Also, the build is currently broken in master for --enable-helper-
> extn.
> >> E.g. OFP cannot be built against it.
> >> >
> >> > This is not broken IMHO, it is by design, OFP depends on the helpers,
> we
> >> need helpers to either be support for apps that we maintain and don't
> use
> >> (current case) or we enforce OFP and others to use the abstract API
> that
> >> all our own executable use. Maintaining code we don't use is dangerous,
> >> you should have to choose to do it, but I like the rename, that is fine
> >> and is progress, hence the review.
> >> >
> >> > I think splitting out all of all the executables will make this much
> >> cleaner.
> >> > All ODP upstream executables will use the abstract API and be
> portable
> >> to any platform and OS. Meanwhile the odp-Linux repo can have a Linux
> >> specific apis such as the legacy one OFP is using and that is just that
> >> implementations choice, as we have said there is no requirement to use
> >> helpers, or linux, and OFP might want to attach itself to that, but
> the
> >> tests and examples must remain agnostic until we state we are dropping
> >> that goal.
> >> >
> >> >
> >> > Please try:
> >> > ./configure --enable-helper-extn
> >> > make
> >> >
> >> > It does not compile, due to:
> >> >
> >> > platform/linux-generic/thread.c:282:12: error:
> >> \u2018odph_linux_thread_create\u2019 defined but not used [-
> Werror=unused-
> >> function]
> >> > static int odph_linux_thread_create(odph_odpthread_t *thread_tbl,
> >> >
> >> >
> >> > My patch fixes this also.
> >> >
> >> >
> >> > -Petri
> >> >
> >> >
