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

From: Kees Cook <keescook@chromium.org>
To: Will Deacon <will@kernel.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Jan Glauber <jglauber@marvell.com>,
	Will Deacon <will.deacon@arm.com>,
	Jayachandran Chandrasekharan Nair <jnair@marvell.com>,
	Hanjun Guo <guohanjun@huawei.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v5] arm64: kernel: implement fast refcount checking
Date: Mon, 22 Jul 2019 09:43:54 -0700	[thread overview]
Message-ID: <201907220942.69F916657@keescook> (raw)
In-Reply-To: <20190710122117.kk5xgei56r7vfmmj@willie-the-truck>

On Wed, Jul 10, 2019 at 01:21:18PM +0100, Will Deacon wrote:
> On Wed, Jul 03, 2019 at 08:12:12PM +0200, Ard Biesheuvel wrote:
> > 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.
> 
> I'll post the patches after the merge window, but I've pushed them here in
> the meantime in case anybody gets a chance to take them for a spin:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git/log/?h=refcount/full

Can the last patch get split up a bit? There's the inlining move, then
there is the atomic_fetch*() changes. Putting that all in one patch
makes the series a bit harder to review.

(Also, what happened to the *_checked() variations?)

-- 
Kees Cook

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