Re: [dpdk-dev] [PATCH] testpmd: add nanosleep in main loop

[Marcelo Tosatti <mtosatti@redhat.com>]

On Sun, Nov 12, 2017 at 11:14:35PM +0000, Ananyev, Konstantin wrote:
> 
> 
> > -----Original Message-----
> > From: Marcelo Tosatti [mailto:mtosatti@redhat.com]
> > Sent: Saturday, November 11, 2017 3:50 AM
> > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> > Cc: Adrien Mazarguil <adrien.mazarguil@6wind.com>; dev@dpdk.org; Luiz Capitulino <lcapitulino@redhat.com>; Daniel Bristot de Oliveira
> > <bristot@redhat.com>
> > Subject: Re: [dpdk-dev] [PATCH] testpmd: add nanosleep in main loop
> > 
> > On Fri, Nov 10, 2017 at 10:14:23AM +0000, Ananyev, Konstantin wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Adrien Mazarguil
> > > > Sent: Friday, November 10, 2017 9:12 AM
> > > > To: Marcelo Tosatti <mtosatti@redhat.com>
> > > > Cc: dev@dpdk.org; Luiz Capitulino <lcapitulino@redhat.com>; Daniel Bristot de Oliveira <bristot@redhat.com>
> > > > Subject: Re: [dpdk-dev] [PATCH] testpmd: add nanosleep in main loop
> > > >
> > > > Hi Marcelo,
> > > >
> > > > On Fri, Nov 10, 2017 at 04:02:10AM -0200, Marcelo Tosatti wrote:
> > > > >
> > > > > This patch allows a configurable pair of values to be set, which
> > > > > controls
> > > > > the frequency and length of a nanosleep call performed at test-pmd's
> > > > > iofwd main loop.
> > > > >
> > > > > The problem is the following: it is necessary to execute code
> > > > > on isolated CPUs which is not part of the packet forwarding load.
> > > > >
> > > > > For example:
> > > > >
> > > > >  "echo val > /sys/kernel/debug/tracing/buffer_size_kb"
> > > > >
> > > > > hangs the process, because the DPDK thread has higher
> > > > > priority than the workqueue thread which executes the flush from
> > > > > CPU local tracebuffer to CPU global trace buffer [the workitem
> > > > > in case].
> > > > >
> > > > > There are more serious issues than the trace-cmd bug, such as XFS
> > > > > workitems failing to execute causing filesystem corruption.
> > > > >
> > > > > To workaround this problem, until a proper kernel
> > > > > solution is developed, allow DPDK to nanosleep
> > > > > (hopefully with a small enough frequency and interval
> > > > > so that the performance is within acceptable levels).
> > > >
> > > > I understand the need to do something about it, however the nanosleep()
> > > > approach seems questionable to me.
> > > >
> > > > Testpmd's forwarding modes (particularly I/O) are used for benchmarking
> > > > purposes by many and are therefore sensitive to change. This code path is
> > > > currently free from system calls for that reason and nanosleep() is an
> > > > expensive one by definition. Even if optional or called at a low frequency,
> > > > the presence of this new code has an impact.
> > > >
> > > > Since testpmd is a development tool not supposed to run in a production
> > > > environment, is there really a need for it to be patched to work around a
> > > > (temporary) Linux kernel bug?
> > > >
> > > > If so, why is I/O the only forwarding mode impacted?
> > > >
> > > > If it's used in a production environment and such a fix can't wait, have
> > > > other workarounds been considered:
> > > >
> > > > - Replacing testpmd in I/O mode with a physical cable or switch?
> > > >
> > > > - Using proper options on the kernel command line as described in [1], such
> > > >   as isolcpus, rcu_nocbs, nohz_full?
> > > >
> > > > [1] doc/guides/howto/pvp_reference_benchmark.rst
> > >
> > >
> > > Agree with Adrian here - the patch doesn't fix the problem in any case,
> > 
> > It does fix the problem as the original message describes the testing.
> 
> If the user will run testpmd with different fwd mode
> (macfwd, csum, txonly, etc.) - he would hit exactly the same problem.
> If the user would run any other of DPDK sample applications (l2fwd, l3fwd, etc.) -
> he would hit the same problem again.
> If some of DPDK customers have a busy loop inside their production system -
> they would hit that problem too.

Yes. Are you suggesting any improvement to the patch?

> As I understand - that problem is even not DPDK related - any application that uses
> busy loop inside can be affected.
> Correct?

Yes.

> So I think the patch doesn't fix the problem, all it does - helps to avoid
> particular manifestation of it.

To fix the problem in l3fwd, the same logic must be applied to l3fwd.
To fix the problem in DPDK customer app busy loop, the application must
be fixed.

As long as the same logic is applied to the particular application,
then that application is _fixed_.

> BTW, if it is a generic kernel problem - I suppose there should be some 
> record in kernel bugzilla to track it, right?
> If so, could you probably provide some reference to it? 

It is debatable.

> >From other side - testpmd is not a production app - 
> as the name implies it is a tool to test PMDs functionality and performance.
> Specially iofwd  is sort of synthetic benchmark that allows to measure
> highest possible PMD performance.
> That's why I think many people (and me too) would prefer to keep it intact
> and free from any system calls.

It is free from system calls. The system calls are only enabled if 

> If you are that desperate to provide some workaround sepcially for testpmd -
> my suggestion would be to introduce new fwd mode here that would call
> nanosleep() periodically, while keeping original iofwd mode intact. 

This is what the patch does already. Original iofwd mode is intact.

> > > while introducing an unnecessary slowdown in testpmd iofwd mode.
> > 
> > It is not unnecessary: it is a mandatory slowdown for any approach
> > which fixes the problem, whether it's in DPDK or not.
> 
> dpdk runs on other OSes too (freebsd).
> For non-linux users it definetly looks like an unnecessary one.

I bet FreeBSD also has threads and if configured to work in 
fully isolated mode, 

> 
> > 
> > > Please think up some other approach.
> > > Konstantin
> > 
> > What characteristics are you looking for in "some other approach"?
> > That DPDK is not interrupted? Impossible.
> 
> Even if it has to be interrupted - why this can't be done transparently to the user?
> Via some high-priority kernel thread/interrupt or so?  

It can be. 


> > See, the thing here is that nanosleep length and frequency are
> > controllable, which allows an application developer to tune the values
> > _and_ still meet their performance metrics.
> 
> Honestly, I can't see how you can force each and every application developer
> to start injecting nanosleep() into every busy loop inside their applications.
> Not to mention already existing legacy apps.
> 
> Konstantin

Ok, the example has been posted, it might be useful for someone.