Re: [dpdk-dev] [PATCH v3 6/9] eal: register non-EAL threads as lcores

[Thomas Monjalon <thomas@monjalon.net>]

02/07/2020 15:06, David Marchand:
> On Wed, Jul 1, 2020 at 1:58 PM Ananyev, Konstantin
> <konstantin.ananyev@intel.com> wrote:
> > > If the final users finally hit the situation you describe, it means
> > > that the multiprocess had been in use so far and was known to be in
> > > use (*hopefully*).
> >
> > Yes.
> >
> > > So is it not a problem of design/non-regression testing when
> > > integrating the new API in the first place?
> >
> > Not sure I understand you here...
> > If you saying that for SP benchmarking/testing current approach
> > is sufficient, then - yes it is.
> > Or are you saying it would be hard to create a test-case to
> > reproduce such problematic scenario?
> 
> I am saying that getting to a problematic scenario that only the final
> users get, would be a failure in the development, documentation and
> validation of the application.
> 
> When the developer integrates this new API, the developer will read
> the API description.
> 
> - If the limitation on mp is understood and accepted, the application
> documentation will be updated to reflect this.
> Users can then know mp is not available.
> If the users still try to use it, it can be a support issue.
> The users will then report to support people who should be aware this
> is not supported (the documentation says so).
> 
> - If the application needs mp support because of X, Y reasons, the
> developer integrating the new API, should complain upstream that the
> new API requires mp support.
> If the developer does not complain but still uses the API.. well too
> bad (or it falls through to the following point).
> 
> - The application needs mp support, the developer did not catch the
> warning in the API (the kids are home, hard to concentrate)...
> The new API will be used for datapath processing threads, so non
> regression perf tests will be run.
> On the other hand, the application uses mp for X, Y reasons, so there
> will be associated test cases.
> I can't tell for sure, but I find it hard to believe a validation team
> would never do tests that combine both.

Please let's conclude.

The proposed API for thread registration does not support multi-process.
This limitation is documented and there are some runtime checks.
If this API is used in an application design, it should be clear
that attaching a secondary process is forbidden.
If a user tries to attach a secondary process before the application
registers a thread, then future thread registration will fail.
If a user tries to attach a secondary process after a thread registration,
then the secondary process will fail.
It means that depending on when the user *wrongly* attach a secondary
process (despite the documented limitation of the application),
the failure will happen in a different context.
I think it is OK.

The alternative is to introduce a new EAL flag or a new API to make sure
the failure will happen when attaching a secondary process,
even if no thread is registered yet.
I think adding such addition would be weird from user or API perspective.

Please note that accepting the thread registration API does not prevent
from adding an EAL flag or a new API later.
That's why I vote for merging this series.

Acked-by: Thomas Monjalon <thomas@monjalon.net>