lkml.org 
[lkml]   [2016]   [Oct]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC] arm64: Enforce observed order for spinlock and data
On 2016-10-01 14:11, Mark Rutland wrote:
> Hi Brent,
>
> Evidently my questions weren't sufficiently clear; even with your
> answers it's
> not clear to me what precise issue you're attempting to solve. I've
> tried to be
> more specific this time.
>
> At a high-level, can you clarify whether you're attempting to solve is:
>
> (a) a functional correctness issue (e.g. data corruption)
> (b) a performance issue
>
> And whether this was seen in practice, or found through code
> inspection?
>
> On Sat, Oct 01, 2016 at 12:11:36PM -0400, bdegraaf@codeaurora.org
> wrote:
>> On 2016-09-30 15:32, Mark Rutland wrote:
>> >On Fri, Sep 30, 2016 at 01:40:57PM -0400, Brent DeGraaf wrote:
>> >>+ * so LSE needs an explicit barrier here as well. Without this, the
>> >>+ * changed contents of the area protected by the spinlock could be
>> >>+ * observed prior to the lock.
>> >>+ */
>> >
>> >By whom? We generally expect that if data is protected by a lock, you take
>> >the lock before accessing it. If you expect concurrent lockless readers,
>> >then there's a requirement on the writer side to explicitly provide the
>> >ordering it requires -- spinlocks are not expected to provide that.
>>
>> More details are in my response to Robin, but there is an API arm64
>> supports
>> in spinlock.h which is used by lockref to determine whether a lock is
>> free or
>> not. For that code to work properly without adding these barriers,
>> that API
>> needs to take the lock.
>
> Can you please provide a concrete example of a case where things go
> wrong,
> citing the code (or access pattern) in question? e.g. as in the commit
> messages
> for:
>
> 8e86f0b409a44193 ("arm64: atomics: fix use of acquire + release for
> full barrier semantics)
> 859b7a0e89120505 ("mm/slub: fix lockups on PREEMPT && !SMP kernels")
>
> (for the latter, I messed up the register -> var mapping in one
> paragraph, but
> the style and reasoning is otherwise sound).
>
> In the absence of a concrete example as above, it's very difficult to
> reason
> about what the underlying issue is, and what a valid fix would be for
> said
> issue.
>
>> >What pattern of accesses are made by readers and writers such that there is
>> >a problem?
>
> Please note here I was asking specifically w.r.t. the lockref code,
> e.g. which
> loads could see stale data, and what does the code do based on this
> value such
> that there is a problem.
>
>> I added the barriers to the readers/writers because I do not know
>> these are
>> not similarly abused. There is a lot of driver code out there, and
>> ensuring
>> order is the safest way to be sure we don't get burned by something
>> similar
>> to the lockref access.
>
> Making the architecture-provided primitives overly strong harms
> performance and
> efficiency (in general), makes the code harder to maintain and optimise
> in
> future, and only masks the issue (which could crop up on other
> architectures,
> for instance).
>
> Thanks,
> Mark.


Thinking about this, as the reader/writer code has no known "abuse"
case, I'll
remove it from the patchset, then provide a v2 patchset with a detailed
explanation for the lockref problem using the commits you provided as an
example,
as well as performance consideration.

Brent

\
 
 \ /
  Last update: 2016-10-03 21:21    [W:0.057 / U:0.056 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site