[patch, mips, memmove] Remove memmove's use of memcpy on MIPS

[patch, mips, memmove] Remove memmove's use of memcpy on MIPS

[Steve Ellcey]

While doing some other testing Maciej Rozycki found a bug in the MIPS
memmove.  A few weeks ago (after the release of 2.17) I updated the MIPS
memcpy.S to use the prepare-to-store prefetch instruction.  This makes
memcpy (particularly long memcpy's) much faster.  The problem is that the
prepare-to-store prefetch is not a 'real' prefetch, it doesn't read the
memory into the cache but, if the data is not already in cache, it creates a
cache line with all zeros and assumes that the user will write to the entire
line.  If you try to read from that cache line or if you don't write to the
entire cache line before it is flushed you will get bad results.

By setting MEMCPY_OK_FOR_FWD_MEMMOVE in memmove.c, we are causing memmove
to call memcpy for overlapping forward memmoves and while the MIPS memcpy
does correctly read the source bytes before writing them, it may do a
(prepare-to-store) prefetch on data in the source buffer before reading it.
This will corrupt the data it is trying to read and give bad results.

Rather then use a slower safer prefetch for memcpy, which would also fix
this problem,  I would like to remove the setting of MEMCPY_OK_FOR_FWD_MEMMOVE
in memmove.c.

OK for checkin?

Steve Ellcey
sellcey@mips.com



2013-01-25  Steve Ellcey  <sellcey@mips.com>

	* sysdeps/mips/memmove.c: Do not set MEMCPY_OK_FOR_FWD_MEMMOVE.



diff --git a/ports/sysdeps/mips/memmove.c b/ports/sysdeps/mips/memmove.c
index fd36859..ae312d1 100644
--- a/ports/sysdeps/mips/memmove.c
+++ b/ports/sysdeps/mips/memmove.c
@@ -18,6 +18,8 @@
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
-/* MIPS' implementation of memcpy is safe to use for memmove.  */
-#define MEMCPY_OK_FOR_FWD_MEMMOVE 1
+/* MIPS' implementation of memcpy is not safe to use for memmove
+   due to the use of the prepare-to-store prefetch so we do not
+   define MEMCPY_OK_FOR_FWD_MEMMOVE.  */
+
 #include <string/memmove.c>

Re: [patch, mips, memmove] Remove memmove's use of memcpy on MIPS

[Chris Metcalf]

On 1/25/2013 1:17 PM, Steve Ellcey wrote:
> While doing some other testing Maciej Rozycki found a bug in the MIPS
> memmove.  A few weeks ago (after the release of 2.17) I updated the MIPS
> memcpy.S to use the prepare-to-store prefetch instruction.  This makes
> memcpy (particularly long memcpy's) much faster.  The problem is that the
> prepare-to-store prefetch is not a 'real' prefetch, it doesn't read the
> memory into the cache but, if the data is not already in cache, it creates a
> cache line with all zeros and assumes that the user will write to the entire
> line.  If you try to read from that cache line or if you don't write to the
> entire cache line before it is flushed you will get bad results.

See ports/sysdeps/tile/tilegx/memcpy.c for how the tile architecture handles this issue.  The memcpy code advances until the destination pointer is cache-aligned, then loops cacheline at a time, first reading 64 bytes, then doing the prepare-to-store (aka "wh64"), then writing all 64 bytes.  This ensures that no matter what overlap the source has with the destination, we have captured any data that might be overwritten by the "wh64" instruction.  Since our memory architecture is non-blocking we can issue all eight reads without waiting for them to complete.

Would doing something like this for MIPS allow your memcpy to be safe for forward memmove?

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [patch, mips, memmove] Remove memmove's use of memcpy on MIPS

[Steve Ellcey]

On Fri, 2013-01-25 at 13:55 -0500, Chris Metcalf wrote:

> 
> See ports/sysdeps/tile/tilegx/memcpy.c for how the tile architecture handles this issue.
> The memcpy code advances until the destination pointer is cache-aligned, then loops cacheline
> at a time, first reading 64 bytes, then doing the prepare-to-store (aka "wh64"), then writing
> all 64 bytes.  This ensures that no matter what overlap the source has with the destination,
> we have captured any data that might be overwritten by the "wh64" instruction.  Since our memory
> architecture is non-blocking we can issue all eight reads without waiting for them to complete.
> 
> Would doing something like this for MIPS allow your memcpy to be safe for forward memmove?

I like this implementation, unfortunately I don't have a
CHIP_L2_LINE_SIZE macro to check the cache line size and my cache line
size could be anywhere from 16 to 128 bytes.  I currently optimize for
32 bytes but make sure it works correctly for anything less then 128
bytes.  I like the use of 'safe' prefetches when you can't use 'wh64'. I
may experiment and see if I can create a C version of memcpy like you
have that works as well or better then my existing asm version, but the
use of the MIPS partial word loads (lwl/lwr, ldl/ldr) in my assembly
version may make it difficult, or at least make it require more inline
assembly.

I think I still need to check in my current patch for now to fix ToT
memmove while I look into making a better memcpy.

Steve Ellcey
sellcey@mips.com

Re: [patch, mips, memmove] Remove memmove's use of memcpy on MIPS

[Joseph S. Myers]

On Fri, 25 Jan 2013, Steve Ellcey  wrote:

> Rather then use a slower safer prefetch for memcpy, which would also fix
> this problem,  I would like to remove the setting of MEMCPY_OK_FOR_FWD_MEMMOVE
> in memmove.c.

Removing the MIPS memmove.c is OK.  There's no point in having the file 
when it doesn't define MEMCPY_OK_FOR_FWD_MEMMOVE

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [patch, mips, memmove] Remove memmove's use of memcpy on MIPS

[Steve Ellcey]

On Fri, 2013-01-25 at 20:16 +0000, Joseph S. Myers wrote:
> On Fri, 25 Jan 2013, Steve Ellcey  wrote:
> 
> > Rather then use a slower safer prefetch for memcpy, which would also fix
> > this problem,  I would like to remove the setting of MEMCPY_OK_FOR_FWD_MEMMOVE
> > in memmove.c.
> 
> Removing the MIPS memmove.c is OK.  There's no point in having the file 
> when it doesn't define MEMCPY_OK_FOR_FWD_MEMMOVE

I am hoping to improve memcpy at some point so that it will be safe to
use memcpy in memmove again.  Given that, do you think I should still
just remove the MIPS memmove.c and recreate it later when needed or
would it make sense to keep it so I can just set the macro when it is
safe to do so.

Steve Ellcey
sellcey@mips.com

Re: [patch, mips, memmove] Remove memmove's use of memcpy on MIPS

[Joseph S. Myers]

On Fri, 25 Jan 2013, Steve Ellcey wrote:

> I am hoping to improve memcpy at some point so that it will be safe to
> use memcpy in memmove again.  Given that, do you think I should still
> just remove the MIPS memmove.c and recreate it later when needed or
> would it make sense to keep it so I can just set the macro when it is
> safe to do so.

It should be removed, as currently not useful, then recreated if and when 
such a file is useful again.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [patch, mips, memmove] Remove memmove's use of memcpy on MIPS

[Maxim Kuvyrkov]

On 26/01/2013, at 10:28 AM, Steve Ellcey wrote:

> On Fri, 2013-01-25 at 20:16 +0000, Joseph S. Myers wrote:
>> On Fri, 25 Jan 2013, Steve Ellcey  wrote:
>> 
>>> Rather then use a slower safer prefetch for memcpy, which would also fix
>>> this problem,  I would like to remove the setting of MEMCPY_OK_FOR_FWD_MEMMOVE
>>> in memmove.c.
>> 
>> Removing the MIPS memmove.c is OK.  There's no point in having the file 
>> when it doesn't define MEMCPY_OK_FOR_FWD_MEMMOVE
> 
> I am hoping to improve memcpy at some point so that it will be safe to
> use memcpy in memmove again.  Given that, do you think I should still
> just remove the MIPS memmove.c and recreate it later when needed or
> would it make sense to keep it so I can just set the macro when it is
> safe to do so.

If you want to keep memmove at top speed you could add a memmove-safe instance of memcpy.S and then use that one in memmove.  I.e. (roughly),

sysdeps/mips/memmove.c:
#define MEMCPY memcpy_for_memmove
#define MEMCPY_OK_FOR_FWD_MEMMOVE 1
#include <string/memmove.c> // Adjust string/memmove.c to use "MEMCPY" instead of "memcpy".

sysdeps/mips/memcpy_for_memmove.S:
#define MEMCPY memcpy_for_memmove
#define HIDDEN .hidden // Don't export this copy of memcpy.
#define PREFETCH ... // Define safe prefetch.
#include "memcpy.S"

--
Maxim Kuvyrkov
KugelWorks