MIME-Version: 1.0
References: <20230228132118.978145284@linutronix.de> <20230228132910.991359171@linutronix.de>
In-Reply-To: <20230228132910.991359171@linutronix.de>
From:   Linus Torvalds <torvalds@linuxfoundation.org>
Date:   Tue, 28 Feb 2023 16:42:23 -0800
Message-ID: <CAHk-=wjeMbHK61Ee+Ug4w8AGHCSDx94GuLs5bPXhHNhA_+RjzA@mail.gmail.com>
Subject: Re: [patch 2/3] atomics: Provide rcuref - scalable reference counting
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     LKML <linux-kernel@vger.kernel.org>, x86@kernel.org,
        Wangyang Guo <wangyang.guo@intel.com>,
        Arjan Van De Ven <arjan.van.de.ven@intel.com>,
        Will Deacon <will@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Boqun Feng <boqun.feng@gmail.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Marc Zyngier <maz@kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, netdev@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Feb 28, 2023 at 6:33=E2=80=AFAM Thomas Gleixner <tglx@linutronix.de=
> wrote:
>
> This unconditional increment avoids the inc_not_zero() problem, but
> requires a more complex implementation on the put() side when the count
> drops from 1 to 0.
>
> When this transition is detected then it is attempted to mark the referen=
ce
> count dead, by setting it to the midpoint of the dead zone with a single
> atomic_cmpxchg_release() operation. This operation can fail due to a
> concurrent rcuref_get() elevating the reference count from 0 to 1.

This looks sane to me, however it does look like the code is not really opt=
imal.

This is supposed to be a critical function, and is inlined:

> +static inline __must_check bool rcuref_get(rcuref_t *ref)
> +{
> +       unsigned int old =3D atomic_fetch_add_relaxed(1, &ref->refcnt);
> +
> +       if (likely(old < RCUREF_MAXREF))
> +               return true;

but that comparison would be much better if RCUREF_MAXREF was
0x80000000 and you'd end up just checking the sign of the result,
instead of checking big numbers.

Also, this optimal value choice ends up being architecture-specific,
since some do the "fetch_add", and others tend to prefer "add_return",
and so the point that is cheapest to check ends up depending on which
architecture it is.

This may seem like nit-picking, but I absolutely *HATE* our current
refcount interface for how absolutely horrid the code generation ends
up being. It's gotten better, but it's still not great.

So if we're introducing yet another refcount interface, and it's done
in the name of efficiency, I would *really* want it to actually be
exactly that: efficient. Not some half-way thing.

And yes, that may mean that it should have some architecture-specific
code (with fallback defaults for the generic case).

               Linus