DPDK patches and discussions
 help / color / mirror / Atom feed
From: Jerin Jacob <jerinjacobk@gmail.com>
To: "Burakov, Anatoly" <anatoly.burakov@intel.com>
Cc: Harman Kalra <hkalra@marvell.com>, dpdk-dev <dev@dpdk.org>,
	David Hunt <david.hunt@intel.com>,
	 Liang Ma <liang.j.ma@intel.com>,
	Reshma Pattan <reshma.pattan@intel.com>
Subject: Re: [dpdk-dev] [EXT] Re: [PATCH 3/3] l3fwd-power: add interrupt-only mode
Date: Mon, 15 Jun 2020 21:59:14 +0530
Message-ID: <CALBAE1NmkfbLSzH5ymCeF9YHDHb79LA-JCW3kqexHrfs7-4+ig@mail.gmail.com> (raw)
In-Reply-To: <c8a90a13-9a51-f271-2649-f398659a6084@intel.com>

On Mon, Jun 15, 2020 at 9:15 PM Burakov, Anatoly
<anatoly.burakov@intel.com> wrote:
>
> On 15-Jun-20 4:21 PM, Jerin Jacob wrote:
> > On Mon, Jun 15, 2020 at 8:35 PM Burakov, Anatoly
> > <anatoly.burakov@intel.com> wrote:
> >>
> >> On 15-Jun-20 12:43 PM, Jerin Jacob wrote:
> >>> On Mon, Jun 15, 2020 at 5:02 PM Burakov, Anatoly
> >>> <anatoly.burakov@intel.com> wrote:
> >>>>
> >>>> On 02-Jun-20 1:16 PM, Harman Kalra wrote:
> >>>>> On Tue, Jun 02, 2020 at 03:53:07PM +0530, Harman Kalra wrote:
> >>>>>> On Mon, Jun 01, 2020 at 01:50:26PM +0100, Burakov, Anatoly wrote:
> >>>>>>> On 30-May-20 11:02 AM, Harman Kalra wrote:
> >>>>>>>> On Fri, May 29, 2020 at 03:19:45PM +0100, Burakov, Anatoly wrote:
> >>>>>>>>> External Email
> >>>>>>>>>
> >>>>>>>>> ----------------------------------------------------------------------
> >>>>>>>>> On 29-May-20 2:19 PM, Harman Kalra wrote:
> >>>>>>>>>
> >>>>>>>>>>>           if (ret < 0)
> >>>>>>>>>>>                   rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n");
> >>>>>>>>>>> -       if (app_mode != APP_MODE_TELEMETRY && init_power_library())
> >>>>>>>>>>> +       if (app_mode == APP_MODE_DEFAULT)
> >>>>>>>>>>> +               app_mode = APP_MODE_LEGACY;
> >>>>>>>>>>> +
> >>>>>>>>>>> +       /* only legacy and empty poll mode rely on power library */
> >>>>>>>>>>> +       if ((app_mode == APP_MODE_LEGACY || app_mode == APP_MODE_EMPTY_POLL) &&
> >>>>>>>>>>> +                       init_power_library())
> >>>>>>>>>>>                   rte_exit(EXIT_FAILURE, "init_power_library failed\n");
> >>>>>>>>>> Hi,
> >>>>>>>>>>
> >>>>>>>>>> Rather than just exiting from here can we have a else condition to
> >>>>>>>>>> automatically enter into the "interrupt only" mode.
> >>>>>>>>>> Please correct me if I am missing something.
> >>>>>>>>>
> >>>>>>>>> Hi,
> >>>>>>>>>
> >>>>>>>>> Thanks for your review. I don't think silently proceeding is a good idea. If
> >>>>>>>>> the user wants interrupt-only mode, they should request it. Silently falling
> >>>>>>>>> back to interrupt-only mode will create an illusion of successful
> >>>>>>>>> initialization and set the wrong expectation for how the application will
> >>>>>>>>> behave.
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> Hi,
> >>>>>>>>
> >>>>>>>> Thanks for the explanation which even I also believe is logically perfect.
> >>>>>>>>
> >>>>>>>> But since l3fwd-power is an old application and has many users around
> >>>>>>>> which are currently using this app in interrupt only mode without giving
> >>>>>>>> an extra argument. But suddenly they will start getting failure messages with
> >>>>>>>> the new patchset.
> >>>>>>>>
> >>>>>>>> My only intent with else condition was backward compatibility.
> >>>>>>>> Or may be we can have more descriptive failure message, something like
> >>>>>>>> "init_power_library failed, check manual for other modes".
> >>>>>>>>
> >>>>>>>> Thanks
> >>>>>>>> Harman
> >>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>>> I think we can compormise on an informative log message suggesting to use
> >>>>>>> interrupt mode. I'm not keen on reverting to previous buggy behavior :)
> >>>>>>>
> >>>>>> Hi
> >>>>>>
> >>>>>> I am not insisting to revert to previous behavior, I am just trying to
> >>>>>> highlight some probable issues that many users might face as its an old
> >>>>>> application.
> >>>>>> Since many arm based soc might not be supporting frequency scaling, can
> >>>>>> we add the following check as soon as the application starts, probe the
> >>>>>> platform if it supports frequency scaling, if not automatically set the
> >>>>>> mode to interrupt mode, something like:
> >>>>>> if (access("/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor",
> >>>>>>                        F_OK))
> >>>>>>        app_mode = APP_MODE_INTERRUPT;
> >>>>>
> >>>>> Sorry, no direct check in application but we can introduce a new API in
> >>>>> power library:
> >>>>>       bool rte_is_freq_scaling() {
> >>>>>            return  access("/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor",
> >>>>>                            F_OK);
> >>>>>       }
> >>>>>
> >>>>> and in the application we can use "rte_is_freq_scaling()" at the start.
> >>>>>
> >>>>
> >>>> What you're suggesting here is effectively what you have already
> >>>> suggested: silently fall back to interrupt-only mode if power lib init
> >>>> failed. I already outlined why i don't think it's a good approach.
> >>>
> >>> Is probing "/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor"
> >>> file presence,
> >>> detects the power lib availability . Right?  Not the failure. Right?
> >>> IMO, it make sense to have following case:
> >>> # first check, Is the system is capable of power lib if so use power lib
> >>> # if the system is not capable of using power lib use interrupt mode.
> >>>
> >>> I think, there is difference between system capable of using power lib
> >>> vs power lib not available or power lib failure.
> >>
> >> I am of the opinion that if a test sets up an unrealistic expectation of
> >> how an application should behave, it's a problem with the test, not with
> >> the application.
> >>
> >> If the system is not capable of running with power lib - the application
> >> shouldn't be requested to run in such mode.
> >
> > But with this patch, default proposed mode is power lib without any
> > explicit request.
>
> The default has always been "use the power library". That was always the
> expected behavior, i believe since the very first version of this
> application. In other words, even if the "requested" behavior is not
> requested explicitly, it has always been that implicitly, and this patch
> is *keeping existing behavior*.

