mm: slab - __cache_alloc NULL prefetch fix

mm: slab - __cache_alloc NULL prefetch fix

[Cyrill Gorcunov]

Do not prefetch NULL

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---

Please check

 mm/slab.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Index: linux-2.6.git/mm/slab.c
===================================================================
--- linux-2.6.git.orig/mm/slab.c
+++ linux-2.6.git/mm/slab.c
@@ -3395,10 +3395,11 @@ __cache_alloc(struct kmem_cache *cachep,
 	objp = __do_cache_alloc(cachep, flags);
 	local_irq_restore(save_flags);
 	objp = cache_alloc_debugcheck_after(cachep, flags, objp, caller);
-	prefetchw(objp);
 
-	if (likely(objp))
+	if (likely(objp)) {
+		prefetchw(objp);
 		kmemcheck_slab_alloc(cachep, flags, objp, obj_size(cachep));
+	}
 
 	if (unlikely((flags & __GFP_ZERO) && objp))
 		memset(objp, 0, obj_size(cachep));

Re: mm: slab - __cache_alloc NULL prefetch fix

[Cyrill Gorcunov]

[Cyrill Gorcunov - Thu, Nov 20, 2008 at 07:44:00PM +0300]
| Do not prefetch NULL
| 
| Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
| ---
| 
| Please check
|
...

even having prefetch(NULL) as valid we do check if
object is not NULL anyway so I thought there is no
need to prefetch(NULL) in case of fail.
 
		- Cyrill -

Re: mm: slab - __cache_alloc NULL prefetch fix

[Christoph Lameter]

On Thu, 20 Nov 2008, Cyrill Gorcunov wrote:

> Do not prefetch NULL

Why not? Does it matter?

Re: mm: slab - __cache_alloc NULL prefetch fix

[Christoph Lameter]

On Thu, 20 Nov 2008, Cyrill Gorcunov wrote:

> even having prefetch(NULL) as valid we do check if
> object is not NULL anyway so I thought there is no
> need to prefetch(NULL) in case of fail.

The object is only checked if SLAB_DEBUG is on.

Re: mm: slab - __cache_alloc NULL prefetch fix

[Cyrill Gorcunov]

[Christoph Lameter - Thu, Nov 20, 2008 at 12:16:28PM -0600]
| On Thu, 20 Nov 2008, Cyrill Gorcunov wrote:
| 
| > even having prefetch(NULL) as valid we do check if
| > object is not NULL anyway so I thought there is no
| > need to prefetch(NULL) in case of fail.
| 
| The object is only checked if SLAB_DEBUG is on.
| 

My fault Christoph, I've been messed by kmemcheck_slab_alloc
which in turn actually becomes zero function if not tuned on.
So -- all is fine, sorry for noise.

		- Cyrill -

Re: mm: slab - __cache_alloc NULL prefetch fix

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 473 bytes --]

On Thu, 20 Nov 2008 19:44:00 +0300, Cyrill Gorcunov said:

> -	prefetchw(objp);
>  
> -	if (likely(objp))
> +	if (likely(objp)) {
> +		prefetchw(objp);
>  		kmemcheck_slab_alloc(cachep, flags, objp, obj_size(cachep));
> +	}

Although it probably makes sense to not bother prefetching NULL, I also
need to wonder how useful it is to prefetch something that we then
turn around and dereference in the very next line of code.

Maybe we should just lose the prefetch entirely?

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: mm: slab - __cache_alloc NULL prefetch fix

[Pekka Enberg]

Valdis.Kletnieks@vt.edu wrote:
> On Thu, 20 Nov 2008 19:44:00 +0300, Cyrill Gorcunov said:
> 
>> -	prefetchw(objp);
>>  
>> -	if (likely(objp))
>> +	if (likely(objp)) {
>> +		prefetchw(objp);
>>  		kmemcheck_slab_alloc(cachep, flags, objp, obj_size(cachep));
>> +	}
> 
> Although it probably makes sense to not bother prefetching NULL, I also
> need to wonder how useful it is to prefetch something that we then
> turn around and dereference in the very next line of code.

Note that kmemcheck_slab_alloc() is going to be a no-op on anything but 
a special developer or tester debug config.

Re: mm: slab - __cache_alloc NULL prefetch fix

[Cyrill Gorcunov]

[Valdis.Kletnieks@vt.edu - Thu, Nov 20, 2008 at 02:58:28PM -0500]
| On Thu, 20 Nov 2008 19:44:00 +0300, Cyrill Gorcunov said:
| 
| > -	prefetchw(objp);
| >  
| > -	if (likely(objp))
| > +	if (likely(objp)) {
| > +		prefetchw(objp);
| >  		kmemcheck_slab_alloc(cachep, flags, objp, obj_size(cachep));
| > +	}
| 
| Although it probably makes sense to not bother prefetching NULL, I also
| need to wonder how useful it is to prefetch something that we then
| turn around and dereference in the very next line of code.
| 
| Maybe we should just lose the prefetch entirely?

In case of obj == NULL it would not be a really penalty.
I heard there was a problem passing NULL to prefetch on
some PPC machines but PPC code already well protected
against it. So in most cases obj would not be null and
hint cpu that we will use this obj soon is good choise
I think. In case of kmemcheck it's true if only kmemcheck
was turned on, otherwise this if(obj) with empty kmemcheck_slab_alloc
body will be eliminated by compiler but in case of having there
(inside of 'if' block) prefetchw(objp) I'm not sure what gcc
will decide :-) Anyway -- it was a false alram from me since
having prefetch in original place is not problem. And maybe
modern cpus even have a special trap for NULL prefetch
target and eliminate it out on decoding stage without penalty
(just a guess).

		- Cyrill -