Patchwork powerpc/cell: make ptcal more reliable

login
register
mail settings
Submitter stenzel@de.ibm.com
Date May 4, 2009, 7:32 p.m.
Message ID <87iqkgd5qm.fsf@de.ibm.com>
Download mbox | patch
Permalink /patch/26855/
State Superseded
Headers show

Comments

stenzel@de.ibm.com - May 4, 2009, 7:32 p.m.
This is for QS21. The following patch allocates pages only from 
the specified node, moves the ptcal area into the middle of the 
allocated page to avoid potential prefetch problems and prints 
the address of the ptcal area to facilitate diagnostics. 
    
Signed-off-by: Gerhard Stenzel <gerhard.stenzel@de.ibm.com>

===================================================================

Best regards, 

Gerhard Stenzel, Linux on Cell Development, LTC
-------------------------------------------------------------------------------------
IBM Deutschland Entwicklung GmbH
Vorsitzender des Aufsichtsrats: Martin Jetter | Geschaeftsfuehrung: Erich Baier
Sitz der Gesellschaft: Boeblingen | Registergericht: Amtsgericht Stuttgart, HRB 243294
Jeremy Kerr - May 5, 2009, 12:57 a.m.
Hi Gerhard,

> This is for QS21. The following patch allocates pages only from
> the specified node, moves the ptcal area into the middle of the
> allocated page to avoid potential prefetch problems and prints
> the address of the ptcal area to facilitate diagnostics.

You're seeing prefetches that cross a page boundary?

> -	area->pages = alloc_pages_node(area->nid, GFP_KERNEL, area->order);
> +	area->pages = alloc_pages_node(area->nid, GFP_KERNEL |
> GFP_THISNODE, area->order);

Best to keep this under 80 cols.

>
> -	if (!area->pages)
> +	if (!area->pages) {
> +		printk(KERN_INFO "%s: no page on node %d\n",
> +			__FUNCTION__, area->nid);
>  		goto out_free_area;
> +	}

That could probably be a KERN_ERR, as we don't have ptcal enabled on 
that node. Also, I believe __func__ is preferred over __FUNCTION__.

> -	addr = __pa(page_address(area->pages));
> +	/*
> +	 * We move the ptcal area to the middle of the allocated
> +	 * page, in order to avoid prefetches in memcpy and similar
> +	 * functions stepping on it.
> +	 */
> +	addr = __pa(page_address(area->pages)) + (PAGE_SIZE >> 1);

Minor nitpick, but I think (PAGE_SIZE / 2) better illustrates that 
you're putting the addr in the middle of the page. But either should be 
fine.

> +	printk(KERN_INFO "%s: enabling PTCAL on node %d address=0x%016lx
> PAGE_SIZE>>1=0x%016lx \n",
> +			__FUNCTION__, area->nid, addr, PAGE_SIZE>>1);

80 cols again. Can we do this as a pr_debug?

Cheers,

Jeremy
Gerhard Stenzel - May 5, 2009, 10:47 a.m.
Jeremy Kerr <jk@ozlabs.org> wrote on 05/05/2009 02:57:18 AM:

> Hi Gerhard,

Jeremy, thanks for your comments

>
> > This is for QS21. The following patch allocates pages only from
> > the specified node, moves the ptcal area into the middle of the
> > allocated page to avoid potential prefetch problems and prints
> > the address of the ptcal area to facilitate diagnostics.
>
> You're seeing prefetches that cross a page boundary?
>
> > -   area->pages = alloc_pages_node(area->nid, GFP_KERNEL, area->order);
> > +   area->pages = alloc_pages_node(area->nid, GFP_KERNEL |
> > GFP_THISNODE, area->order);
>
> Best to keep this under 80 cols.

ok

>
> >
> > -   if (!area->pages)
> > +   if (!area->pages) {
> > +      printk(KERN_INFO "%s: no page on node %d\n",
> > +         __FUNCTION__, area->nid);
> >        goto out_free_area;
> > +   }
>
> That could probably be a KERN_ERR, as we don't have ptcal enabled on
> that node. Also, I believe __func__ is preferred over __FUNCTION__.

Since this is the default for the kdump kernel in most cases, how about
KERN_WARNING instead.:

>
> > -   addr = __pa(page_address(area->pages));
> > +   /*
> > +    * We move the ptcal area to the middle of the allocated
> > +    * page, in order to avoid prefetches in memcpy and similar
> > +    * functions stepping on it.
> > +    */
> > +   addr = __pa(page_address(area->pages)) + (PAGE_SIZE >> 1);
>
> Minor nitpick, but I think (PAGE_SIZE / 2) better illustrates that
> you're putting the addr in the middle of the page. But either should be
> fine.

ok. I prefer to keep it like this. It should be clear from the comment.

>
> > +   printk(KERN_INFO "%s: enabling PTCAL on node %d address=0x%016lx
> > PAGE_SIZE>>1=0x%016lx \n",
> > +         __FUNCTION__, area->nid, addr, PAGE_SIZE>>1);
>
> 80 cols again. Can we do this as a pr_debug?

"PAGE_SIZE>>1" can be omitted anyway. Removing it brings it below 80 cols.
Regarding pr_debug, the message should appear only on QS21 where it can be
of interest. So, I would prefer to keep a printk.

>
> Cheers,
>
> Jeremy

Thanks again,

Best regards,

Gerhard Stenzel, Linux on Cell/Hybrid Technologies, LTC
-----------------------------------------------------------------------------------------------------------------------------------

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martin Jetter | Geschäftsführung: Erich
Baier
Sitz der Gesellschaft: Böblingen | Registergericht: Amtsgericht Stuttgart,
HRB 243294

Patch

Index: linux-2.6/arch/powerpc/platforms/cell/ras.c
===================================================================
--- linux-2.6.orig/arch/powerpc/platforms/cell/ras.c
+++ linux-2.6/arch/powerpc/platforms/cell/ras.c
@@ -122,12 +122,22 @@  static int __init cbe_ptcal_enable_on_no
 
 	area->nid = nid;
 	area->order = order;
-	area->pages = alloc_pages_node(area->nid, GFP_KERNEL, area->order);
+	area->pages = alloc_pages_node(area->nid, GFP_KERNEL | GFP_THISNODE, area->order);
 
-	if (!area->pages)
+	if (!area->pages) {
+		printk(KERN_INFO "%s: no page on node %d\n",
+			__FUNCTION__, area->nid);
 		goto out_free_area;
+	}
 
-	addr = __pa(page_address(area->pages));
+	/*
+	 * We move the ptcal area to the middle of the allocated
+	 * page, in order to avoid prefetches in memcpy and similar
+	 * functions stepping on it.
+	 */
+	addr = __pa(page_address(area->pages)) + (PAGE_SIZE >> 1);
+	printk(KERN_INFO "%s: enabling PTCAL on node %d address=0x%016lx PAGE_SIZE>>1=0x%016lx \n",
+			__FUNCTION__, area->nid, addr, PAGE_SIZE>>1);
 
 	ret = -EIO;
 	if (rtas_call(ptcal_start_tok, 3, 1, NULL, area->nid,