[U-Boot] [PATCH 1/3] spl: ram: Do not memcpy() identical buffers

[Marek Vasut]

On 5/28/19 1:19 PM, Tom Rini wrote:
> On Tue, May 28, 2019 at 05:24:34AM +0200, Marek Vasut wrote:
>> On 5/28/19 5:04 AM, Tom Rini wrote:
>>> On Tue, May 28, 2019 at 04:44:52AM +0200, Marek Vasut wrote:
>>>> On 5/28/19 4:42 AM, Tom Rini wrote:
>>>>> On Tue, May 28, 2019 at 04:07:44AM +0200, Marek Vasut wrote:
>>>>>> On 5/28/19 4:06 AM, Tom Rini wrote:
>>>>>>> On Tue, May 28, 2019 at 03:49:13AM +0200, Marek Vasut wrote:
>>>>>>>
>>>>>>>> If the source and destination buffer address is identical, there is
>>>>>>>> no need to memcpy() the content. Skip the memcpy() in such a case.
>>>>>>>>
>>>>>>>> Signed-off-by: Marek Vasut <marex at denx.de>
>>>>>>>> Cc: Michal Simek <michal.simek at xilinx.com>
>>>>>>>> Cc: Tom Rini <trini at konsulko.com>
>>>>>>>
>>>>>>> Shouldn't memcpy catch that itself?
>>>>>>>
>>>>>> memcpy(3) says
>>>>>>        The memcpy() function copies n bytes from memory area src to
>>>>>> memory area dest.  The memory areas must not overlap.  Use memmove(3) if
>>>>>> the memory areas do overlap.
>>>>>
>>>>> OK, and shouldn't memcpy optimize that case?  Does it usually?
>>>>
>>>> As the manpage says "The memory areas must not overlap." , I would
>>>> expect it does not have to ?
>>>
>>> I guess I'm not being clear enough, sorry.  Go look at how this is
>>> implemented in a few places please and report back to us.  Someone else,
>>> or many someone else, have probably already figured out if optimizing
>>> this case in general, in memcpy, is a good idea or not.  Thanks!
>>
>> If even [1] says the behavior is undefined, then what does it matter
>> whether some implementation added an optimization for such undefined
>> behavior? We cannot depend on it, can we ?
>>
>> [1] https://pubs.opengroup.org/onlinepubs/009695399/functions/memcpy.html
> 
> Yes, yes it would be worth seeing if other groups that have looked into
> the performance impact here have also decided to optimize this undefined
> behavior or not, thanks.

I will just drop this patch, since U-Boot memcpy() implementation does
this check. But let me be very clear here, that check is part of
undefined behavior (!) and I don't think it's the right thing to do in
memcpy() itself.

-- 
Best regards,
Marek Vasut