Re: [PATCH v5] arm64: kernel: implement fast refcount checking

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On Wed, 3 Jul 2019 at 15:40, Will Deacon <will@kernel.org> wrote:
>
> Hi Ard,
>
> On Wed, Jun 19, 2019 at 12:54:31PM +0200, Ard Biesheuvel wrote:
> > This adds support to arm64 for fast refcount checking, as contributed
> > by Kees for x86 based on the implementation by grsecurity/PaX.
> >
> > The general approach is identical: the existing atomic_t helpers are
> > cloned for refcount_t, with the arithmetic instruction modified to set
> > the PSTATE flags, and one or two branch instructions added that jump to
> > an out of line handler if overflow, decrement to zero or increment from
> > zero are detected.
> >
> > One complication that we have to deal with on arm64 is the fact that
> > it has two atomics implementations: the original LL/SC implementation
> > using load/store exclusive loops, and the newer LSE one that does mostly
> > the same in a single instruction. So we need to clone some parts of
> > both for the refcount handlers, but we also need to deal with the way
> > LSE builds fall back to LL/SC at runtime if the hardware does not
> > support it.
>
> I've been looking at this today and I still don't understand why this can't
> be written as simple wrappers around atomic_t. What am I missing? To be more
> concrete, why can't we implement the refcount functions along the lines of
> the code below?
>

There was a lot of pushback against the use of refcount_t in the
beginning, given that the checked flavor was slower than unchecked
atomic_t, and IIRC, it was mainly the networking folks that opposed
it. So the whole idea is that the code performs as closely to atomic_t
as possible, which is why the code is simply the atomic_t asm
implementations, but with a -s suffix added to the arithmetic
instructions so they set PSTATE, and one or two conditional branch
instructions added.

Your approach is going to require one or two additional compare
instructions, increasing the instruction count. This may not matter on
fast OoO cores, but it probably will affect somebody's benchmark
somewhere.

However, I'd be in favour of switching to your code, since it is much
simpler and more maintainable, so if you spin it as a proper patch, we
can do some comparative analysis of the performance.

> We can't call refcount_error_report() like this, but perhaps a
> WARN_ON_RATELIMIT would be enough. However, I'm sure there's a reason
> for the complexity in your proposal -- I just can't spot it.
>


> --->8
>
> static __always_inline void refcount_add(int i, refcount_t *r)
> {
>         int old = atomic_fetch_add_relaxed(i, &r->refs);
>
>         if (unlikely(old <= 0 || old + i <= 0))
>                 refcount_set(r, INT_MIN / 2);
> }
>
> static __always_inline void refcount_inc(refcount_t *r)
> {
>         refcount_add(1, r);
> }
>
> static __always_inline void refcount_dec(refcount_t *r)
> {
>         int old = atomic_fetch_sub_release(1, &r->refs);
>
>         if (unlikely(old <= 1))
>                 refcount_set(r, INT_MIN / 2);
> }
>
> static __always_inline __must_check bool refcount_sub_and_test(int i,
>                                                                refcount_t *r)
> {
>         int old = atomic_fetch_sub(i, &r->refs);
>
>         if (old - i < 0)
>                 refcount_set(r, INT_MIN / 2);
>
>         return old == i;
> }
>
> static __always_inline __must_check bool refcount_dec_and_test(refcount_t *r)
> {
>         return refcount_sub_and_test(1, r);
> }
>
> static __always_inline __must_check bool refcount_add_not_zero(int i,
>                                                                refcount_t *r)
> {
>         int old = refcount_read(r);
>
>         do {
>                 if (!old)
>                         break;
>         } while (!atomic_try_cmpxchg_relaxed(&r->refs, &old, old + i));
>
>         if (old < 0 || old + i < 0)
>                 refcount_set(r, INT_MIN / 2);
>
>         return old;
> }
>
> static __always_inline __must_check bool refcount_inc_not_zero(refcount_t *r)
> {
>         return refcount_add_not_zero(1, r);
> }

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel