Re: uvm ncolors [was: CVS commit: src/sys/arch/x86/x86]

[Simon Burge <simonb%NetBSD.org@localhost>]

Izumi Tsutsui wrote:

> simonb%NetBSD.org@localhost wrote:
> 
> > OK to back this out and apply this one instead?  I'll probably change
> > the wording in the comment slightly.  I also had the panic in an #ifdef
> > DIAGNOSTIC, but now think that it's worth enabling all the time - the
> > lossage mode when this is wrong isn't obvious.
> 
> I think it's better to fallback to ncolors=1 with
> some warnings in case of !powerof2(ncolors) because:
> - we could assume (n_cache_index * cache_line_size) is
>   always a power of two if the CPU has a sane design
>   (unless MD cache detection code has some bug)

Can we _always_ assume that?  At first, I thought a 6MB 16-way
associative cache made more sense than a 24-way cache.  Is a 6MB
16-way cache impossible to do?

> - if the CPU has a really odd number of cache indexes,
>   our current page coloring code can't handle it properly
>   and the only sane way is disabling the optimization

This I'm also not sure about :-)  If we really do have 96 colours,
will using 32 be better than 1 or not?

As an aside, I did do some testing with ncolors = {1,2,32,64}
with build.sh and can't really see any difference anyway...

> - but no need to panic in that case otherwise we'll get
>   possible panics on each brandnew CPU unless MD cache
>   detection code like intel_cpuid_cache_info[] is always
>   up-to-date

Agreed.  My current patch doesn't panic at all.

> Maybe we need the similar check in uvm_page_init().

Yah, I'd just thought about that earlier today as well.

I've attached my current patch below.  It looks like the big
sticking point now is - do we use some manufactured value or
just 1 if we get an "odd" number of colours?

Cheers,
Simon.
--
Index: uvm_page.c
===================================================================
RCS file: /cvsroot/src/sys/uvm/uvm_page.c,v
retrieving revision 1.131
diff -d -p -u -r1.131 uvm_page.c
--- uvm_page.c  24 Mar 2008 08:53:25 -0000      1.131
+++ uvm_page.c  22 Apr 2008 13:10:08 -0000
@@ -165,6 +165,7 @@ static union {
  * local prototypes
  */
 
+static int uvm_sanitise_colours(int);
 static void uvm_pageinsert(struct vm_page *);
 static void uvm_pageinsert_after(struct vm_page *, struct vm_page *);
 static void uvm_pageremove(struct vm_page *);
@@ -174,6 +175,22 @@ static void uvm_pageremove(struct vm_pag
  */
 
 /*
+ * uvm_sanitise_colours: check that ncolors is a power-of-two
+ * and adjust if not to the largest power-of-two that divides
+ * cleanly into the original requested number of colours.
+ * Default to 1 if ncolors is 0.
+ */
+static int
+uvm_sanitise_colours(int ncolors)
+{
+
+       if (ncolors <= 0)
+               return 1;
+
+       return 1 << (ffs(ncolors) - 1);
+}
+
+/*
  * uvm_pageinsert: insert a page in the object and the hash table
  * uvm_pageinsert_after: insert a page into the specified place in listq
  *
@@ -371,6 +388,10 @@ uvm_page_init(vaddr_t *kvm_startp, vaddr
         */
        if (uvmexp.ncolors == 0)
                uvmexp.ncolors = 1;
+       /*
+        * validate the number of page colors.
+        */
+       uvmexp.ncolors = uvm_sanitise_colours(uvmexp.ncolors);
        uvmexp.colormask = uvmexp.ncolors - 1;
 
        /*
@@ -939,6 +960,11 @@ uvm_page_recolor(int newncolors)
        vsize_t bucketcount;
        int lcv, color, i, ocolors;
 
+       /*
+        * validate the new number of page colors.
+        */
+       newncolors = uvm_sanitise_colours(newncolors);
+
        if (newncolors <= uvmexp.ncolors)
                return;