Re: [PATCH] string: Improve the generic strlcpy() implementation

[Chris Metcalf <cmetcalf@ezchip.com>]

On 10/19/2015 08:42 AM, Rasmus Villemoes wrote:
> On Mon, Oct 05 2015, Ingo Molnar <mingo@kernel.org> wrote:
>> Interesting. I noticed that strscpy() says this in its comments:
>>
>>   * In addition, the implementation is robust to the string changing out
>>   * from underneath it, unlike the current strlcpy() implementation.
> Apologies for beating a dead horse, but:
>
> 		c = *(unsigned long *)(src+res);
> 		if (has_zero(c, &data, &constants)) {
> 			data = prep_zero_mask(c, data, &constants);
> 			data = create_zero_mask(data);
> 			*(unsigned long *)(dest+res) = c & zero_bytemask(data);
> 			return res + find_zero(data);
> 		}
> 		*(unsigned long *)(dest+res) = c;
>
> I wonder whether an insane compiler might actually reload c before
> storing to dest+res, so that we'd have exactly the same problem of
> embedded nul characters?

Using READ_ONCE() on x86_64 with gcc 4.8 adds a bit of
overhead; for some reason the compiler converts some
mov instructions to lea+mov, unnecessarily as far as I can see.
And looking at older compilers, gcc 4.4 substantially
pessimizes the output with READ_ONCE, though perhaps we
don't care about compilers that old.

Still, your argument is certainly plausible in terms of
actually guaranteeing the semantics we are claiming rather
than leaving it to the compiler to Do The Right Thing.
Unsurprisingly, the current code is causing the compiler
to do the right thing, but of course past performance is no
guarantee of future results, as they say.

So I'm kind of on the fence, maybe leaning slightly
towards thinking the READ_ONCE semantics is worth it.
Maybe someone with a tip gcc can see if the performance
difference has been fixed there?

-- 
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com