See below,
>
> >
> >>
> >> "The application behaved that way before" - yes, it did. It was a bug in
> >> the application, that it allowed users to effectively misuse the
> >> application and use it despite the fact that it was in a half-working
> >> state. This problem has been addressed by 1) not allowing the
> >> application to run in half-working state, and 2) adding a new mode where
> >> the old "expected" behavior is *actually* expected and is "full working
> >> state" now.
> >>
> >> Therefore, all users who were previously misusing the application to do
> >> something it was not designed to do because of a bug in the
> >> implementation, should now fix their usage and use the correct mode -
> >> and such breakage is IMO necessary to call attention to earlier misuse
> >> in the tools, and to correct this usage.
> >>
> >> What bothers me about your suggestion is that it is impossible to fail
> >> the test if the wrong mode was requested (as in, if we request the
> >> power-lib mode on a system that doesn't have freq scaling) - it instead
> >> silently falls back to a mode that is almost guaranteed to work.
> >
> > I agree that it should fail, i.e someone explicitly request,
> > power-lib mode or any mode
> > and it should fail application if the platform we can not do that.
> >
> > My suggestion is all about, what is the default, IMO, if no argument
> > is specified,
> > the application should _probe_ the capability of the platfrom and
> > choose the mode. One can override
> > the probe to select the desired one. In such mode, fail to configure
> > the mode should result in
> > an error.
>
> This would change the default behavior that has always existed with this
> application, and would still be subject to silent failure issue
> *because* older tests may not account for this implied assumption of
> "the application will run no matter what", leading to a possible false
> positive test result.
>
> Now, if the default was "not to run and ask explicitly what mode should
> the user use" - i can see how we could both agree on that. It's not
> unprecedented - l3fwd itself won't run unless you explicitly specify
> core config, so we *could* request additional parameters by default. I
> would've also agreed with the "probe" thing *if it was a new application
> without any pre-existing usages* - but it isn't, and in this case IMO
> this is doubly important because there may be tests out there that *rely
> on a bug* and thus should be explicitly broken and addressed (like the
> internal test we had that uncovered the issue in the first place).
>
> In other words, in the perfect world, i would agree with you :) As it
> stands though, i think that intentionally breaking tests that are
> themselves broken and rely on wrong assumptions is more important than
> keeping them working.

OK. Let's enumerate the pros and cons of both approaches?
Approach a: Auto probe
Approach b: Current patch

Approach a:
+ Application picks up the mode based on platform capability
+ No change in behavior wrt existing l3fwd-power where the platform
has only interrupt support.
(otherwise, It will fail to boot up, the CI etc we need to patch based
on the DPDK version)

I am not sure approach b has pros wrt approach a.

