Re: [U-Boot] [PATCH 1/3] spl: ram: Do not memcpy() identical buffers
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 Cc: Michal Simek Cc: Tom Rini >>> >>> 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 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 1/3] spl: ram: Do not memcpy() identical buffers
On 5/28/2019 4:19 AM, 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 Cc: Michal Simek Cc: Tom Rini 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 don't think this is an optimization question, really. Calling memcpy with overlapping areas is an error. The result is explicitly undefined. It may well be that all the existing implementations effectively do nothing, either by explicit check or by actually copying the data over itself. However, to rely on that behavior is asking for trouble down the line. Undefined behavior is exactly that. Don't do it. ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 1/3] spl: ram: Do not memcpy() identical buffers
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 > >> Cc: Michal Simek > >> Cc: Tom Rini > > > > 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. -- Tom signature.asc Description: PGP signature ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 1/3] spl: ram: Do not memcpy() identical buffers
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 >> Cc: Michal Simek >> Cc: Tom Rini > > 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 -- Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 1/3] spl: ram: Do not memcpy() identical buffers
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 > Cc: Michal Simek > Cc: Tom Rini > >>> > >>> 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! -- Tom signature.asc Description: PGP signature ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 1/3] spl: ram: Do not memcpy() identical buffers
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 Cc: Michal Simek Cc: Tom Rini >>> >>> 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 ? -- Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 1/3] spl: ram: Do not memcpy() identical buffers
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 > >> Cc: Michal Simek > >> Cc: Tom Rini > > > > 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? -- Tom signature.asc Description: PGP signature ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 1/3] spl: ram: Do not memcpy() identical buffers
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 >> Cc: Michal Simek >> Cc: Tom Rini > > 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. -- Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 1/3] spl: ram: Do not memcpy() identical buffers
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 > Cc: Michal Simek > Cc: Tom Rini Shouldn't memcpy catch that itself? -- Tom signature.asc Description: PGP signature ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH 1/3] spl: ram: Do not memcpy() identical buffers
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
Cc: Michal Simek
Cc: Tom Rini
---
common/spl/spl_ram.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/common/spl/spl_ram.c b/common/spl/spl_ram.c
index 954e91a004..2ef33f717e 100644
--- a/common/spl/spl_ram.c
+++ b/common/spl/spl_ram.c
@@ -24,7 +24,8 @@ static ulong spl_ram_load_read(struct spl_load_info *load,
ulong sector,
{
debug("%s: sector %lx, count %lx, buf %lx\n",
__func__, sector, count, (ulong)buf);
- memcpy(buf, (void *)(CONFIG_SPL_LOAD_FIT_ADDRESS + sector), count);
+ if (buf != CONFIG_SPL_LOAD_FIT_ADDRESS + sector)
+ memcpy(buf, (void *)(CONFIG_SPL_LOAD_FIT_ADDRESS + sector),
count);
return count;
}
--
2.20.1
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
