Received: by 2002:a25:e74b:0:0:0:0:0 with SMTP id e72csp1263625ybh; Thu, 16 Jul 2020 07:33:10 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxwEvA4LS0fRzEByGYIsPB3tY9t8rCUN1YL5aZnFH6zW6rk3oJXrBPn/D+ojLrPp48Segkc X-Received: by 2002:a17:906:3984:: with SMTP id h4mr4298630eje.254.1594909990572; Thu, 16 Jul 2020 07:33:10 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1594909990; cv=none; d=google.com; s=arc-20160816; b=XW89XoIUCp9MzyuAWQ+95jfMMvdOjR3TfXI5/umFPRkKTf7Rb9WhyH+rdaI1EeTH5G SqkojBKhLwMT0Hgl17nhr4jZB/CQepjM9ptVEk+FNGJdM5f1T2eTmGoNFzvM9vUKTuU0 h3xqS9GOZSt77En9U1/mYfJAPycpVZiOlPvxMyW5tmlNZyTl5+BZiXdbqjlYzo70D6nP piwLHsfCdoofKWHIhbYqVxYbo6dILuS/pLJbd3wnvfq23JMai/HDav+ApPt+AalQLQYf 5kqLpawXGnAc1F/gzYjqUalAfEHndEyqRUZt1RGIGKQFiomMwbs1L2AUE/dUJ6/pnoeS RtbA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version; bh=8Bd27BmhqWz8TqlSUmOpu+EthUyTvCA8jY/Af5VGQ3s=; b=VrYbevykzr0ZCuPt4TD03Uf04dWP1MK69503JHQjEEEsErVMtrzeLlsyRXRA8ru7uv vN/f3kfc8XnTf+ebwtoe0Z2ZFYIfw/YV4FDgCTNVlaHsfv+PyfthVGKfVe2ReMxNwrG4 qk2VvxR9zTdPHdx+IEiKNNgUX+VPuNg1FJP05HRtbQb9dEr/P+aAbiFwIpslsk+gbQli SWL+sTU0Rul/SkmUsg5pvJJcwkZjFiSRSyRNWGqREMLYEI81k3Ltt/m8Czc5XKiok8eK A7GNkgleNNHnvLbIfDbgOAFt3WwlEIr6HvL7jG2z08rHCEfqr+FFK/n5MlMY6whJJObs Ecxw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id g1si3338797edy.370.2020.07.16.07.32.46; Thu, 16 Jul 2020 07:33:10 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728579AbgGPOcf (ORCPT + 99 others); Thu, 16 Jul 2020 10:32:35 -0400 Received: from mail-oi1-f195.google.com ([209.85.167.195]:34599 "EHLO mail-oi1-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727844AbgGPOcf (ORCPT ); Thu, 16 Jul 2020 10:32:35 -0400 Received: by mail-oi1-f195.google.com with SMTP id e4so5257182oib.1; Thu, 16 Jul 2020 07:32:33 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=8Bd27BmhqWz8TqlSUmOpu+EthUyTvCA8jY/Af5VGQ3s=; b=BZ3SIeeQM7h/zVr1+okMZoTGN2ZkqDf2mxfQhu1jEApNpK5wjGUNcf5Jvx+dVqdW/B 1PbysApJOEJhrxOWu6PG4XMw0k12c/M1N7zbqhBsWKwj+bDVOYq3xidswyNQb47PmIDV cIv6Swp6lnqxpqkdU6b6AWeG+aMViAwK5aTyK1xKXZF3dhp3Yj7DAUN+hPVpCcoGz2vA vuLDldiEiiiINuypKkNxt5nNA1wY1QK0gGipPwLtX9G6FXBbxXoEPs6yL/BSuWVDmWgk FKR0zbKFLtr8srJXWux4Y+bp5FBzcGXLkHDEfUzqe1tsNSELfy+Bjo+60vPZtXzAnmvm yPiQ== X-Gm-Message-State: AOAM530zKWTwhaTvELKlfWwF7jsrP1E25EOS/7XVGCrtdMJhQ8sCrNEd L43iKNC4J4b1s0q6QLFJG08Dvidqg7qyKXwWRVc= X-Received: by 2002:aca:f58a:: with SMTP id t132mr3944252oih.68.1594909952509; Thu, 16 Jul 2020 07:32:32 -0700 (PDT) MIME-Version: 1.0 References: <3955470.QvD6XneCf3@kreacher> <87r1tdiqpu.fsf@riseup.net> <87imeoihqs.fsf@riseup.net> In-Reply-To: <87imeoihqs.fsf@riseup.net> From: "Rafael J. Wysocki" Date: Thu, 16 Jul 2020 16:32:18 +0200 Message-ID: Subject: Re: [PATCH] cpufreq: intel_pstate: Implement passive mode with HWP enabled To: Francisco Jerez Cc: "Rafael J. Wysocki" , "Rafael J. Wysocki" , Linux PM , Linux Documentation , LKML , Peter Zijlstra , Srinivas Pandruvada , Giovanni Gherdovich , Doug Smythies Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Jul 15, 2020 at 11:35 PM Francisco Jerez wrote: > > "Rafael J. Wysocki" writes: > > > On Wed, Jul 15, 2020 at 2:09 AM Francisco Jerez wrote: > >> > >> "Rafael J. Wysocki" writes: > >> > >> > From: Rafael J. Wysocki > >> > > >> > Allow intel_pstate to work in the passive mode with HWP enabled and > >> > make it set the HWP minimum performance limit (HWP floor) to the > >> > P-state value given by the target frequency supplied by the cpufreq > >> > governor, so as to prevent the HWP algorithm and the CPU scheduler > >> > from working against each other, at least when the schedutil governor > >> > is in use, and update the intel_pstate documentation accordingly. > >> > > >> > Among other things, this allows utilization clamps to be taken > >> > into account, at least to a certain extent, when intel_pstate is > >> > in use and makes it more likely that sufficient capacity for > >> > deadline tasks will be provided. > >> > > >> > After this change, the resulting behavior of an HWP system with > >> > intel_pstate in the passive mode should be close to the behavior > >> > of the analogous non-HWP system with intel_pstate in the passive > >> > mode, except that in the frequency range below the base frequency > >> > (ie. the frequency retured by the base_frequency cpufreq attribute > >> > in sysfs on HWP systems) the HWP algorithm is allowed to go above > >> > the floor P-state set by intel_pstate with or without hardware > >> > coordination of P-states among CPUs in the same package. > >> > > >> > Also note that the setting of the HWP floor may not be taken into > >> > account by the processor in the following cases: > >> > > >> > * For the HWP floor in the range of P-states above the base > >> > frequency, referred to as the turbo range, the processor has a > >> > license to choose any P-state from that range, either below or > >> > above the HWP floor, just like a non-HWP processor in the case > >> > when the target P-state falls into the turbo range. > >> > > >> > * If P-states of the CPUs in the same package are coordinated > >> > at the hardware level, the processor may choose a P-state > >> > above the HWP floor, just like a non-HWP processor in the > >> > analogous case. > >> > > >> > With this change applied, intel_pstate in the passive mode > >> > assumes complete control over the HWP request MSR and concurrent > >> > changes of that MSR (eg. via the direct MSR access interface) are > >> > overridden by it. > >> > > >> > Signed-off-by: Rafael J. Wysocki > >> > --- > >> > > >> > This basically unifies the passive mode behavior of intel_pstate for systems > >> > with and without HWP enabled. The only case in which there is a difference > >> > between the two (after this patch) is below the turbo range, where the HWP > >> > algorithm can go above the floor regardless of whether or not P-state are > >> > coordinated package-wide (this means the systems with per-core P-states > >> > mostly is where the difference can be somewhat visible). > >> > > >> > Since the passive mode hasn't worked with HWP at all, and it is not going to > >> > the default for HWP systems anyway, I don't see any drawbacks related to making > >> > this change, so I would consider this as 5.9 material unless there are any > >> > serious objections. > >> > > >> > Thanks! > >> > > >> > --- > >> > Documentation/admin-guide/pm/intel_pstate.rst | 89 +++++++--------- > >> > drivers/cpufreq/intel_pstate.c | 141 ++++++++++++++++++++------ > >> > 2 files changed, 152 insertions(+), 78 deletions(-) > >> > > >> > Index: linux-pm/drivers/cpufreq/intel_pstate.c > >> > =================================================================== > >> > --- linux-pm.orig/drivers/cpufreq/intel_pstate.c > >> > +++ linux-pm/drivers/cpufreq/intel_pstate.c > >> > @@ -36,6 +36,7 @@ > >> > #define INTEL_PSTATE_SAMPLING_INTERVAL (10 * NSEC_PER_MSEC) > >> > > >> > #define INTEL_CPUFREQ_TRANSITION_LATENCY 20000 > >> > +#define INTEL_CPUFREQ_TRANSITION_DELAY_HWP 5000 > >> > #define INTEL_CPUFREQ_TRANSITION_DELAY 500 > >> > > >> > #ifdef CONFIG_ACPI > >> > @@ -222,6 +223,7 @@ struct global_params { > >> > * preference/bias > >> > * @epp_saved: Saved EPP/EPB during system suspend or CPU offline > >> > * operation > >> > + * @epp_cached Cached HWP energy-performance preference value > >> > * @hwp_req_cached: Cached value of the last HWP Request MSR > >> > * @hwp_cap_cached: Cached value of the last HWP Capabilities MSR > >> > * @last_io_update: Last time when IO wake flag was set > >> > @@ -259,6 +261,7 @@ struct cpudata { > >> > s16 epp_policy; > >> > s16 epp_default; > >> > s16 epp_saved; > >> > + s16 epp_cached; > >> > u64 hwp_req_cached; > >> > u64 hwp_cap_cached; > >> > u64 last_io_update; > >> > @@ -676,6 +679,8 @@ static int intel_pstate_set_energy_pref_ > >> > > >> > value |= (u64)epp << 24; > >> > ret = wrmsrl_on_cpu(cpu_data->cpu, MSR_HWP_REQUEST, value); > >> > + > >> > + WRITE_ONCE(cpu_data->epp_cached, epp); > >> > >> Why introduce a new EPP cache variable if there is already > >> hwp_req_cached? If intel_pstate_set_energy_pref_index() is failing to > >> update hwp_req_cached maybe we should fix that instead. That will save > >> you a little bit of work in intel_cpufreq_adjust_hwp(). > > > > Yes, it would, but then we'd need to explicitly synchronize > > intel_pstate_set_energy_pref_index() with the scheduler context which > > I'd rather avoid. > > > > How does using a differently named variable save you from doing that? It is a separate variable. The only updater of epp_cached, except for the initialization, is intel_pstate_set_energy_pref_index() and it cannot race with another instance of itself, so there are no concurrent writes to epp_cached. In the passive mode the only updater of hwp_req_cached, except for the initialization, is intel_cpufreq_adjust_hwp() (or there is a bug in the patch that I have missed) and it cannot race with another instance of itself for the same CPU, so there are no concurrent writes to hwp_req_cached. if intel_pstate_set_energy_pref_index() updated hwp_req_cached directly, however, it might be updated in two places concurrently and so explicit synchronization would be necessary. > And won't the EPP setting programmed by intel_pstate_set_energy_pref_index() > be lost if intel_pstate_hwp_boost_up() or some other user of > hwp_req_cached is executed afterwards with the current approach? The value written to the register by it may be overwritten by a concurrent intel_cpufreq_adjust_hwp(), but that is not a problem, because next time intel_cpufreq_adjust_hwp() runs for the target CPU, it will pick up the updated epp_cached value which will be written to the register. So there may be a short time window after the intel_pstate_set_energy_pref_index() invocation in which the new EPP value may not be in effect, but in general there is no guarantee that the new EPP will take effect immediately after updating the MSR anyway, so that race doesn't matter. That said, that race is avoidable, but I was thinking that trying to avoid it might not be worth it. Now I see a better way to avoid it, though, so I'm going to update the patch to that end. > Seems like a bug to me. It is racy, but not every race is a bug. > >> > } else { > >> > if (epp == -EINVAL) > >> > epp = (pref_index - 1) << 2; > >> > @@ -2047,6 +2052,7 @@ static int intel_pstate_init_cpu(unsigne > >> > cpu->epp_default = -EINVAL; > >> > cpu->epp_powersave = -EINVAL; > >> > cpu->epp_saved = -EINVAL; > >> > + WRITE_ONCE(cpu->epp_cached, -EINVAL); > >> > } > >> > > >> > cpu = all_cpu_data[cpunum]; > >> > @@ -2245,7 +2251,10 @@ static int intel_pstate_verify_policy(st > >> > > >> > static void intel_cpufreq_stop_cpu(struct cpufreq_policy *policy) > >> > { > >> > - intel_pstate_set_min_pstate(all_cpu_data[policy->cpu]); > >> > + if (hwp_active) > >> > + intel_pstate_hwp_force_min_perf(policy->cpu); > >> > + else > >> > + intel_pstate_set_min_pstate(all_cpu_data[policy->cpu]); > >> > } > >> > > >> > static void intel_pstate_stop_cpu(struct cpufreq_policy *policy) > >> > @@ -2253,12 +2262,10 @@ static void intel_pstate_stop_cpu(struct > >> > pr_debug("CPU %d exiting\n", policy->cpu); > >> > > >> > intel_pstate_clear_update_util_hook(policy->cpu); > >> > - if (hwp_active) { > >> > + if (hwp_active) > >> > intel_pstate_hwp_save_state(policy); > >> > - intel_pstate_hwp_force_min_perf(policy->cpu); > >> > - } else { > >> > - intel_cpufreq_stop_cpu(policy); > >> > - } > >> > + > >> > + intel_cpufreq_stop_cpu(policy); > >> > } > >> > > >> > static int intel_pstate_cpu_exit(struct cpufreq_policy *policy) > >> > @@ -2388,13 +2395,82 @@ static void intel_cpufreq_trace(struct c > >> > fp_toint(cpu->iowait_boost * 100)); > >> > } > >> > > >> > +static void intel_cpufreq_adjust_hwp(struct cpudata *cpu, u32 target_pstate, > >> > + bool fast_switch) > >> > +{ > >> > + u64 prev = READ_ONCE(cpu->hwp_req_cached), value = prev; > >> > + s16 epp; > >> > + > >> > + value &= ~HWP_MIN_PERF(~0L); > >> > + value |= HWP_MIN_PERF(target_pstate); > >> > + > >> > + /* > >> > + * The entire MSR needs to be updated in order to update the HWP min > >> > + * field in it, so opportunistically update the max too if needed. > >> > + */ > >> > + value &= ~HWP_MAX_PERF(~0L); > >> > + value |= HWP_MAX_PERF(cpu->max_perf_ratio); > >> > + > >> > + /* > >> > + * In case the EPP has been adjusted via sysfs, write the last cached > >> > + * value of it to the MSR as well. > >> > + */ > >> > + epp = READ_ONCE(cpu->epp_cached); > >> > + if (epp >= 0) { > >> > + value &= ~GENMASK_ULL(31, 24); > >> > + value |= (u64)epp << 24; > >> > + } > >> > + > >> > + if (value == prev) > >> > + return; > >> > + > >> > + WRITE_ONCE(cpu->hwp_req_cached, value); > >> > + if (fast_switch) > >> > + wrmsrl(MSR_HWP_REQUEST, value); > >> > + else > >> > + wrmsrl_on_cpu(cpu->cpu, MSR_HWP_REQUEST, value); > >> > +} > >> > >> I've asked this question already but you may have missed it: Given that > >> you are of the opinion that [1] should be implemented in schedutil > >> instead with intel_pstate in HWP passive mode, what's your plan for > >> exposing the HWP_MAX_PERF knob to the governor in addition to > >> HWP_MIN_PERF, since the interface implemented here only allows the > >> governor to provide a single frequency? > >> > >> [1] https://lwn.net/ml/linux-pm/20200428032258.2518-1-currojerez@riseup.net/ > > > > This is not just about the schedutil governor, but about cpufreq > > governors in general (someone may still want to use the performance > > governor on top of intel_pstate, for example). > > > > And while governors can only provide one frequency, the policy limits > > in the cpufreq framework are based on QoS lists now and so it is > > possible to add a max limit request, say from a driver, to the max QoS > > list, and update it as needed, causing the max policy limit to be > > adjusted. > > > > That said I'm not exactly sure how useful the max limit generally is > > in practice on HWP systems, given that setting it above the base > > frequency causes it to be ignored, effectively, and the turbo range > > may be wider than the range of P-states below the base frequency. > > > > I don't think that's accurate. I've looked at hundreds of traces while > my series [1] was in control of HWP_REQ_MAX and I've never seen an > excursion above the maximum HWP_REQ_MAX control specified by it within a > given P-state domain, even while that maximum specified was well into > the turbo range. I'm not going to argue with your experience. :-) What I'm saying is that there is no guarantee that the processor will always select P-states below HWP_REQ_MAX in the turbo range. That may not happen in practice, but it is not precluded AFAICS. Also while HWP_REQ_MAX can work in practice most of the time with HWP enabled, without HWP there is no easy way to limit the max frequency if the current request falls into the turbo range. The HWP case is more important nowadays, but there still are systems without it and ideally they should be covered as well. > So, yeah, I agree that HWP_REQ_MAX is nothing like a > hard limit, particularly when multiple threads are running on the same > clock domain, but the processor will still make its best effort to limit > the clock frequency to the maximum of the requested maximums, even if it > happens to be within the turbo range. That doesn't make it useless. I haven't used the word "useless" anywhere in my previous message. Using the max frequency to control power has merit, but how much of it is there depends on some factors that may change from one system to another. The alternative power control methods may be more reliable in general. > The exact same thing can be said about controlling HWP_REQ_MIN as you're > doing now in this revision of your patch, BTW. Which has been clearly stated in the changelog I believe. The point here is that this is as good as using the perf control register to ask for a given P-state without HWP which trying to drive the max too is added complexity. > If you don't believe me here is the turbostat sample with maximum > Bzy_MHz I get on the computer I'm sitting on right now while compiling a > kernel on CPU0 if I set HWP_REQ_MAX to 0x1c (within the turbo range): > > | Core CPU Avg_MHz Busy% Bzy_MHz HWP_REQ PkgWatt CorWatt > | - - 757 27.03 2800 0x0000000000000000 7.13 4.90 > | 0 0 2794 99.77 2800 0x0000000080001c04 7.13 4.90 > | 0 2 83 2.98 2800 0x0000000080001c04 > | 1 1 73 2.60 2800 0x0000000080001c04 > | 1 3 78 2.79 2800 0x0000000080001c04 > > With the default HWP_REQUEST: > > | Core CPU Avg_MHz Busy% Bzy_MHz HWP_REQ PkgWatt CorWatt > | - - 814 27.00 3015 0x0000000000000000 8.49 6.18 > | 0 0 2968 98.24 3021 0x0000000080001f04 8.49 6.18 > | 0 2 84 2.81 2982 0x0000000080001f04 > | 1 1 99 3.34 2961 0x0000000080001f04 > | 1 3 105 3.60 2921 0x0000000080001f04 > > > Generally, I'm not quite convinced that limiting the max frequency is > > really the right choice for controlling the processor's power draw on > > the systems in question. There are other ways to do that, which in > > theory should be more effective. I mentioned RAPL somewhere in this > > context and there's the GUC firmware too. > > I feel like we've had that conversation before and it's somewhat > off-topic so I'll keep it short: Yes, in theory RAPL is more effective > than HWP_REQ_MAX as a mechanism to limit the absolute power consumption > of the processor package, but that's not the purpose of [1], its purpose > is setting a lower limit to the energy efficiency of the processor when > the maximum usable CPU frequency is known (due to the existence of an IO > device bottleneck) Whether or not that frequency is actually known seems quite questionable to me, but that's aside. More important, it is unclear to me what you mean by "a lower limit to the energy efficiency of the processor". I guess what you mean is that the processor might decide to go for a more energy-efficient configuration by increasing its frequency in a "race to idle" fashion (in response to a perceived utilization spike) and you want to prevent that from occurring. Or, generally speaking, that the CPU performance scaling logic, either in the kernel or in the processor itself, might select a higher operating frequency of a CPU in response to a perceived utilization spike, but that may be a mistake in the presence of another data processing device sharing the power budget with the processor, so you want to prevent that from taking place. In both cases, I wouldn't call that limiting the energy-efficiency of the processor. Effectively, this means putting a limit on the processor's power budget, which is exactly what RAPL is for. > -- And if the maximum usable CPU frequency is the > information we have at hand, How so? How can you tell what that frequency is? > controlling the maximum CPU frequency > directly is optimal, rather than trying to find the RAPL constraint that > achieves the same average frequency by trial an error. Also, in theory, > even if you had an oracle to tell you what the appropriate RAPL > constraint is, the result would necessarily be more energy-inefficient > than controlling the maximum CPU frequency directly, since you're giving > the processor additional freedom to run at frequencies above the one you > want to average, which is guaranteed to be more energy-inefficient than > running at that fixed frequency, assuming we are in the region of > convexity of the processor's power curve. So the reason why you want to limit the processor's max frequency in the first place is because it is sharing the power budget with something else. If there's no sharing of the power budget or thermal constraints, there is no need to limit the CPU frequency other than for the sake of saving energy. What you can achieve by limiting the max CPU frequency is to make the processor draw less power (and cause it to use either less or more energy, depending on the energy-efficiency curve). You don't know how much less power it will draw then, however. You seem to be saying "I know exactly what the maximum frequency of the CPU can be, so why I don't set it as the upper limit", but I'm questioning the source of that knowledge. Does it not come from knowing the power budget you want to give to the processor? > Anyway, if you still have some disagreement on the theoretical details > you're more than welcome to bring up the matter on the other thread [1], > or accept the invitation for a presentation I sent you months ago... ;) Why don't we continue the discussion here instead? I think we are getting to the bottom of things here.