I.e On the x86 platform where freq scaling is present then SHOULD NOT
have any difference in the approach a vs
approach b. ie. Auto probe finds the system is capable of freq scaling
and picks the powerlib. its is win-win case,
I am not sure, What I am missing?








>
> --
> Thanks,
> Anatoly

  reply	other threads:[~2020-06-15 16:29 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-28  9:13 [dpdk-dev] [PATCH 0/3] Add interrupt-only mode to l3fwd-power Anatoly Burakov
2020-05-28  9:13 ` [dpdk-dev] [PATCH 1/3] l3fwd-power: disable interrupts by default Anatoly Burakov
2020-05-28  9:13 ` [dpdk-dev] [PATCH 2/3] l3fwd-power: only allow supported power library envs Anatoly Burakov
2020-05-28  9:13 ` [dpdk-dev] [PATCH 3/3] l3fwd-power: add interrupt-only mode Anatoly Burakov
2020-05-29 13:19   ` Harman Kalra
2020-05-29 14:19     ` Burakov, Anatoly
2020-05-30 10:02       ` [dpdk-dev] [EXT] " Harman Kalra
2020-06-01 12:50         ` Burakov, Anatoly
2020-06-02 10:23           ` Harman Kalra
2020-06-02 12:16             ` Harman Kalra
2020-06-15 11:31               ` Burakov, Anatoly
2020-06-15 11:43                 ` Jerin Jacob
2020-06-15 15:05                   ` Burakov, Anatoly
2020-06-15 15:21                     ` Jerin Jacob
2020-06-15 15:45                       ` Burakov, Anatoly
2020-06-15 16:29                         ` Jerin Jacob [this message]
2020-06-16  9:31                           ` Burakov, Anatoly
2020-06-16 17:09                             ` Jerin Jacob
2020-06-08  1:24 ` [dpdk-dev] [PATCH 0/3] Add interrupt-only mode to l3fwd-power Wang, Yinan
2020-06-18 17:18 ` [dpdk-dev] [PATCH v2 0/7] " Anatoly Burakov
2020-06-19 10:53   ` [dpdk-dev] [PATCH v3 " Anatoly Burakov
2020-07-11 11:35     ` Thomas Monjalon
2020-06-19 10:53   ` [dpdk-dev] [PATCH v3 1/7] l3fwd-power: disable interrupts by default Anatoly Burakov
2020-06-19 10:53   ` [dpdk-dev] [PATCH v3 2/7] l3fwd-power: only allow supported power library envs Anatoly Burakov
2020-06-19 10:53   ` [dpdk-dev] [PATCH v3 3/7] l3fwd-power: code style and flow fixes Anatoly Burakov
2020-06-19 10:53   ` [dpdk-dev] [PATCH v3 4/7] l3fwd-power: add support for requesting legacy mode Anatoly Burakov
2020-06-19 10:53   ` [dpdk-dev] [PATCH v3 5/7] l3fwd-power: add interrupt-only mode Anatoly Burakov
2020-06-19 10:53   ` [dpdk-dev] [PATCH v3 6/7] power: add API to probe support for a specific env Anatoly Burakov
2020-06-19 10:53   ` [dpdk-dev] [PATCH v3 7/7] l3fwd-power: add auto-selection of default mode Anatoly Burakov
2020-07-11 10:07     ` Thomas Monjalon
2020-06-18 17:18 ` [dpdk-dev] [PATCH v2 1/7] l3fwd-power: disable interrupts by default Anatoly Burakov
2020-06-18 17:18 ` [dpdk-dev] [PATCH v2 2/7] l3fwd-power: only allow supported power library envs Anatoly Burakov
2020-06-18 17:18 ` [dpdk-dev] [PATCH v2 3/7] l3fwd-power: code style and flow fixes Anatoly Burakov
2020-06-18 17:18 ` [dpdk-dev] [PATCH v2 4/7] l3fwd-power: add support for requesting legacy mode Anatoly Burakov
2020-06-18 17:18 ` [dpdk-dev] [PATCH v2 5/7] l3fwd-power: add interrupt-only mode Anatoly Burakov
2020-06-18 17:18 ` [dpdk-dev] [PATCH v2 6/7] power: add API to probe support for a specific env Anatoly Burakov
2020-06-18 17:18 ` [dpdk-dev] [PATCH v2 7/7] l3fwd-power: add auto-selection of default mode Anatoly Burakov
2020-06-19  7:37   ` [dpdk-dev] [EXT] " Harman Kalra
2020-06-19  9:56     ` Burakov, Anatoly

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CALBAE1NmkfbLSzH5ymCeF9YHDHb79LA-JCW3kqexHrfs7-4+ig@mail.gmail.com \
    --to=jerinjacobk@gmail.com \
    --cc=anatoly.burakov@intel.com \
    --cc=david.hunt@intel.com \
    --cc=dev@dpdk.org \
    --cc=hkalra@marvell.com \
    --cc=liang.j.ma@intel.com \
    --cc=reshma.pattan@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git