Date: Fri, 23 Jul 2010 12:14:40 +0100
From: Colin Watson <cjwatson@ubuntu.com>
To: The development of GNU GRUB <grub-devel@gnu.org>
Message-ID: <20100723111440.GU21862@riva.ucam.org>
References: <20100623213838.GI21862@riva.ucam.org> <4C24F508.6040501@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <4C24F508.6040501@gmail.com>
User-Agent: Mutt/1.5.18 (2008-05-17)
Subject: Re: [PATCH] Optimise memset on i386
Precedence: list
Reply-To: The development of GNU GRUB <grub-devel@gnu.org>

On Fri, Jun 25, 2010 at 08:27:20PM +0200, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> > +void *
> > +grub_memset (void *s, int c, grub_size_t n)
> > +{
> > +  unsigned char *p = (unsigned char *) s;
> > +
> > +  while (n--)
> > +    *p++ = (unsigned char) c;
> > +
> > +  return s;
> > +}
> 
> Attached is a possible generic implementation. Not even compile-tested.
> Could you test it and compare disasm of this version on i386 and glibc
> i386 version. Perhaps they are equivalent. If they are for maintainance
> reasons it's better to always use generic one.

Thanks for this, and sorry for my delay.

I haven't compared the disassembly, but once I fixed up your code it
performed within about 10% of the optimised version I posted before, so
about 640ms original, 160ms with i386-specific memset, and 176ms with
your generic memset.  Even though that isn't quite equivalent, I don't
think that 16ms is going to be a major problem, and so I would suggest
going with the generic version.  Please check the following.

2010-07-23  Vladimir Serbinenko  <phcoder@gmail.com>
2010-07-23  Colin Watson  <cjwatson@ubuntu.com>

	* kern/misc.c (grub_memset): Optimise to reduce cache stalls.

=== modified file 'kern/misc.c'
--- kern/misc.c	2010-07-02 17:35:07 +0000
+++ kern/misc.c	2010-07-23 11:05:32 +0000
@@ -518,12 +518,39 @@ grub_strndup (const char *s, grub_size_t
 }
 
 void *
-grub_memset (void *s, int c, grub_size_t n)
+grub_memset (void *s, int c, grub_size_t len)
 {
-  unsigned char *p = (unsigned char *) s;
+  void *p = s;
+  grub_uint8_t pattern8 = c;
 
-  while (n--)
-    *p++ = (unsigned char) c;
+  if (len >= 3 * sizeof (unsigned long))
+    {
+      unsigned long patternl = 0;
+      grub_size_t i;
+
+      for (i = 0; i < sizeof (unsigned long); i++)
+	patternl |= ((unsigned long) pattern8) << (8 * i);
+
+      while (len > 0 && (((grub_addr_t) p) & (sizeof (unsigned long) - 1)))
+	{
+	  *(grub_uint8_t *) p = pattern8;
+	  p = (grub_uint8_t *) p + 1;
+	  len--;
+	}
+      while (len >= sizeof (unsigned long))
+	{
+	  *(unsigned long *) p = patternl;
+	  p = (unsigned long *) p + 1;
+	  len -= sizeof (unsigned long);
+	}
+    }
+
+  while (len > 0)
+    {
+      *(grub_uint8_t *) p = pattern8;
+      p = (grub_uint8_t *) p + 1;
+      len--;
+    }
 
   return s;
 }

-- 
Colin Watson                                       [cjwatson@ubuntu.com]