Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1521929 > unrolled thread

[RFC][PATCH 0/7] kref improvements

Back to article view | Back to linux.kernel

Contents

  [RFC][PATCH 0/7] kref improvements Peter Zijlstra <peterz@infradead.org> - 2016-11-14 18:50 +0100
    [RFC][PATCH 5/7] kref: Implement kref_put_lock() Peter Zijlstra <peterz@infradead.org> - 2016-11-14 18:50 +0100
      Re: [RFC][PATCH 5/7] kref: Implement kref_put_lock() Kees Cook <keescook@chromium.org> - 2016-11-14 21:40 +0100
        Re: [RFC][PATCH 5/7] kref: Implement kref_put_lock() Peter Zijlstra <peterz@infradead.org> - 2016-11-15 09:00 +0100
    [RFC][PATCH 6/7] kref: Avoid more abuse Peter Zijlstra <peterz@infradead.org> - 2016-11-14 18:50 +0100
    [RFC][PATCH 4/7] kref: Use kref_get_unless_zero() more Peter Zijlstra <peterz@infradead.org> - 2016-11-14 18:50 +0100
    [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-14 18:50 +0100
      Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Ingo Molnar <mingo@kernel.org> - 2016-11-15 09:50 +0100
        Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-15 10:50 +0100
          Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Ingo Molnar <mingo@kernel.org> - 2016-11-15 11:10 +0100
            Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-15 11:50 +0100
              Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Ingo Molnar <mingo@kernel.org> - 2016-11-15 14:10 +0100
                Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Kees Cook <keescook@chromium.org> - 2016-11-15 19:10 +0100
                  Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-15 20:20 +0100
                    Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Kees Cook <keescook@chromium.org> - 2016-11-15 20:30 +0100
                      Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Ingo Molnar <mingo@kernel.org> - 2016-11-16 09:40 +0100
                        Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Greg KH <gregkh@linuxfoundation.org> - 2016-11-16 10:00 +0100
                          Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Ingo Molnar <mingo@kernel.org> - 2016-11-16 10:10 +0100
                            Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Greg KH <gregkh@linuxfoundation.org> - 2016-11-16 10:30 +0100
                        Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-16 11:20 +0100
                          Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Kees Cook <keescook@chromium.org> - 2016-11-16 20:00 +0100
                            Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-17 09:40 +0100
                              Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Kees Cook <keescook@chromium.org> - 2016-11-17 21:00 +0100
                        Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Kees Cook <keescook@chromium.org> - 2016-11-16 19:50 +0100
      Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Boqun Feng <boqun.feng@gmail.com> - 2016-11-15 13:40 +0100
        Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-15 14:10 +0100
          Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Boqun Feng <boqun.feng@gmail.com> - 2016-11-15 15:20 +0100
            Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-17 10:30 +0100
              Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Boqun Feng <boqun.feng@gmail.com> - 2016-11-17 10:50 +0100
                Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-17 11:40 +0100
                  Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-17 11:50 +0100
                    Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Greg KH <gregkh@linuxfoundation.org> - 2016-11-17 12:10 +0100
                      Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-17 18:10 +0100
                  Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-17 18:30 +0100
              Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Will Deacon <will.deacon@arm.com> - 2016-11-17 18:10 +0100
                Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Boqun Feng <boqun.feng@gmail.com> - 2016-11-18 09:30 +0100
                  Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Will Deacon <will.deacon@arm.com> - 2016-11-18 11:20 +0100
              Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Will Deacon <will.deacon@arm.com> - 2016-11-17 18:10 +0100
                Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-17 18:30 +0100
      RE: [RFC][PATCH 7/7] kref: Implement using refcount_t "Reshetova, Elena" <elena.reshetova@intel.com> - 2016-11-18 11:10 +0100
        Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-18 12:40 +0100
          Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Will Deacon <will.deacon@arm.com> - 2016-11-18 18:10 +0100
            Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-18 20:00 +0100
            Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Boqun Feng <boqun.feng@gmail.com> - 2016-11-21 05:10 +0100
              Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Ingo Molnar <mingo@kernel.org> - 2016-11-21 08:50 +0100
                Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Boqun Feng <boqun.feng@gmail.com> - 2016-11-21 09:40 +0100
          Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Boqun Feng <boqun.feng@gmail.com> - 2016-11-21 09:50 +0100
            Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-21 10:10 +0100
              Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Boqun Feng <boqun.feng@gmail.com> - 2016-11-21 10:40 +0100
      RE: [RFC][PATCH 7/7] kref: Implement using refcount_t "Reshetova, Elena" <elena.reshetova@intel.com> - 2016-11-18 11:50 +0100
        Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-18 12:00 +0100
          RE: [RFC][PATCH 7/7] kref: Implement using refcount_t "Reshetova, Elena" <elena.reshetova@intel.com> - 2016-11-18 18:00 +0100
            Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-18 20:00 +0100
              RE: [RFC][PATCH 7/7] kref: Implement using refcount_t "Reshetova, Elena" <elena.reshetova@intel.com> - 2016-11-19 08:20 +0100
                Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-19 12:50 +0100
    Re: [RFC][PATCH 2/7] kref: Add kref_read() Christoph Hellwig <hch@infradead.org> - 2016-11-14 19:20 +0100
      Re: [RFC][PATCH 2/7] kref: Add kref_read() Greg KH <gregkh@linuxfoundation.org> - 2016-11-15 08:30 +0100
        Re: [RFC][PATCH 2/7] kref: Add kref_read() Peter Zijlstra <peterz@infradead.org> - 2016-11-15 08:50 +0100
        [PATCH] printk, locking/atomics, kref: Introduce new %pAr and %pAk  format string options for atomic_t and 'struct kref' Ingo Molnar <mingo@kernel.org> - 2016-11-15 09:40 +0100
          [PATCH v2] printk, locking/atomics, kref: Introduce new %pAr and  %pAk format string options for atomic_t and 'struct kref' Ingo Molnar <mingo@kernel.org> - 2016-11-15 09:50 +0100
            Re: [PATCH v2] printk, locking/atomics, kref: Introduce new %pAr and  %pAk format string options for atomic_t and 'struct kref' Peter Zijlstra <peterz@infradead.org> - 2016-11-15 10:30 +0100
              [PATCH v3] printk, locking/atomics, kref: Introduce new %pAa and  %pAk format string options for atomic_t and 'struct kref' Ingo Molnar <mingo@kernel.org> - 2016-11-15 10:50 +0100
            Re: [PATCH v2] printk, locking/atomics, kref: Introduce new %pAr and  %pAk format string options for atomic_t and 'struct kref' kbuild test robot <lkp@intel.com> - 2016-11-15 11:10 +0100
          Re: [PATCH] printk, locking/atomics, kref: Introduce new %pAr and  %pAk format string options for atomic_t and 'struct kref' Linus Torvalds <torvalds@linux-foundation.org> - 2016-11-15 17:50 +0100
            Re: [PATCH] printk, locking/atomics, kref: Introduce new %pAr and  %pAk format string options for atomic_t and 'struct kref' Ingo Molnar <mingo@kernel.org> - 2016-11-16 09:20 +0100
    Re: [RFC][PATCH 0/7] kref improvements Greg KH <gregkh@linuxfoundation.org> - 2016-11-15 08:30 +0100
      Re: [RFC][PATCH 0/7] kref improvements Ingo Molnar <mingo@kernel.org> - 2016-11-15 08:50 +0100
        Re: [RFC][PATCH 0/7] kref improvements Greg KH <gregkh@linuxfoundation.org> - 2016-11-15 16:10 +0100
      Re: [RFC][PATCH 0/7] kref improvements Peter Zijlstra <peterz@infradead.org> - 2016-11-15 08:50 +0100
    Re: [RFC][PATCH 2/7] kref: Add kref_read() Greg KH <gregkh@linuxfoundation.org> - 2016-11-15 08:40 +0100
      Re: [RFC][PATCH 2/7] kref: Add kref_read() Peter Zijlstra <peterz@infradead.org> - 2016-11-15 09:10 +0100
        Re: [RFC][PATCH 2/7] kref: Add kref_read() Kees Cook <keescook@chromium.org> - 2016-11-15 22:00 +0100
          Re: [RFC][PATCH 2/7] kref: Add kref_read() Greg KH <gregkh@linuxfoundation.org> - 2016-11-16 09:30 +0100
            Re: [RFC][PATCH 2/7] kref: Add kref_read() Peter Zijlstra <peterz@infradead.org> - 2016-11-16 11:20 +0100
              Re: [RFC][PATCH 2/7] kref: Add kref_read() Greg KH <gregkh@linuxfoundation.org> - 2016-11-16 11:20 +0100
            Re: [RFC][PATCH 2/7] kref: Add kref_read() Greg KH <gregkh@linuxfoundation.org> - 2016-11-16 11:20 +0100
            Re: [RFC][PATCH 2/7] kref: Add kref_read() Daniel Borkmann <daniel@iogearbox.net> - 2016-11-16 11:20 +0100
          Re: [RFC][PATCH 2/7] kref: Add kref_read() Peter Zijlstra <peterz@infradead.org> - 2016-11-16 11:10 +0100
            Re: [RFC][PATCH 2/7] kref: Add kref_read() Kees Cook <keescook@chromium.org> - 2016-11-16 20:00 +0100
              Re: [RFC][PATCH 2/7] kref: Add kref_read() Peter Zijlstra <peterz@infradead.org> - 2016-11-17 09:40 +0100
                Re: [RFC][PATCH 2/7] kref: Add kref_read() David Windsor <dave@progbits.org> - 2016-11-17 13:50 +0100
                  RE: [RFC][PATCH 2/7] kref: Add kref_read() "Reshetova, Elena" <elena.reshetova@intel.com> - 2016-11-17 15:40 +0100
                    Re: [RFC][PATCH 2/7] kref: Add kref_read() Peter Zijlstra <peterz@infradead.org> - 2016-11-17 18:10 +0100
                      RE: [RFC][PATCH 2/7] kref: Add kref_read() "Reshetova, Elena" <elena.reshetova@intel.com> - 2016-11-17 18:10 +0100
                      RE: [RFC][PATCH 2/7] kref: Add kref_read() "Reshetova, Elena" <elena.reshetova@intel.com> - 2016-11-17 19:10 +0100
                        Re: [RFC][PATCH 2/7] kref: Add kref_read() Peter Zijlstra <peterz@infradead.org> - 2016-11-17 20:20 +0100
                        Re: [RFC][PATCH 2/7] kref: Add kref_read() Peter Zijlstra <peterz@infradead.org> - 2016-11-17 20:40 +0100
                  Re: [RFC][PATCH 2/7] kref: Add kref_read() Peter Zijlstra <peterz@infradead.org> - 2016-11-17 18:20 +0100
                Re: [RFC][PATCH 2/7] kref: Add kref_read() Kees Cook <keescook@chromium.org> - 2016-11-17 20:40 +0100

Page 2 of 5 — ← Prev page 1 [2] 3 4 5  Next page →

#1523755 — Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

On Wed, Nov 16, 2016 at 2:15 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Nov 16, 2016 at 09:31:55AM +0100, Ingo Molnar wrote:
>>
>> * Kees Cook <keescook@chromium.org> wrote:
>>
>> > On Tue, Nov 15, 2016 at 11:16 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>> > >
>> > >
>> > > On 15 November 2016 19:06:28 CET, Kees Cook <keescook@chromium.org> wrote:
>> > >
>> > >>I'll want to modify this in the future; I have a config already doing
>> > >>"Bug on data structure corruption" that makes the warn/bug choice.
>> > >>It'll need some massaging to fit into the new refcount_t checks, but
>> > >>it should be okay -- there needs to be a way to complete the
>> > >>saturation, etc, but still kill the offending process group.
>> > >
>> > > Ideally we'd create a new WARN like construct that continues in kernel space
>> > > and terminates the process on return to user. That way there would be minimal
>> > > kernel state corruption.
>>
>> Yeah, so the problem is that sometimes you are p0wned the moment you return to a
>> corrupted stack, and some of these checks only detect corruption after the fact.
>
> So the case here is about refcounts, with the saturation semantics we
> avoid the use-after-free case which is all this is about. So actually
> continuation of execution is harmless vs the attack vector in question.
>
> Corrupting the stack is another attack vector, one that refcount
> overflow is entirely unrelated to and not one I think we should consider
> here.
>
> The problem with BUG and insta killing the task is that refcounts are
> typically done under locks, if you kill the task before the unlock,
> you've wrecked kernel state in unrecoverable ways.

My intention with what I'm designing is to couple the "panic_on_oops"
sysctl logic with a "kernel structure corruption has been detected"
warning. That way, one can select, at runtime, if the kernel should
panic instantly on hitting this, or just do its best to clean things
up and kill the process. There basically isn't a use-case for BUG in
this situation. Either you're risk-averse enough to want to take the
entire machine down, or you want to kill the offending process and
clean up to continue running.

I'm still evolving how to best do it, and right now it's a rather
large hammer (now controlled by a CONFIG called
CONFIG_BUG_ON_DATA_CORRUPTION in -next but it will likely disappear
entirely as its design has evolved). I intend to improve it first and
then expand its coverage in the kernel. It requires extracting some of
the per-arch BUG logic into a real kernel API, and combining it with
existing pieces of the WARN API.

-Kees

-- 
Kees Cook
Nexus Security

[toc] | [prev] | [next] | [standalone]

#1524164 — Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

On Wed, Nov 16, 2016 at 10:55:16AM -0800, Kees Cook wrote:
> My intention with what I'm designing is to couple the "panic_on_oops"

There is a panic_on_warn knob too.

[toc] | [prev] | [next] | [standalone]

#1524783 — Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

On Thu, Nov 17, 2016 at 12:33 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Nov 16, 2016 at 10:55:16AM -0800, Kees Cook wrote:
>> My intention with what I'm designing is to couple the "panic_on_oops"
>
> There is a panic_on_warn knob too.

Yes, and that tends to be "too much". There is a need to create
something between warn and bug.

-Kees

-- 
Kees Cook
Nexus Security

[toc] | [prev] | [next] | [standalone]

#1523747 — Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

On Wed, Nov 16, 2016 at 12:31 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Kees Cook <keescook@chromium.org> wrote:
>
>> On Tue, Nov 15, 2016 at 11:16 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>> >
>> >
>> > On 15 November 2016 19:06:28 CET, Kees Cook <keescook@chromium.org> wrote:
>> >
>> >>I'll want to modify this in the future; I have a config already doing
>> >>"Bug on data structure corruption" that makes the warn/bug choice.
>> >>It'll need some massaging to fit into the new refcount_t checks, but
>> >>it should be okay -- there needs to be a way to complete the
>> >>saturation, etc, but still kill the offending process group.
>> >
>> > Ideally we'd create a new WARN like construct that continues in kernel space
>> > and terminates the process on return to user. That way there would be minimal
>> > kernel state corruption.
>
> Yeah, so the problem is that sometimes you are p0wned the moment you return to a
> corrupted stack, and some of these checks only detect corruption after the fact.

Exactly.

>> Right, though I'd like to be conservative about the kernel execution
>> continuing... I'll experiment with it.
>
> So what I'd love to see is to have a kernel option that re-introduces some
> historic root (and other) holes that can be exploited deterministically -
> obviously default disabled.
>
> I'd restrict this to reasonably 'deterministic' holes, and the exploits themselves
> could be somewhere in tools/. (Obviously only where the maintainers agree to host
> the code.) They wouldn't give a root shell, they'd only test whether they reached
> uid0 (or some other elevated privilege).

Have you looked at what lkdtm (CONFIG_LKDTM) does? It is explicitly a
collection of specific bad behaviors designed to trigger kernel flaw
mitigations.

> The advantages of such a suite would be:
>
>  - Uptodate tests on modern kernels: It would allow the (controlled) testing of
>    live kernel exploits even on the latest kernel - and would allow the testing of
>    various defensive measures.
>
>  - It would also make sure that defensive measures _remain_ effective against
>    similar categories of bugs. We've had defensive measure regressions in the
>    past, which was only discovered when the next exploit came out ...
>
>  - Testing of new defensive measures: It would help convert this whole
>    probabilistic and emotion driven "kernel protection" business into something
>    somewhat more rational. For example new protection mechanisms should have a
>    demonstrated ability to turn an existing exploit test into something less
>    dangerous.
>
>  - Education: It would teach kernel developers the various patterns of holes,
>    right in the code. Maybe being more directly exposed to what can get you p0wned
>    is both a stronger education force plus it could give people ideas about how to
>    protect better.
>
>  - I also think that collecting the various problems into a single place will give
>    us new insights into patterns, bug counts and various exploit techniques.

Unless I'm missing some detail of your idea, lkdtm already does all of this.

> The disadvantages would be:
>
>  - Maintenance: do we want to add extra (compiled out by default) code to the
>    kernel whose only purpose is to demonstrate certain types of bugs?
>
>  - Exposing exploits: Do we want to host a powerful collection of almost-exploits
>    in tools/ ? I don't think we have a choice but to face the problem directly -
>    but others might disagree.

They don't need to be exploits to test self-protection systems.

> I think most of the negatives could be kept small by starting small, allowing
> maintainers to explicitly opt-in, and observing the effects as we go. But YMMV.

I certainly think lkdtm could be further expanded, but I'd love to see
what you think is specifically missing...

-Kees

-- 
Kees Cook
Nexus Security

[toc] | [prev] | [next] | [standalone]

#1522639 — Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

[Multipart message — attachments visible in raw view] — view raw

Hi Peter,

On Mon, Nov 14, 2016 at 06:39:53PM +0100, Peter Zijlstra wrote:
[...]
> +/*
> + * Similar to atomic_dec_and_test(), it will BUG on underflow and fail to
> + * decrement when saturated at UINT_MAX.
> + *
> + * Provides release memory ordering, such that prior loads and stores are done
> + * before a subsequent free.

I'm not sure this is correct, the RELEASE semantics is for the STORE
part of cmpxchg, and semantically it will guarantee that memory
operations after cmpxchg won't be reordered upwards, for example, on
ARM64, the following code:

	WRITE_ONCE(x, 1)
	
	atomic_cmpxchg_release(&a, 1, 2);
	  r1 = ll(&a)
	  if (r1 == 1) {
	    sc_release(&a, 2);
	  }
	
	free()

could be reordered as, I think:

	atomic_cmpxchg_release(&a, 1, 2);
	  r1 = ll(&a)
	  if (r1 == 1) {
	    free()
	    WRITE_ONCE(x, 1)
	    sc_release(&a, 2);
	  }

Of course, we need to wait for Will to confirm about this. But if this
could happen, we'd better to use a smp_mb()+atomic_cmpxchg_relaxed()
here and for other refcount_dec_and_*().

That said, I think the really ordering guarantee we need here is that
two cmpxchg()s in refcount_dec_and_*() could be paired with each other
to ensure nobody observes a freed object inside a refcount critical
section. In that case, we need atomic_cmpxchg() here for ordering on
both sides. Or maybe replace atomic_read() with smp_load_acquire().

Regards,
Boqun


> + */
> +static inline __must_check
> +bool refcount_dec_and_test(refcount_t *r)
> +{
> +	unsigned int old, new, val = atomic_read(&r->refs);
> +
> +	for (;;) {
> +		if (val == UINT_MAX)
> +			return false;
> +
> +		new = val - 1;
> +		if (new > val)
> +			BUG(); /* underflow */
> +
> +		old = atomic_cmpxchg_release(&r->refs, val, new);
> +		if (old == val)
> +			break;
> +
> +		val = old;
> +	}
> +
> +	return !new;
> +}
> +
> +/*
> + * Similar to atomic_dec_and_mutex_lock(), it will BUG on underflow and fail
> + * to decrement when saturated at UINT_MAX.
> + *
> + * Provides release memory ordering, such that prior loads and stores are done
> + * before a subsequent free. This allows free() while holding the mutex.
> + */
> +static inline __must_check
> +bool refcount_dec_and_mutex_lock(refcount_t *r, struct mutex *lock)
> +{
> +	unsigned int old, new, val = atomic_read(&r->refs);
> +	bool locked = false;
> +
> +	for (;;) {
> +		if (val == UINT_MAX)
> +			return false;
> +
> +		if (val == 1 && !locked) {
> +			locked = true;
> +			mutex_lock(lock);
> +		}
> +
> +		new = val - 1;
> +		if (new > val) {
> +			if (locked)
> +				mutex_unlock(lock);
> +			BUG(); /* underflow */
> +		}
> +
> +		old = atomic_cmpxchg_release(&r->refs, val, new);
> +		if (old == val)
> +			break;
> +
> +		val = old;
> +	}
> +
> +	if (new && locked)
> +		mutex_unlock(lock);
> +
> +	return !new;
> +}
> +
> +/*
> + * Similar to atomic_dec_and_lock(), it will BUG on underflow and fail
> + * to decrement when saturated at UINT_MAX.
> + *
> + * Provides release memory ordering, such that prior loads and stores are done
> + * before a subsequent free. This allows free() while holding the lock.
> + */
> +static inline __must_check
> +bool refcount_dec_and_lock(refcount_t *r, spinlock_t *lock)
> +{
> +	unsigned int old, new, val = atomic_read(&r->refs);
> +	bool locked = false;
> +
> +	for (;;) {
> +		if (val == UINT_MAX)
> +			return false;
> +
> +		if (val == 1 && !locked) {
> +			locked = true;
> +			spin_lock(lock);
> +		}
> +
> +		new = val - 1;
> +		if (new > val) {
> +			if (locked)
> +				spin_unlock(lock);
> +			BUG(); /* underflow */
> +		}
> +
> +		old = atomic_cmpxchg_release(&r->refs, val, new);
> +		if (old == val)
> +			break;
> +
> +		val = old;
> +	}
> +
> +	if (new && locked)
> +		spin_unlock(lock);
> +
> +	return !new;
> +}
> +
> +#endif /* _LINUX_REFCOUNT_H */
> 
> 

[toc] | [prev] | [next] | [standalone]

#1522670 — Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

On Tue, Nov 15, 2016 at 08:33:37PM +0800, Boqun Feng wrote:
> Hi Peter,
> 
> On Mon, Nov 14, 2016 at 06:39:53PM +0100, Peter Zijlstra wrote:
> [...]
> > +/*
> > + * Similar to atomic_dec_and_test(), it will BUG on underflow and fail to
> > + * decrement when saturated at UINT_MAX.
> > + *
> > + * Provides release memory ordering, such that prior loads and stores are done
> > + * before a subsequent free.
> 
> I'm not sure this is correct, the RELEASE semantics is for the STORE
> part of cmpxchg, and semantically it will guarantee that memory
> operations after cmpxchg won't be reordered upwards, for example, on
> ARM64, the following code:
> 
> 	WRITE_ONCE(x, 1)
> 	
> 	atomic_cmpxchg_release(&a, 1, 2);
> 	  r1 = ll(&a)
> 	  if (r1 == 1) {
> 	    sc_release(&a, 2);
> 	  }
> 	
> 	free()
> 
> could be reordered as, I think:
> 
> 	atomic_cmpxchg_release(&a, 1, 2);
> 	  r1 = ll(&a)
> 	  if (r1 == 1) {
> 	    free()
> 	    WRITE_ONCE(x, 1)
> 	    sc_release(&a, 2);
> 	  }
> 
> Of course, we need to wait for Will to confirm about this. But if this
> could happen, we'd better to use a smp_mb()+atomic_cmpxchg_relaxed()
> here and for other refcount_dec_and_*().

Can't happen I think because of the control dependency between
dec_and_test() and free().

That is, the cmpxchg_release() must complete to determine if it was
successful or it needs a retry. The success, combined with the state of
the variable will then determine if we call free().

So I don't think we can get free() (which very much includes stores) to
happen before the store-release.

[toc] | [prev] | [next] | [standalone]

#1522760 — Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

[Multipart message — attachments visible in raw view] — view raw

On Tue, Nov 15, 2016 at 02:01:54PM +0100, Peter Zijlstra wrote:
> On Tue, Nov 15, 2016 at 08:33:37PM +0800, Boqun Feng wrote:
> > Hi Peter,
> > 
> > On Mon, Nov 14, 2016 at 06:39:53PM +0100, Peter Zijlstra wrote:
> > [...]
> > > +/*
> > > + * Similar to atomic_dec_and_test(), it will BUG on underflow and fail to
> > > + * decrement when saturated at UINT_MAX.
> > > + *
> > > + * Provides release memory ordering, such that prior loads and stores are done
> > > + * before a subsequent free.
> > 
> > I'm not sure this is correct, the RELEASE semantics is for the STORE
> > part of cmpxchg, and semantically it will guarantee that memory
> > operations after cmpxchg won't be reordered upwards, for example, on
> > ARM64, the following code:
> > 
> > 	WRITE_ONCE(x, 1)
> > 	
> > 	atomic_cmpxchg_release(&a, 1, 2);
> > 	  r1 = ll(&a)
> > 	  if (r1 == 1) {
> > 	    sc_release(&a, 2);
> > 	  }
> > 	
> > 	free()
> > 
> > could be reordered as, I think:
> > 
> > 	atomic_cmpxchg_release(&a, 1, 2);
> > 	  r1 = ll(&a)
> > 	  if (r1 == 1) {
> > 	    free()
> > 	    WRITE_ONCE(x, 1)
> > 	    sc_release(&a, 2);
> > 	  }
> > 
> > Of course, we need to wait for Will to confirm about this. But if this
> > could happen, we'd better to use a smp_mb()+atomic_cmpxchg_relaxed()
> > here and for other refcount_dec_and_*().
> 
> Can't happen I think because of the control dependency between
> dec_and_test() and free().
> 
> That is, the cmpxchg_release() must complete to determine if it was
> successful or it needs a retry. The success, combined with the state of
> the variable will then determine if we call free().
> 

The thing is that determination of the variable's state(i.e.
store_release() succeeds) and the actual writeback to memory are two
separate events. So yes, free() won't execute before store_release()
commits successfully, but there is no barrier here to order the memory
effects of store_release() and free().

See a similar example:

https://marc.info/?l=linux-s390&m=146604339321723&w=2


But as I said, we actually only need the pairing of orderings:

1) load part of cmpxchg -> free() 
2) object accesses -> store part of cmpxchg

Ordering #1 can be achieved via control dependency as you pointed out
that free()s very much includes stores. And ordering #2 can be achieved
with RELEASE.

So the code is right, I just thought the comment may be misleading. The
reason we use cmpxchg_release() is just for achieving ordering #2, and
not to order "prior loads and stores" with "a subsequent free".

Am I missing some subtle orderings here?

Regards,
Boqun

> So I don't think we can get free() (which very much includes stores) to
> happen before the store-release.

[toc] | [prev] | [next] | [standalone]

#1524184 — Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

On Tue, Nov 15, 2016 at 10:19:09PM +0800, Boqun Feng wrote:
> On Tue, Nov 15, 2016 at 02:01:54PM +0100, Peter Zijlstra wrote:
> > On Tue, Nov 15, 2016 at 08:33:37PM +0800, Boqun Feng wrote:
> > > Hi Peter,
> > > 
> > > On Mon, Nov 14, 2016 at 06:39:53PM +0100, Peter Zijlstra wrote:
> > > [...]
> > > > +/*
> > > > + * Similar to atomic_dec_and_test(), it will BUG on underflow and fail to
> > > > + * decrement when saturated at UINT_MAX.
> > > > + *
> > > > + * Provides release memory ordering, such that prior loads and stores are done
> > > > + * before a subsequent free.
> > > 
> > > I'm not sure this is correct, the RELEASE semantics is for the STORE
> > > part of cmpxchg, and semantically it will guarantee that memory
> > > operations after cmpxchg won't be reordered upwards, for example, on
> > > ARM64, the following code:
> > > 
> > > 	WRITE_ONCE(x, 1)
> > > 	
> > > 	atomic_cmpxchg_release(&a, 1, 2);
> > > 	  r1 = ll(&a)
> > > 	  if (r1 == 1) {
> > > 	    sc_release(&a, 2);
> > > 	  }
> > > 	
> > > 	free()
> > > 
> > > could be reordered as, I think:
> > > 
> > > 	atomic_cmpxchg_release(&a, 1, 2);
> > > 	  r1 = ll(&a)
> > > 	  if (r1 == 1) {
> > > 	    free()
> > > 	    WRITE_ONCE(x, 1)
> > > 	    sc_release(&a, 2);
> > > 	  }
> > > 
> > > Of course, we need to wait for Will to confirm about this. But if this
> > > could happen, we'd better to use a smp_mb()+atomic_cmpxchg_relaxed()
> > > here and for other refcount_dec_and_*().
> > 
> > Can't happen I think because of the control dependency between
> > dec_and_test() and free().
> > 
> > That is, the cmpxchg_release() must complete to determine if it was
> > successful or it needs a retry. The success, combined with the state of
> > the variable will then determine if we call free().
> > 
> 
> The thing is that determination of the variable's state(i.e.
> store_release() succeeds) and the actual writeback to memory are two
> separate events. So yes, free() won't execute before store_release()
> commits successfully, but there is no barrier here to order the memory
> effects of store_release() and free().

Doesn't matter. If we dropped the refcount to 0, nobody else will be
observing this memory anymore (unless ill-formed program). The only
thing we need is that the free() will not be speculated.

This is because all RmW on a specific variable, irrespective of their
memory ordering on other loads/stores, are totally ordered against one
another.

> But as I said, we actually only need the pairing of orderings:
> 
> 1) load part of cmpxchg -> free() 
> 2) object accesses -> store part of cmpxchg
> 
> Ordering #1 can be achieved via control dependency as you pointed out
> that free()s very much includes stores. And ordering #2 can be achieved
> with RELEASE.
> 
> So the code is right, I just thought the comment may be misleading. The
> reason we use cmpxchg_release() is just for achieving ordering #2, and
> not to order "prior loads and stores" with "a subsequent free".
> 
> Am I missing some subtle orderings here?

I would want to further quality 1), it must be no earlier than the load
of the last / successful ll/sc round.

At that point we're guaranteed a reference count of 1 that _will_ drop
to 0, and thus nobody else (should) reference that memory anymore.

If we agree on this, I'll update the comment :-) Will, do you too agree?

[toc] | [prev] | [next] | [standalone]

#1524205 — Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

[Multipart message — attachments visible in raw view] — view raw

On Thu, Nov 17, 2016 at 10:28:00AM +0100, Peter Zijlstra wrote:
> On Tue, Nov 15, 2016 at 10:19:09PM +0800, Boqun Feng wrote:
> > On Tue, Nov 15, 2016 at 02:01:54PM +0100, Peter Zijlstra wrote:
> > > On Tue, Nov 15, 2016 at 08:33:37PM +0800, Boqun Feng wrote:
> > > > Hi Peter,
> > > > 
> > > > On Mon, Nov 14, 2016 at 06:39:53PM +0100, Peter Zijlstra wrote:
> > > > [...]
> > > > > +/*
> > > > > + * Similar to atomic_dec_and_test(), it will BUG on underflow and fail to
> > > > > + * decrement when saturated at UINT_MAX.
> > > > > + *
> > > > > + * Provides release memory ordering, such that prior loads and stores are done
> > > > > + * before a subsequent free.
> > > > 
> > > > I'm not sure this is correct, the RELEASE semantics is for the STORE
> > > > part of cmpxchg, and semantically it will guarantee that memory
> > > > operations after cmpxchg won't be reordered upwards, for example, on
> > > > ARM64, the following code:
> > > > 
> > > > 	WRITE_ONCE(x, 1)
> > > > 	
> > > > 	atomic_cmpxchg_release(&a, 1, 2);
> > > > 	  r1 = ll(&a)
> > > > 	  if (r1 == 1) {
> > > > 	    sc_release(&a, 2);
> > > > 	  }
> > > > 	
> > > > 	free()
> > > > 
> > > > could be reordered as, I think:
> > > > 
> > > > 	atomic_cmpxchg_release(&a, 1, 2);
> > > > 	  r1 = ll(&a)
> > > > 	  if (r1 == 1) {
> > > > 	    free()
> > > > 	    WRITE_ONCE(x, 1)
> > > > 	    sc_release(&a, 2);
> > > > 	  }
> > > > 
> > > > Of course, we need to wait for Will to confirm about this. But if this
> > > > could happen, we'd better to use a smp_mb()+atomic_cmpxchg_relaxed()
> > > > here and for other refcount_dec_and_*().
> > > 
> > > Can't happen I think because of the control dependency between
> > > dec_and_test() and free().
> > > 
> > > That is, the cmpxchg_release() must complete to determine if it was
> > > successful or it needs a retry. The success, combined with the state of
> > > the variable will then determine if we call free().
> > > 
> > 
> > The thing is that determination of the variable's state(i.e.
> > store_release() succeeds) and the actual writeback to memory are two
> > separate events. So yes, free() won't execute before store_release()
> > commits successfully, but there is no barrier here to order the memory
> > effects of store_release() and free().
> 
> Doesn't matter. If we dropped the refcount to 0, nobody else will be
> observing this memory anymore (unless ill-formed program). The only
> thing we need is that the free() will not be speculated.
> 
> This is because all RmW on a specific variable, irrespective of their
> memory ordering on other loads/stores, are totally ordered against one
> another.
> 

Fair enough ;-)

> > But as I said, we actually only need the pairing of orderings:
> > 
> > 1) load part of cmpxchg -> free() 
> > 2) object accesses -> store part of cmpxchg
> > 
> > Ordering #1 can be achieved via control dependency as you pointed out
> > that free()s very much includes stores. And ordering #2 can be achieved
> > with RELEASE.
> > 
> > So the code is right, I just thought the comment may be misleading. The
> > reason we use cmpxchg_release() is just for achieving ordering #2, and
> > not to order "prior loads and stores" with "a subsequent free".
> > 
> > Am I missing some subtle orderings here?
> 
> I would want to further quality 1), it must be no earlier than the load
> of the last / successful ll/sc round.
> 

Great, that's more accurate!

> At that point we're guaranteed a reference count of 1 that _will_ drop
> to 0, and thus nobody else (should) reference that memory anymore.
> 
> If we agree on this, I'll update the comment :-) Will, do you too agree?

Agreed ;-)

Control dependencies and RELEASE are totally enough for the internal
correctness of refcount_t along with its interactivity with free().
People better not reply order guarantees other than this ;-)

Regards,
Boqun

[toc] | [prev] | [next] | [standalone]

#1524290 — Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

On Thu, Nov 17, 2016 at 05:48:51PM +0800, Boqun Feng wrote:
> > > But as I said, we actually only need the pairing of orderings:
> > > 
> > > 1) load part of cmpxchg -> free() 
> > > 2) object accesses -> store part of cmpxchg
> > > 
> > > Ordering #1 can be achieved via control dependency as you pointed out
> > > that free()s very much includes stores. And ordering #2 can be achieved
> > > with RELEASE.
> > > 
> > > So the code is right, I just thought the comment may be misleading. The
> > > reason we use cmpxchg_release() is just for achieving ordering #2, and
> > > not to order "prior loads and stores" with "a subsequent free".
> > > 
> > > Am I missing some subtle orderings here?
> > 
> > I would want to further quality 1), it must be no earlier than the load
> > of the last / successful ll/sc round.
> > 
> 
> Great, that's more accurate!
> 
> > At that point we're guaranteed a reference count of 1 that _will_ drop
> > to 0, and thus nobody else (should) reference that memory anymore.
> > 
> > If we agree on this, I'll update the comment :-) Will, do you too agree?
> 
> Agreed ;-)
> 
> Control dependencies and RELEASE are totally enough for the internal
> correctness of refcount_t along with its interactivity with free().
> People better not reply order guarantees other than this ;-)

Hurm.. let me ruin my own argument.

Since the free() stores could leak upwards until that ll, and object
stores can be delayed until the sc, we still have a problem. Just not
with the thread that free()s or any other thread that knew about the
object.

The problem comes from any other thread doing an allocation, since its
possible to observe the memory as freed while there are stores pending
to it, we can have those delayed stores trample on our freshly allocated
and initialized object.

The stores must really not be before the SC, so I fear we must either
add an smp_wmb() after the release, or punt and use the fully ordered
cmpxchg().

[toc] | [prev] | [next] | [standalone]

#1524324 — Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

On Thu, Nov 17, 2016 at 11:29:59AM +0100, Peter Zijlstra wrote:
> On Thu, Nov 17, 2016 at 05:48:51PM +0800, Boqun Feng wrote:
> > > > But as I said, we actually only need the pairing of orderings:
> > > > 
> > > > 1) load part of cmpxchg -> free() 
> > > > 2) object accesses -> store part of cmpxchg
> > > > 
> > > > Ordering #1 can be achieved via control dependency as you pointed out
> > > > that free()s very much includes stores. And ordering #2 can be achieved
> > > > with RELEASE.
> > > > 
> > > > So the code is right, I just thought the comment may be misleading. The
> > > > reason we use cmpxchg_release() is just for achieving ordering #2, and
> > > > not to order "prior loads and stores" with "a subsequent free".
> > > > 
> > > > Am I missing some subtle orderings here?
> > > 
> > > I would want to further quality 1), it must be no earlier than the load
> > > of the last / successful ll/sc round.
> > > 
> > 
> > Great, that's more accurate!
> > 
> > > At that point we're guaranteed a reference count of 1 that _will_ drop
> > > to 0, and thus nobody else (should) reference that memory anymore.
> > > 
> > > If we agree on this, I'll update the comment :-) Will, do you too agree?
> > 
> > Agreed ;-)
> > 
> > Control dependencies and RELEASE are totally enough for the internal
> > correctness of refcount_t along with its interactivity with free().
> > People better not reply order guarantees other than this ;-)
> 
> Hurm.. let me ruin my own argument.
> 
> Since the free() stores could leak upwards until that ll, and object
> stores can be delayed until the sc, we still have a problem. Just not
> with the thread that free()s or any other thread that knew about the
> object.
> 
> The problem comes from any other thread doing an allocation, since its
> possible to observe the memory as freed while there are stores pending
> to it, we can have those delayed stores trample on our freshly allocated
> and initialized object.
> 
> The stores must really not be before the SC, so I fear we must either
> add an smp_wmb() after the release, or punt and use the fully ordered
> cmpxchg().

And let me note here that RCU users can use a fully relaxed put, because
call_rcu() guarantees a grace-period between the call_rcu and the
free(), which in turn provides a full memory barrier that orders things.

We could actually expose that to driver writers by doing something like:

	kref_put_rcu(struct kref *kref, struct rcu_head *head, rcu_callback_t func)
	{
		if (refcount_dec_and_test_relaxed(&kref->refcount))
			call_rcu(head, func);
	}

Do we want to go there?

[toc] | [prev] | [next] | [standalone]

#1524381 — Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

On Thu, Nov 17, 2016 at 11:39:27AM +0100, Peter Zijlstra wrote:
> On Thu, Nov 17, 2016 at 11:29:59AM +0100, Peter Zijlstra wrote:
> > On Thu, Nov 17, 2016 at 05:48:51PM +0800, Boqun Feng wrote:
> > > > > But as I said, we actually only need the pairing of orderings:
> > > > > 
> > > > > 1) load part of cmpxchg -> free() 
> > > > > 2) object accesses -> store part of cmpxchg
> > > > > 
> > > > > Ordering #1 can be achieved via control dependency as you pointed out
> > > > > that free()s very much includes stores. And ordering #2 can be achieved
> > > > > with RELEASE.
> > > > > 
> > > > > So the code is right, I just thought the comment may be misleading. The
> > > > > reason we use cmpxchg_release() is just for achieving ordering #2, and
> > > > > not to order "prior loads and stores" with "a subsequent free".
> > > > > 
> > > > > Am I missing some subtle orderings here?
> > > > 
> > > > I would want to further quality 1), it must be no earlier than the load
> > > > of the last / successful ll/sc round.
> > > > 
> > > 
> > > Great, that's more accurate!
> > > 
> > > > At that point we're guaranteed a reference count of 1 that _will_ drop
> > > > to 0, and thus nobody else (should) reference that memory anymore.
> > > > 
> > > > If we agree on this, I'll update the comment :-) Will, do you too agree?
> > > 
> > > Agreed ;-)
> > > 
> > > Control dependencies and RELEASE are totally enough for the internal
> > > correctness of refcount_t along with its interactivity with free().
> > > People better not reply order guarantees other than this ;-)
> > 
> > Hurm.. let me ruin my own argument.
> > 
> > Since the free() stores could leak upwards until that ll, and object
> > stores can be delayed until the sc, we still have a problem. Just not
> > with the thread that free()s or any other thread that knew about the
> > object.
> > 
> > The problem comes from any other thread doing an allocation, since its
> > possible to observe the memory as freed while there are stores pending
> > to it, we can have those delayed stores trample on our freshly allocated
> > and initialized object.
> > 
> > The stores must really not be before the SC, so I fear we must either
> > add an smp_wmb() after the release, or punt and use the fully ordered
> > cmpxchg().
> 
> And let me note here that RCU users can use a fully relaxed put, because
> call_rcu() guarantees a grace-period between the call_rcu and the
> free(), which in turn provides a full memory barrier that orders things.
> 
> We could actually expose that to driver writers by doing something like:
> 
> 	kref_put_rcu(struct kref *kref, struct rcu_head *head, rcu_callback_t func)
> 	{
> 		if (refcount_dec_and_test_relaxed(&kref->refcount))
> 			call_rcu(head, func);
> 	}
> 
> Do we want to go there?

No, please no...

[toc] | [prev] | [next] | [standalone]

#1524475 — Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

On Thu, Nov 17, 2016 at 12:03:33PM +0100, Greg KH wrote:
> On Thu, Nov 17, 2016 at 11:39:27AM +0100, Peter Zijlstra wrote:

> > And let me note here that RCU users can use a fully relaxed put, because
> > call_rcu() guarantees a grace-period between the call_rcu and the
> > free(), which in turn provides a full memory barrier that orders things.
> > 
> > We could actually expose that to driver writers by doing something like:
> > 
> > 	kref_put_rcu(struct kref *kref, struct rcu_head *head, rcu_callback_t func)
> > 	{
> > 		if (refcount_dec_and_test_relaxed(&kref->refcount))
> > 			call_rcu(head, func);
> > 	}
> > 
> > Do we want to go there?
> 
> No, please no...

Hehe, fair enough. Although core kernel 'power' users might really want
that. The relaxed memory order stuff saves quite a few cycles on weak
archs.

[toc] | [prev] | [next] | [standalone]

#1524563 — Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

On Thu, Nov 17, 2016 at 06:54:32PM +0800, Boqun Feng wrote:
> 
> If our freshly allocated object got trampled by stores in free(), that's the
> problem of allocator and free(), right? Because in that case, it's them who
> mess up the synchronization.
> 

Oh right, duh.

[toc] | [prev] | [next] | [standalone]

#1524476 — Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

On Thu, Nov 17, 2016 at 05:11:10PM +0100, Peter Zijlstra wrote:
> On Thu, Nov 17, 2016 at 12:08:36PM +0000, Will Deacon wrote:
> > All sounds reasonable to me. It's worth pointing out that you can't create
> > order using a control dependency hanging off the status flag of a
> > store-conditional, but the code in question here has the dependency from
> > the loaded value, which is sufficient.
> 
> Yeah, I'm always surprised by that 'feature'. Is that ARM specific? Or
> so more LL/SC archs have this?

In general, I'm not sure, but I think PPC does allow for the control
dependency.

Will

[toc] | [prev] | [next] | [standalone]

#1525078 — Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

[Multipart message — attachments visible in raw view] — view raw

On Thu, Nov 17, 2016 at 04:36:24PM +0000, Will Deacon wrote:
> On Thu, Nov 17, 2016 at 05:11:10PM +0100, Peter Zijlstra wrote:
> > On Thu, Nov 17, 2016 at 12:08:36PM +0000, Will Deacon wrote:
> > > All sounds reasonable to me. It's worth pointing out that you can't create
> > > order using a control dependency hanging off the status flag of a
> > > store-conditional, but the code in question here has the dependency from
> > > the loaded value, which is sufficient.
> > 
> > Yeah, I'm always surprised by that 'feature'. Is that ARM specific? Or
> > so more LL/SC archs have this?
> 
> In general, I'm not sure, but I think PPC does allow for the control
> dependency.
> 

You guys mean the "control dependency" from a sc to subsequent WRITE,
like in the following litmus?

	PPC sc-control
	""
	{
	0:r11=x;0:r12=y;0:r3=1;0:r10=0;
	1:r11=x;1:r12=y;
	}

	P0                   | P1             ;
	lwarx r2, r10, r11   | lwz r2, 0(r12) ;
	stwcx. r3, r10, r11  | lwsync           ;
	bne Fail             |                ;
	stw r3, 0(r12)       | lwz r1, 0(r11) ;
	Fail:                |                ;

	exists
	(1:r2 = 1 /\ x = 1 /\ 1:r1 = 0)

PPCMEM and herd both said the exists-clause could be triggered
"Sometimes".

And ISA said:

"""
Because a Store Conditional instruction may complete before its store
has been performed, a conditional Branch instruction that depends on the
CR0 value set by a Store Conditional instruction does not order the
Store Conditional's store with respect to storage accesses caused by
instructions that follow the Branch.
"""

So ppc doesn't honor this "control dependency". ;-)

Regards,
Boqun


> Will

[toc] | [prev] | [next] | [standalone]

#1525152 — Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

On Fri, Nov 18, 2016 at 04:26:34PM +0800, Boqun Feng wrote:
> On Thu, Nov 17, 2016 at 04:36:24PM +0000, Will Deacon wrote:
> > On Thu, Nov 17, 2016 at 05:11:10PM +0100, Peter Zijlstra wrote:
> > > On Thu, Nov 17, 2016 at 12:08:36PM +0000, Will Deacon wrote:
> > > > All sounds reasonable to me. It's worth pointing out that you can't create
> > > > order using a control dependency hanging off the status flag of a
> > > > store-conditional, but the code in question here has the dependency from
> > > > the loaded value, which is sufficient.
> > > 
> > > Yeah, I'm always surprised by that 'feature'. Is that ARM specific? Or
> > > so more LL/SC archs have this?
> > 
> > In general, I'm not sure, but I think PPC does allow for the control
> > dependency.
> > 
> 
> You guys mean the "control dependency" from a sc to subsequent WRITE,
> like in the following litmus?
> 
> 	PPC sc-control
> 	""
> 	{
> 	0:r11=x;0:r12=y;0:r3=1;0:r10=0;
> 	1:r11=x;1:r12=y;
> 	}
> 
> 	P0                   | P1             ;
> 	lwarx r2, r10, r11   | lwz r2, 0(r12) ;
> 	stwcx. r3, r10, r11  | lwsync           ;
> 	bne Fail             |                ;
> 	stw r3, 0(r12)       | lwz r1, 0(r11) ;
> 	Fail:                |                ;
> 
> 	exists
> 	(1:r2 = 1 /\ x = 1 /\ 1:r1 = 0)
> 
> PPCMEM and herd both said the exists-clause could be triggered
> "Sometimes".
> 
> And ISA said:
> 
> """
> Because a Store Conditional instruction may complete before its store
> has been performed, a conditional Branch instruction that depends on the
> CR0 value set by a Store Conditional instruction does not order the
> Store Conditional's store with respect to storage accesses caused by
> instructions that follow the Branch.
> """
> 
> So ppc doesn't honor this "control dependency". ;-)

Oh, wow! I was feeling like the odd duck after talking to paulmck and
Alan Stern about this at LPC, but now it looks like I have company on
the island of misfit memory models.

Will

[toc] | [prev] | [next] | [standalone]

#1524481 — Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

On Thu, Nov 17, 2016 at 10:28:00AM +0100, Peter Zijlstra wrote:
> On Tue, Nov 15, 2016 at 10:19:09PM +0800, Boqun Feng wrote:
> > But as I said, we actually only need the pairing of orderings:
> > 
> > 1) load part of cmpxchg -> free() 
> > 2) object accesses -> store part of cmpxchg
> > 
> > Ordering #1 can be achieved via control dependency as you pointed out
> > that free()s very much includes stores. And ordering #2 can be achieved
> > with RELEASE.
> > 
> > So the code is right, I just thought the comment may be misleading. The
> > reason we use cmpxchg_release() is just for achieving ordering #2, and
> > not to order "prior loads and stores" with "a subsequent free".
> > 
> > Am I missing some subtle orderings here?
> 
> I would want to further quality 1), it must be no earlier than the load
> of the last / successful ll/sc round.
> 
> At that point we're guaranteed a reference count of 1 that _will_ drop
> to 0, and thus nobody else (should) reference that memory anymore.
> 
> If we agree on this, I'll update the comment :-) Will, do you too agree?

All sounds reasonable to me. It's worth pointing out that you can't create
order using a control dependency hanging off the status flag of a
store-conditional, but the code in question here has the dependency from
the loaded value, which is sufficient.

Will

[toc] | [prev] | [next] | [standalone]

#1524571 — Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

On Thu, Nov 17, 2016 at 12:08:36PM +0000, Will Deacon wrote:
> All sounds reasonable to me. It's worth pointing out that you can't create
> order using a control dependency hanging off the status flag of a
> store-conditional, but the code in question here has the dependency from
> the loaded value, which is sufficient.

Yeah, I'm always surprised by that 'feature'. Is that ARM specific? Or
so more LL/SC archs have this?

[toc] | [prev] | [next] | [standalone]

#1525148 — RE: [RFC][PATCH 7/7] kref: Implement using refcount_t

>Provide refcount_t, an atomic_t like primitive built just for refcounting.
>It provides overflow and underflow checks as well as saturation semantics such that when it overflows, we'll never attempt to free it again, ever.

Peter do you have the changes to the refcount_t interface compare to the version in this patch? 
We are now starting working on atomic_t --> refcount_t conversions and it would save a bit of work to have latest version from you that we can be based upon. 

Best Regards,
Elena.

[toc] | [prev] | [next] | [standalone]

Page 2 of 5 — ← Prev page 1 [2] 3 4 5  Next page →

Back to top | Article view | linux.kernel

csiph-web