Re: [PATCH 8/8] arm64: Rewrite __arch_clear_user()

From: Robin Murphy <robin.murphy@arm.com>
To: Mark Rutland <mark.rutland@arm.com>
Cc: will@kernel.org, catalin.marinas@arm.com,
	linux-arm-kernel@lists.infradead.org, yangyingliang@huawei.com
Subject: Re: [PATCH 8/8] arm64: Rewrite __arch_clear_user()
Date: Wed, 12 May 2021 14:51:01 +0100	[thread overview]
Message-ID: <f2a48ade-ad95-c17e-1a12-e4d038f0dc11@arm.com> (raw)
In-Reply-To: <20210512130418.GF88854@C02TD0UTHF1T.local>

On 2021-05-12 14:06, Mark Rutland wrote:
> On Wed, May 12, 2021 at 12:31:39PM +0100, Robin Murphy wrote:
>> On 2021-05-12 11:48, Mark Rutland wrote:
>>> On Tue, May 11, 2021 at 05:12:38PM +0100, Robin Murphy wrote:
>>>> Rewrite __arch_clear_user() with regular
>>>> USER() annotations so that it's clearer what's going on, and take the
>>>> opportunity to minimise the branchiness in the most common paths, which
>>>> also allows the exception fixup to return a more accurate result.
>>>
>>> IIUC this isn't always accurate for the {4,2,1}-byte cases; example
>>> below. I'm not sure whether that's intentional since the commit message
>>> says "more accurate" rather than "accurate".
>>
>> Indeed, the "more" was definitely significant :)
> 
> :)
> 
>>> If we think that under-estimating is fine, I reckon it'd be worth a
>>> comment to make that clear.
>>
>> Indeed for smaller amounts there's no change in fixup behaviour at all, but
>> I have to assume that underestimating by up to 100% is probably OK since
>> we've been underestimating by fully 100% for nearly 10 years now. I don't
>> believe it's worth having any more complexity than necessary for the fault
>> case - grepping for clear_user() usage suggests that nobody really cares
>> about the return value beyond whether it's zero or not, so the minor
>> "improvement" here is more of a nice-to-have TBH.
>>
>> The existing comment doesn't actually explain anything either, which is why
>> I didn't replace it, but I'm happy to add something if you like.
> 
> I don't have strong feelings either way, but I think that we should at
> least document this, since that'll at least save us rehashing the same
> point in future. :)
> 
> That said, IIUC to make this always accurate we only need two ADDs (diff
> below). Since those will only be executed at most once each, I suspect
> they won't have a measureable impact in practice.
> 
> So maybe it's worth adding them to avoid any risk that someone needs
> this to be accurate in future?

Hmm, now that you've caused me to ponder it some more, it can in fact be 
achieved with just two extra ADDs _out of line_, and still neatly enough 
that I'm now definitely going to do that. Thanks for the push!

Robin.

> 
> Mark.
> 
> ---->8----
> diff --git a/arch/arm64/lib/clear_user.S b/arch/arm64/lib/clear_user.S
> index 1005345b4066..7ef553ec2677 100644
> --- a/arch/arm64/lib/clear_user.S
> +++ b/arch/arm64/lib/clear_user.S
> @@ -32,12 +32,14 @@ USER(9f, sttr       xzr, [x2, #-8])
>   
>   2:     tbz     x1, #2, 3f
>   USER(9f, sttr  wzr, [x0])
> +       add     x0, x0, #4
>   USER(9f, sttr  wzr, [x2, #-4])
>          mov     x0, #0
>          ret
>   
>   3:     tbz     x1, #1, 4f
>   USER(9f, sttrh wzr, [x0])
> +       add     x0, x0, #2
>   4:     tbz     x1, #0, 5f
>   USER(9f, sttrb wzr, [x2, #-1])
>   5:     mov     x0, #0
> 

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