Re: [dpdk-dev] [PATCH] app/testpmd: fix testpmd exit using ctrl+d

["Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>]

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Wednesday, July 25, 2018 1:18 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Mordechay
> Haimovsky <motih@mellanox.com>; Thomas Monjalon
> <thomas@monjalon.net>; Singh, Jasvinder <jasvinder.singh@intel.com>
> Cc: dev@dpdk.org; Iremonger, Bernard <bernard.iremonger@intel.com>;
> Pattan, Reshma <reshma.pattan@intel.com>; olivier.matz@6wind.com; Van
> Haaren, Harry <harry.van.haaren@intel.com>
> Subject: RE: [dpdk-dev] [PATCH] app/testpmd: fix testpmd exit using ctrl+d
> 
> 
> 
> > -----Original Message-----
> > From: Dumitrescu, Cristian
> > Sent: Wednesday, July 25, 2018 12:57 PM
> > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Mordechay
> Haimovsky <motih@mellanox.com>; Thomas Monjalon
> > <thomas@monjalon.net>; Singh, Jasvinder <jasvinder.singh@intel.com>
> > Cc: dev@dpdk.org; Iremonger, Bernard <bernard.iremonger@intel.com>;
> Pattan, Reshma <reshma.pattan@intel.com>;
> > olivier.matz@6wind.com; Van Haaren, Harry
> <harry.van.haaren@intel.com>
> > Subject: RE: [dpdk-dev] [PATCH] app/testpmd: fix testpmd exit using ctrl+d
> >
> >
> >
> > > -----Original Message-----
> > > From: Ananyev, Konstantin
> > > Sent: Wednesday, July 25, 2018 12:39 PM
> > > To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Mordechay
> > > Haimovsky <motih@mellanox.com>; Thomas Monjalon
> > > <thomas@monjalon.net>; Singh, Jasvinder <jasvinder.singh@intel.com>
> > > Cc: dev@dpdk.org; Iremonger, Bernard <bernard.iremonger@intel.com>;
> > > Pattan, Reshma <reshma.pattan@intel.com>; olivier.matz@6wind.com;
> Van
> > > Haaren, Harry <harry.van.haaren@intel.com>
> > > Subject: RE: [dpdk-dev] [PATCH] app/testpmd: fix testpmd exit using
> ctrl+d
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Dumitrescu, Cristian
> > > > Sent: Wednesday, July 25, 2018 10:36 AM
> > > > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Mordechay
> > > Haimovsky <motih@mellanox.com>; Thomas Monjalon
> > > > <thomas@monjalon.net>; Singh, Jasvinder
> <jasvinder.singh@intel.com>
> > > > Cc: dev@dpdk.org; Iremonger, Bernard
> <bernard.iremonger@intel.com>;
> > > Pattan, Reshma <reshma.pattan@intel.com>;
> > > > olivier.matz@6wind.com
> > > > Subject: RE: [dpdk-dev] [PATCH] app/testpmd: fix testpmd exit using
> ctrl+d
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Ananyev, Konstantin
> > > > > Sent: Wednesday, July 25, 2018 10:04 AM
> > > > > To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>;
> Mordechay
> > > > > Haimovsky <motih@mellanox.com>; Thomas Monjalon
> > > > > <thomas@monjalon.net>; Singh, Jasvinder
> <jasvinder.singh@intel.com>
> > > > > Cc: dev@dpdk.org; Iremonger, Bernard
> <bernard.iremonger@intel.com>;
> > > > > Pattan, Reshma <reshma.pattan@intel.com>;
> olivier.matz@6wind.com
> > > > > Subject: RE: [dpdk-dev] [PATCH] app/testpmd: fix testpmd exit using
> > > ctrl+d
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of
> Dumitrescu,
> > > > > Cristian
> > > > > > Sent: Tuesday, July 24, 2018 5:59 PM
> > > > > > To: Mordechay Haimovsky <motih@mellanox.com>; Thomas
> Monjalon
> > > > > <thomas@monjalon.net>; Singh, Jasvinder
> > > > > > <jasvinder.singh@intel.com>
> > > > > > Cc: dev@dpdk.org; Iremonger, Bernard
> > > <bernard.iremonger@intel.com>;
> > > > > Pattan, Reshma <reshma.pattan@intel.com>;
> > > > > > olivier.matz@6wind.com
> > > > > > Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix testpmd exit
> using
> > > ctrl+d
> > > > > >
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Mordechay Haimovsky [mailto:motih@mellanox.com]
> > > > > > > Sent: Tuesday, July 24, 2018 3:37 PM
> > > > > > > To: Thomas Monjalon <thomas@monjalon.net>; Singh, Jasvinder
> > > > > > > <jasvinder.singh@intel.com>
> > > > > > > Cc: dev@dpdk.org; Iremonger, Bernard
> > > <bernard.iremonger@intel.com>;
> > > > > > > Pattan, Reshma <reshma.pattan@intel.com>;
> > > olivier.matz@6wind.com;
> > > > > > > Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> > > > > > > Subject: RE: [dpdk-dev] [PATCH] app/testpmd: fix testpmd exit
> using
> > > > > ctrl+d
> > > > > > >
> > > > > > > Even after this fix we still have setups that use netvsc  for
> example,
> > > on
> > > > > > > which testpmd exits with rte_panic right after loading it even
> > > without
> > > > > > > touching the KBD.
> > > > > > >
> > > > > > > I recommend returning the previous prompt routine in test-
> > > > > pmd/cmdline.c
> > > > > > > and rework the SOFTNIC section there, preferably moving its poll
> > > section
> > > > > to
> > > > > > > use rte_service in a separate file cleaning the CLI files from PMD-
> > > specific
> > > > > > > implementation.
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > > > > Sent: Tuesday, July 24, 2018 3:34 PM
> > > > > > > > To: Jasvinder Singh <jasvinder.singh@intel.com>
> > > > > > > > Cc: dev@dpdk.org; bernard.iremonger@intel.com;
> > > > > > > > reshma.pattan@intel.com; Mordechay Haimovsky
> > > > > > > <motih@mellanox.com>;
> > > > > > > > olivier.matz@6wind.com; cristian.dumitrescu@intel.com
> > > > > > > > Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix testpmd exit
> > > using
> > > > > ctrl+d
> > > > > > > >
> > > > > > > > Important note:
> > > > > > > > 	testpmd is currently really broken.
> > > > > > > > 	We cannot have a RC2 until it is fixed.
> > > > > > > >
> > > > > > > >
> > > > > > > > 24/07/2018 13:25, Thomas Monjalon:
> > > > > > > > > 23/07/2018 12:44, Jasvinder Singh:
> > > > > > > > > > Fix testpmd app exit by pressing ctrl+d, End-of-Transmission
> > > > > > > > > > character (EOT) on the empty command line.
> > > > > > > > >
> > > > > > > > > Please describe what is the issue and what is the cause.
> > > > > > > > >
> > > > > > > > > > Fixes: 0ad778b398c6 ("app/testpmd: rework softnic forward
> > > > > mode")
> > > > > > > > > >
> > > > > > > > > > Reported-by: Mordechay Haimovsky
> <motih@mellanox.com>
> > > > > > > > > > Signed-off-by: Jasvinder Singh <jasvinder.singh@intel.com>
> > > > > > > > > > ---
> > > > > > > > > >  app/test-pmd/cmdline.c       | 10 ++++++----
> > > > > > > > > >  lib/librte_cmdline/cmdline.c |  2 +-
> > > > > > > > >
> > > > > > > > > It is very suspicious to change the cmdline library.
> > > > > > > > > If there is a bug in this library, it deserves a separate fix.
> > > > > > > >
> > > > > > > >
> > > > > >
> > > > > >
> > > > > > First, testpmd is not really broken, as only thing that changed is the
> Ctrl
> > > +
> > > > > D behavior. I agree this is an issue that we need to fix, as
> > > > > > it looks that it is breaking some automation scripts for some
> people.
> > > > > >
> > > > > > The change in behavior for Ctrl + D exit is caused by replacing the
> call
> > > to
> > > > > cmdline_interact() with calling cmdline_poll() in a loop.
> > > > > > These two approaches should be identical in behavior, but it looks
> like
> > > they
> > > > > are not due to some issue in the cmdline library.
> > > > > > Jasvinder proposed a quick patch, but it looks like something else
> needs
> > > to
> > > > > be fixed in cmdline library in order to bring
> > > > > > cmdline_poll() on parity with cmdline_interact(). Any advice from
> > > Olivier
> > > > > would be very much appreciated!
> > > > > >
> > > > > > It is really a bad practice to use cmdline_interact() in testpmd, as it
> is a
> > > > > blocking call that prohibits doing other things on the same
> > > > > > thread that runs the CLI. Sometimes we need to run other things on
> the
> > > > > same core, e.g. the slow softnic_manage() function.
> > > > >
> > > > > Curious why not use a service core for softnic background stuff, and
> leave
> > > CLI
> > > > > one for CLI?
> > > > > Konstantin
> > > >
> > > > I guess for a test application you can do anything you want, but in real
> life
> > > CPU cores are really expensive and dedicating a CPU core
> > > > just for CLI is a colossal waste.
> > >
> > > Ok, but let's application developer to decide how to use (waste) the
> cores he
> > > owns :)
> > > What I am saying: there is a special thing (developed by Harry) service
> cores.
> > > As I understand why of it's the purpose - allow PMD(s) to allocate CPU
> > > resources for
> > > there background tasks in a unified and transparent way.
> > > From the description above - that seems to fit your needs (softnic
> > > background processing), no?
> > > Konstantin
> > >
> >
> > Then why not put the testpmd CLI itself on a service core? Are you
> volunteering for a patch on this? :)
> 
> It was not me who broke testpmd at first place.

