From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH] AMD, powernow: Update P-state directly when _PSD's CoordType is DOMAIN_COORD_TYPE_HW_ALL Date: Fri, 17 Aug 2012 09:44:32 -0400 Message-ID: <502E4AC0.9080502@amd.com> References: <85190245a94d9945b765.1345135279@localhost.localdomain> <502E36C00200007800095D8B@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <502E36C00200007800095D8B@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel List-Id: xen-devel@lists.xenproject.org On 08/17/2012 06:19 AM, Jan Beulich wrote: >>>> On 16.08.12 at 18:41, Boris Ostrovsky wrote: >> AMD, powernow: Update P-state directly when _PSD's CoordType is >> DOMAIN_COORD_TYPE_HW_ALL >> >> When _PSD's CoordType is DOMAIN_COORD_TYPE_HW_ALL (i.e. shared_type is >> CPUFREQ_SHARED_TYPE_HW) which most often is the case on servers, there is no >> reason to go into on_selected_cpus() code, we call call transition_pstate() >> directly. >> >> Signed-off-by: Boris Ostrovsky > > Looks good to me in general (one minor comment below, but no > need to resubmit just because of this). But it's not really clear > to me whether this actually improves anything (knowing of which > is needed to decide whether to put this in now or after 4.2). I didn't see any noticeable improvement but then I may not have run the right tests. I imagine this may be helpful, for example, in webserver-type environments with frequent P-state transitions when new connections are requested and short-lived threads/processes are created. But I haven't been able to set this up to observe this. I think post-4.2 is fine. -boris > >> --- a/xen/arch/x86/acpi/cpufreq/powernow.c Wed Aug 15 09:41:21 2012 +0100 >> +++ b/xen/arch/x86/acpi/cpufreq/powernow.c Thu Aug 16 18:38:21 2012 +0200 >> @@ -56,20 +56,9 @@ >> >> static struct cpufreq_driver powernow_cpufreq_driver; >> >> -struct drv_cmd { >> - unsigned int type; >> - const cpumask_t *mask; >> - u32 val; >> - int turbo; >> -}; >> - >> -static void transition_pstate(void *drvcmd) >> +static void transition_pstate(void *pstate) >> { >> - struct drv_cmd *cmd; >> - cmd = (struct drv_cmd *) drvcmd; >> - >> - >> - wrmsrl(MSR_PSTATE_CTRL, cmd->val); >> + wrmsrl(MSR_PSTATE_CTRL, *(int *)pstate); > > The variable a pointer to which gets passed in for this function > is "unsigned int", so you surely would need to cast to that type > instead of plain "int". > > Jan > >> } >> >> static void update_cpb(void *data) >> @@ -106,13 +95,11 @@ static int powernow_cpufreq_target(struc >> { >> struct acpi_cpufreq_data *data = cpufreq_drv_data[policy->cpu]; >> struct processor_performance *perf; >> - struct cpufreq_freqs freqs; >> cpumask_t online_policy_cpus; >> - struct drv_cmd cmd; >> - unsigned int next_state = 0; /* Index into freq_table */ >> - unsigned int next_perf_state = 0; /* Index into perf table */ >> - int result = 0; >> - int j = 0; >> + unsigned int next_state; /* Index into freq_table */ >> + unsigned int next_perf_state; /* Index into perf table */ >> + int result; >> + int j; >> >> if (unlikely(data == NULL || >> data->acpi_data == NULL || data->freq_table == NULL)) { >> @@ -125,9 +112,7 @@ static int powernow_cpufreq_target(struc >> target_freq, >> relation, &next_state); >> if (unlikely(result)) >> - return -ENODEV; >> - >> - cpumask_and(&online_policy_cpus, policy->cpus, &cpu_online_map); >> + return result; >> >> next_perf_state = data->freq_table[next_state].index; >> if (perf->state == next_perf_state) { >> @@ -137,26 +122,28 @@ static int powernow_cpufreq_target(struc >> return 0; >> } >> >> - if (policy->shared_type != CPUFREQ_SHARED_TYPE_ANY) >> - cmd.mask = &online_policy_cpus; >> - else >> - cmd.mask = cpumask_of(policy->cpu); >> + if (policy->shared_type == CPUFREQ_SHARED_TYPE_HW && >> + likely(policy->cpu == smp_processor_id())) { >> + transition_pstate(&next_perf_state); >> + cpufreq_statistic_update(policy->cpu, perf->state, next_perf_state); >> + } else { >> + cpumask_and(&online_policy_cpus, policy->cpus, &cpu_online_map); >> >> - freqs.old = perf->states[perf->state].core_frequency * 1000; >> - freqs.new = data->freq_table[next_state].frequency; >> + if (policy->shared_type == CPUFREQ_SHARED_TYPE_ALL || >> + unlikely(policy->cpu != smp_processor_id())) >> + on_selected_cpus(&online_policy_cpus, transition_pstate, >> + &next_perf_state, 1); >> + else >> + transition_pstate(&next_perf_state); >> >> - cmd.val = next_perf_state; >> - cmd.turbo = policy->turbo; >> - >> - on_selected_cpus(cmd.mask, transition_pstate, &cmd, 1); >> - >> - for_each_cpu(j, &online_policy_cpus) >> - cpufreq_statistic_update(j, perf->state, next_perf_state); >> + for_each_cpu(j, &online_policy_cpus) >> + cpufreq_statistic_update(j, perf->state, next_perf_state); >> + } >> >> perf->state = next_perf_state; >> - policy->cur = freqs.new; >> + policy->cur = data->freq_table[next_state].frequency; >> >> - return result; >> + return 0; >> } >> >> static int powernow_cpufreq_verify(struct cpufreq_policy *policy) >> >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xen.org >> http://lists.xen.org/xen-devel > > >