From: Fenghua Yu <fenghua.yu@intel.com>
To: Andy Lutomirski <luto@amacapital.net>
Cc: Andy Lutomirski <luto@kernel.org>,
Thomas Gleixner <tglx@linutronix.de>,
Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
H Peter Anvin <hpa@zytor.com>, Ashok Raj <ashok.raj@intel.com>,
Tony Luck <tony.luck@intel.com>,
Ravi V Shankar <ravi.v.shankar@intel.com>,
linux-kernel <linux-kernel@vger.kernel.org>, x86 <x86@kernel.org>
Subject: Re: [PATCH v4 3/5] x86/umwait: Add sysfs interface to control umwait C0.2 state
Date: Mon, 17 Jun 2019 13:27:03 -0700 [thread overview]
Message-ID: <20190617202702.GB217081@romley-ivt3.sc.intel.com> (raw)
In-Reply-To: <F021B947-90E9-450A-9196-531B7EE965F1@amacapital.net>
On Mon, Jun 10, 2019 at 06:41:31AM -0700, Andy Lutomirski wrote:
>
>
> > On Jun 9, 2019, at 11:02 PM, Fenghua Yu <fenghua.yu@intel.com> wrote:
> >
> >> On Sun, Jun 09, 2019 at 09:24:18PM -0700, Andy Lutomirski wrote:
> >>> On Sun, Jun 9, 2019 at 9:02 PM Fenghua Yu <fenghua.yu@intel.com> wrote:
> >>>
> >>>> On Sat, Jun 08, 2019 at 03:50:32PM -0700, Andy Lutomirski wrote:
> >>>>> On Fri, Jun 7, 2019 at 3:10 PM Fenghua Yu <fenghua.yu@intel.com> wrote:
> >>>>>
> >>>>> C0.2 state in umwait and tpause instructions can be enabled or disabled
> >>>>> on a processor through IA32_UMWAIT_CONTROL MSR register.
> >>>>>
> >>>>> By default, C0.2 is enabled and the user wait instructions result in
> >>>>> lower power consumption with slower wakeup time.
> >>>>>
> >>>>> But in real time systems which require faster wakeup time although power
> >>>>> savings could be smaller, the administrator needs to disable C0.2 and all
> >>>>> C0.2 requests from user applications revert to C0.1.
> >>>>>
> >>>>> A sysfs interface "/sys/devices/system/cpu/umwait_control/enable_c02" is
> >>>>> created to allow the administrator to control C0.2 state during run time.
> >>>>
> >>>> This looks better than the previous version. I think the locking is
> >>>> still rather confused. You have a mutex that you hold while changing
> >>>> the value, which is entirely reasonable. But, of the code paths that
> >>>> write the MSR, only one takes the mutex.
> >>>>
> >>>> I think you should consider making a function that just does:
> >>>>
> >>>> wrmsr(MSR_IA32_UMWAIT_CONTROL, READ_ONCE(umwait_control_cached), 0);
> >>>>
> >>>> and using it in all the places that update the MSR. The only thing
> >>>> that should need the lock is the sysfs code to avoid accidentally
> >>>> corrupting the value, but that code should also use WRITE_ONCE to do
> >>>> its update.
> >>>
> >>> Based on the comment, the illustrative CPU online and enable_c02 store
> >>> functions would be:
> >>>
> >>> umwait_cpu_online()
> >>> {
> >>> wrmsr(MSR_IA32_UMWAIT_CONTROL, READ_ONCE(umwait_control_cached), 0);
> >>> return 0;
> >>> }
> >>>
> >>> enable_c02_store()
> >>> {
> >>> mutex_lock(&umwait_lock);
> >>> umwait_control_c02 = (u32)!c02_enabled;
> >>> WRITE_ONCE(umwait_control_cached, 2 | get_umwait_control_max_time());
> >>> on_each_cpu(umwait_control_msr_update, NULL, 1);
> >>> mutex_unlock(&umwait_lock);
> >>> }
> >>>
> >>> Then suppose umwait_control_cached = 100000 initially and only CPU0 is
> >>> running. Admin change bit 0 in MSR from 0 to 1 to disable C0.2 and is
> >>> onlining CPU1 in the same time:
> >>>
> >>> 1. On CPU1, read umwait_control_cached to eax as 100000 in
> >>> umwait_cpu_online()
> >>> 2. On CPU0, write 100001 to umwait_control_cached in enable_c02_store()
> >>> 3. On CPU1, wrmsr with eax=100000 in umwaint_cpu_online()
> >>> 4. On CPU0, wrmsr with 100001 in enabled_c02_store()
> >>>
> >>> The result is CPU0 and CPU1 have different MSR values.
> >>
> >> Yes, but only transiently, because you didn't finish your example.
> >>
> >> Step 5: enable_c02_store() does on_each_cpu(), and CPU 1 gets updated.
> >
> > There is no sync on wrmsr on CPU0 and CPU1.
>
> What do you mean by sync?
>
> > So a better sequence to
> > describe the problem is changing the order of wrmsr:
> >
> > 1. On CPU1, read umwait_control_cached to eax as 100000 in
> > umwait_cpu_online()
> > 2. On CPU0, write 100001 to umwait_control_cached in enable_c02_store()
> > 3. On CPU0, wrmsr with 100001 in on_each_cpu() in enabled_c02_store()
> > 4. On CPU1, wrmsr with eax=100000 in umwaint_cpu_online()
> >
> > So CPU1 and CPU0 have different MSR values. This won't be transient.
>
> You are still ignoring the wrmsr on CPU1 due to on_each_cpu().
>
Initially umwait_control_cached is 100000 and CPU0 is online while CPU1
is going to be online:
1. On CPU1, cpu_online_mask=0x3 in start_secondary()
2. On CPU1, read umwait_control_cached to eax as 100000 in umwait_cpu_online()
3. On CPU0, write 100001 to umwait_control_cached in enable_c02_store()
4. On CPU0, execute one_each_cpu() in enabled_c02_store():
wrmsr with 100001 on CPU0
wrmsr with 100001 on CPU1
5. On CPU1, wrmsr with eax=100000 in umwaint_cpu_online()
So the MSR is 100000 on CPU1 and 100001 on CPU0. The MSRs are different on
the CPUs.
Is this a right sequence to demonstrate locking issue without the mutex
locking?
Thanks.
-Fenghua
next prev parent reply other threads:[~2019-06-17 20:36 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-07 22:00 [PATCH v4 0/5] x86/umwait: Enable user wait instructions Fenghua Yu
2019-06-07 22:00 ` [PATCH v4 1/5] x86/cpufeatures: Enumerate " Fenghua Yu
2019-06-07 22:00 ` [PATCH v4 2/5] x86/umwait: Initialize umwait control values Fenghua Yu
2019-06-08 22:52 ` Andy Lutomirski
2019-06-10 4:13 ` Fenghua Yu
2019-06-10 4:27 ` Andy Lutomirski
2019-06-11 20:46 ` Thomas Gleixner
2019-06-17 20:46 ` Fenghua Yu
2019-06-18 5:43 ` Thomas Gleixner
2019-06-11 8:50 ` Peter Zijlstra
2019-06-11 17:04 ` Fenghua Yu
2019-06-07 22:00 ` [PATCH v4 3/5] x86/umwait: Add sysfs interface to control umwait C0.2 state Fenghua Yu
2019-06-08 22:50 ` Andy Lutomirski
2019-06-10 3:53 ` Fenghua Yu
2019-06-10 4:24 ` Andy Lutomirski
2019-06-10 6:02 ` Fenghua Yu
2019-06-10 13:41 ` Andy Lutomirski
2019-06-17 20:27 ` Fenghua Yu [this message]
2019-06-17 23:02 ` Andy Lutomirski
2019-06-17 23:11 ` Fenghua Yu
2019-06-17 23:41 ` Andy Lutomirski
2019-06-18 0:00 ` Fenghua Yu
2019-06-18 0:19 ` Andy Lutomirski
2019-06-18 2:32 ` Fenghua Yu
2019-06-08 22:52 ` Andy Lutomirski
2019-06-10 4:04 ` Fenghua Yu
2019-06-10 4:26 ` Andy Lutomirski
2019-06-17 22:48 ` Fenghua Yu
2019-06-17 22:59 ` Andy Lutomirski
2019-06-17 22:51 ` Fenghua Yu
2019-06-11 8:54 ` Peter Zijlstra
2019-06-11 16:04 ` Andy Lutomirski
2019-06-11 17:27 ` Peter Zijlstra
2019-06-17 15:14 ` Andy Lutomirski
2019-06-17 18:11 ` Fenghua Yu
2019-06-07 22:00 ` [PATCH v4 4/5] x86/umwait: Add sysfs interface to control umwait maximum time Fenghua Yu
2019-06-07 22:00 ` [PATCH v4 5/5] x86/umwait: Document umwait control sysfs interfaces Fenghua Yu
2019-06-11 9:01 ` [PATCH v4 0/5] x86/umwait: Enable user wait instructions Peter Zijlstra
2019-06-11 17:37 ` Fenghua Yu
2019-06-17 14:19 ` Peter Zijlstra
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=20190617202702.GB217081@romley-ivt3.sc.intel.com \
--to=fenghua.yu@intel.com \
--cc=ashok.raj@intel.com \
--cc=bp@alien8.de \
--cc=hpa@zytor.com \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@amacapital.net \
--cc=luto@kernel.org \
--cc=mingo@redhat.com \
--cc=ravi.v.shankar@intel.com \
--cc=tglx@linutronix.de \
--cc=tony.luck@intel.com \
--cc=x86@kernel.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).