I don't think you understand the issue. The issue is related to using the cmdline library in non-blocking mode, it has nothing to do with Soft NIC.

> I'd better live without softnic support in testpmd :)

Thanks a lot for your help ;(

> Though I still don't understand why do you feel that service cores are not
> good enough for you?

You don't seem to realize there is an issue that we need to root cause and hopefully fix rather than hiding it. This has nothing to do with service core.

> From my understanding they were introduced for similar purposes
> (Harry please feel free to correct me here).
> So if you think they not fit for your case -
> at least would be good to understand why.

Service core could be used here, as long as a new core is not wasted. Running this on the same core as CLI is also a valid solution.

> Konstantin
> 
> >
> >
> > > >
> > > > We did use the non-blocking cmdline_poll() function instead of the
> > > blocking cmdline_interact() in the past without any issues. The
> > > > issues reported by Moti come as a surprise. It is probably good to see
> what
> > > this is about and see if we can quickly fix the issue in
> > > > cmdline library. Otherwise, we can revert the usage of cmdline_poll()
> with
> > > cmdline_interact().
> > > >
> > > >
> > > > >
> > > > > >
> > > > > > Worst case scenario: We can revert the cmdline_poll() back to
> > > > > cmdline_interact(), this is a small change, but not the proper way of
> > > > > > doing things, as this is simply hiding the issue in cmdline library. It
> > > would
> > > > > also prevent us from testing some Soft NIC functionality.