From: Qian Cai <cai@lca.pw>
To: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>,
elver@google.com, Linux PM <linux-pm@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH -next] power/qos: fix a data race in pm_qos_*_value
Date: Mon, 24 Feb 2020 14:02:39 -0500 [thread overview]
Message-ID: <1582570959.7365.116.camel@lca.pw> (raw)
In-Reply-To: <CAJZ5v0jXZOd0yfnwcP1NrfrXnALx=5E1nmK8DHk8bJ0SLUYzAQ@mail.gmail.com>
On Mon, 2020-02-24 at 10:54 +0100, Rafael J. Wysocki wrote:
> On Mon, Feb 24, 2020 at 2:01 AM Qian Cai <cai@lca.pw> wrote:
> >
> >
> >
> > > On Feb 23, 2020, at 7:12 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > It may be a bug under certain conditions, but you don't mention what
> > > conditions they are. Reporting it as a general bug is not accurate at
> > > the very least.
> >
> > Could we rule out load tearing, store tearing and reload of global_req in cpuidle_governor_latency() for all compilers and architectures which could introduce logic bugs?
> >
> > int global_req = cpu_latency_qos_limit();
> >
> > if (device_req > global_req)
> > device_req = global_req;
> >
> > If under register pressure, the compiler might get ride of the tmp variable, i.e.,
> >
> > If (device_req > cpu_latency_qos_limit())
> > —-> race with the writer.
> > device_req = cpu_latency_qos_limit();
>
> Yes, there is a race here with or without the WRITE_ONCE()/READ_ONCE()
> annotations (note that these annotations don't prevent CPUs from
> reordering things, so device_req may be set before global_req
> regardless).
>
> However, worst-case it may cause an old value to be used and that can
> happen anyway if the entire cpuidle_governor_latency_req() runs
> between the curr_value update and pm_qos_set_value() in
> pm_qos_update_target(), for example.
>
> IOW, there is no guarantee that the new value will be used immediately
> after updating a QoS request anyway.
>
> I agree with adding the annotations (I was considering posting a patch
> doing that myself), but just as a matter of making the intention
> clear.
OK, how about this updated texts?
[PATCH -next] power/qos: annotate a data race in pm_qos_*_value
cpu_latency_constraints.target_value could be accessed concurrently via,
cpu_latency_qos_apply
pm_qos_update_target
pm_qos_set_value
cpuidle_governor_latency_req
cpu_latency_qos_limit
pm_qos_read_value
The read is outside pm_qos_lock critical section which results in a data race.
However, the worst case is that an old value to be used and that can happen
anyway, so annotate this data race using a pair of READ|WRITE_ONCE().
next prev parent reply other threads:[~2020-02-24 19:02 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-21 21:09 [PATCH -next] power/qos: fix a data race in pm_qos_*_value Qian Cai
2020-02-24 0:12 ` Rafael J. Wysocki
2020-02-24 1:01 ` Qian Cai
2020-02-24 9:54 ` Rafael J. Wysocki
2020-02-24 19:02 ` Qian Cai [this message]
2020-02-26 0:10 ` Rafael J. Wysocki
2020-02-26 0:34 ` Qian Cai
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=1582570959.7365.116.camel@lca.pw \
--to=cai@lca.pw \
--cc=elver@google.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=rafael@kernel.org \
--cc=rjw@rjwysocki.net \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.