All of lore.kernel.org
 help / color / mirror / Atom feed
* Should we be using unlikely() around tests of GFP_ZERO?
@ 2011-01-02 23:48 ` Theodore Ts'o
  0 siblings, 0 replies; 18+ messages in thread
From: Theodore Ts'o @ 2011-01-02 23:48 UTC (permalink / raw)
  To: Christoph Lameter, Pekka Enberg, Matt Mackall, linux-mm; +Cc: linux-kernel


Given the patches being busily submitted by trivial patch submitters to
make use kmem_cache_zalloc(), et. al, I believe we should remove the
unlikely() tests around the (gfp_flags & __GFP_ZERO) tests, such as:

-	if (unlikely((flags & __GFP_ZERO) && objp))
+	if ((flags & __GFP_ZERO) && objp)
		memset(objp, 0, obj_size(cachep));

Agreed?  If so, I'll send a patch...

	    					- Ted

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Should we be using unlikely() around tests of GFP_ZERO?
@ 2011-01-02 23:48 ` Theodore Ts'o
  0 siblings, 0 replies; 18+ messages in thread
From: Theodore Ts'o @ 2011-01-02 23:48 UTC (permalink / raw)
  To: Christoph Lameter, Pekka Enberg, Matt Mackall, linux-mm; +Cc: linux-kernel


Given the patches being busily submitted by trivial patch submitters to
make use kmem_cache_zalloc(), et. al, I believe we should remove the
unlikely() tests around the (gfp_flags & __GFP_ZERO) tests, such as:

-	if (unlikely((flags & __GFP_ZERO) && objp))
+	if ((flags & __GFP_ZERO) && objp)
		memset(objp, 0, obj_size(cachep));

Agreed?  If so, I'll send a patch...

	    					- Ted

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Should we be using unlikely() around tests of GFP_ZERO?
  2011-01-02 23:48 ` Theodore Ts'o
@ 2011-01-03  3:46   ` Minchan Kim
  -1 siblings, 0 replies; 18+ messages in thread
From: Minchan Kim @ 2011-01-03  3:46 UTC (permalink / raw)
  To: Theodore Ts'o
  Cc: Christoph Lameter, Pekka Enberg, Matt Mackall, linux-mm,
	linux-kernel, Steven Rostedt

On Mon, Jan 3, 2011 at 8:48 AM, Theodore Ts'o <tytso@mit.edu> wrote:
>
> Given the patches being busily submitted by trivial patch submitters to
> make use kmem_cache_zalloc(), et. al, I believe we should remove the
> unlikely() tests around the (gfp_flags & __GFP_ZERO) tests, such as:
>
> -       if (unlikely((flags & __GFP_ZERO) && objp))
> +       if ((flags & __GFP_ZERO) && objp)
>                memset(objp, 0, obj_size(cachep));
>
> Agreed?  If so, I'll send a patch...

I support it.

Recently Steven tried to gather the information.
http://thread.gmane.org/gmane.linux.kernel/1072767
Maybe he might have a number for that.


>
>                                                - Ted
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>



-- 
Kind regards,
Minchan Kim

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Should we be using unlikely() around tests of GFP_ZERO?
@ 2011-01-03  3:46   ` Minchan Kim
  0 siblings, 0 replies; 18+ messages in thread
From: Minchan Kim @ 2011-01-03  3:46 UTC (permalink / raw)
  To: Theodore Ts'o
  Cc: Christoph Lameter, Pekka Enberg, Matt Mackall, linux-mm,
	linux-kernel, Steven Rostedt

On Mon, Jan 3, 2011 at 8:48 AM, Theodore Ts'o <tytso@mit.edu> wrote:
>
> Given the patches being busily submitted by trivial patch submitters to
> make use kmem_cache_zalloc(), et. al, I believe we should remove the
> unlikely() tests around the (gfp_flags & __GFP_ZERO) tests, such as:
>
> -       if (unlikely((flags & __GFP_ZERO) && objp))
> +       if ((flags & __GFP_ZERO) && objp)
>                memset(objp, 0, obj_size(cachep));
>
> Agreed?  If so, I'll send a patch...

I support it.

Recently Steven tried to gather the information.
http://thread.gmane.org/gmane.linux.kernel/1072767
Maybe he might have a number for that.


>
>                                                - Ted
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>



-- 
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Should we be using unlikely() around tests of GFP_ZERO?
  2011-01-03  3:46   ` Minchan Kim
@ 2011-01-03  7:40     ` Pekka Enberg
  -1 siblings, 0 replies; 18+ messages in thread
From: Pekka Enberg @ 2011-01-03  7:40 UTC (permalink / raw)
  To: Minchan Kim
  Cc: Theodore Ts'o, Christoph Lameter, Pekka Enberg, Matt Mackall,
	linux-mm, linux-kernel, Steven Rostedt, David Rientjes, npiggin

Hi,

On Mon, Jan 3, 2011 at 8:48 AM, Theodore Ts'o <tytso@mit.edu> wrote:
>> Given the patches being busily submitted by trivial patch submitters to
>> make use kmem_cache_zalloc(), et. al, I believe we should remove the
>> unlikely() tests around the (gfp_flags & __GFP_ZERO) tests, such as:
>>
>> -       if (unlikely((flags & __GFP_ZERO) && objp))
>> +       if ((flags & __GFP_ZERO) && objp)
>>                memset(objp, 0, obj_size(cachep));
>>
>> Agreed?  If so, I'll send a patch...

On Mon, Jan 3, 2011 at 5:46 AM, Minchan Kim <minchan.kim@gmail.com> wrote:
> I support it.

I guess the rationale here is that if you're going to take the hit of
memset() you can take the hit of unlikely() as well. We're optimizing
for hot call-sites that allocate a small amount of memory and
initialize everything themselves. That said, I don't think the
unlikely() annotation matters much either way and am for removing it
unless people object to that.

On Mon, Jan 3, 2011 at 5:46 AM, Minchan Kim <minchan.kim@gmail.com> wrote:
> Recently Steven tried to gather the information.
> http://thread.gmane.org/gmane.linux.kernel/1072767
> Maybe he might have a number for that.

That would be interesting, sure.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Should we be using unlikely() around tests of GFP_ZERO?
@ 2011-01-03  7:40     ` Pekka Enberg
  0 siblings, 0 replies; 18+ messages in thread
From: Pekka Enberg @ 2011-01-03  7:40 UTC (permalink / raw)
  To: Minchan Kim
  Cc: Theodore Ts'o, Christoph Lameter, Pekka Enberg, Matt Mackall,
	linux-mm, linux-kernel, Steven Rostedt, David Rientjes, npiggin

Hi,

On Mon, Jan 3, 2011 at 8:48 AM, Theodore Ts'o <tytso@mit.edu> wrote:
>> Given the patches being busily submitted by trivial patch submitters to
>> make use kmem_cache_zalloc(), et. al, I believe we should remove the
>> unlikely() tests around the (gfp_flags & __GFP_ZERO) tests, such as:
>>
>> -       if (unlikely((flags & __GFP_ZERO) && objp))
>> +       if ((flags & __GFP_ZERO) && objp)
>>                memset(objp, 0, obj_size(cachep));
>>
>> Agreed?  If so, I'll send a patch...

On Mon, Jan 3, 2011 at 5:46 AM, Minchan Kim <minchan.kim@gmail.com> wrote:
> I support it.

I guess the rationale here is that if you're going to take the hit of
memset() you can take the hit of unlikely() as well. We're optimizing
for hot call-sites that allocate a small amount of memory and
initialize everything themselves. That said, I don't think the
unlikely() annotation matters much either way and am for removing it
unless people object to that.

On Mon, Jan 3, 2011 at 5:46 AM, Minchan Kim <minchan.kim@gmail.com> wrote:
> Recently Steven tried to gather the information.
> http://thread.gmane.org/gmane.linux.kernel/1072767
> Maybe he might have a number for that.

That would be interesting, sure.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Should we be using unlikely() around tests of GFP_ZERO?
  2011-01-03  7:40     ` Pekka Enberg
@ 2011-01-03 13:45       ` Steven Rostedt
  -1 siblings, 0 replies; 18+ messages in thread
From: Steven Rostedt @ 2011-01-03 13:45 UTC (permalink / raw)
  To: Pekka Enberg
  Cc: Minchan Kim, Theodore Ts'o, Christoph Lameter, Pekka Enberg,
	Matt Mackall, linux-mm, linux-kernel, David Rientjes, npiggin

On Mon, 2011-01-03 at 09:40 +0200, Pekka Enberg wrote:
> Hi,
> 
> On Mon, Jan 3, 2011 at 8:48 AM, Theodore Ts'o <tytso@mit.edu> wrote:
> >> Given the patches being busily submitted by trivial patch submitters to
> >> make use kmem_cache_zalloc(), et. al, I believe we should remove the
> >> unlikely() tests around the (gfp_flags & __GFP_ZERO) tests, such as:
> >>
> >> -       if (unlikely((flags & __GFP_ZERO) && objp))
> >> +       if ((flags & __GFP_ZERO) && objp)
> >>                memset(objp, 0, obj_size(cachep));
> >>
> >> Agreed?  If so, I'll send a patch...
> 
> On Mon, Jan 3, 2011 at 5:46 AM, Minchan Kim <minchan.kim@gmail.com> wrote:
> > I support it.
> 
> I guess the rationale here is that if you're going to take the hit of
> memset() you can take the hit of unlikely() as well. We're optimizing
> for hot call-sites that allocate a small amount of memory and
> initialize everything themselves. That said, I don't think the
> unlikely() annotation matters much either way and am for removing it
> unless people object to that.
> 
> On Mon, Jan 3, 2011 at 5:46 AM, Minchan Kim <minchan.kim@gmail.com> wrote:
> > Recently Steven tried to gather the information.
> > http://thread.gmane.org/gmane.linux.kernel/1072767
> > Maybe he might have a number for that.
> 
> That would be interesting, sure.

Note, you could do it yourself too. Just enable:

  Kernel Hacking -> Tracers -> Branch Profiling
    (Trace likely/unlikely profiler)

   CONFIG_PROFILE_ANNOTATED_BRANCHES

Then search /debug/tracing/trace_stats/branch_annotated.

(hmm, the help in Kconfig is wrong, I need to fix that)


Anyway, here's my box. I just started it an hour ago, and have not been
doing too much on it yet. But here's what I got (using SLUB)


 correct incorrect  %        Function                  File              Line
 ------- ---------  -        --------                  ----              ----
 6890998  2784830  28        slab_alloc                slub.c            1719

That's incorrect 28% of the time.

-- Steve



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Should we be using unlikely() around tests of GFP_ZERO?
@ 2011-01-03 13:45       ` Steven Rostedt
  0 siblings, 0 replies; 18+ messages in thread
From: Steven Rostedt @ 2011-01-03 13:45 UTC (permalink / raw)
  To: Pekka Enberg
  Cc: Minchan Kim, Theodore Ts'o, Christoph Lameter, Pekka Enberg,
	Matt Mackall, linux-mm, linux-kernel, David Rientjes, npiggin

On Mon, 2011-01-03 at 09:40 +0200, Pekka Enberg wrote:
> Hi,
> 
> On Mon, Jan 3, 2011 at 8:48 AM, Theodore Ts'o <tytso@mit.edu> wrote:
> >> Given the patches being busily submitted by trivial patch submitters to
> >> make use kmem_cache_zalloc(), et. al, I believe we should remove the
> >> unlikely() tests around the (gfp_flags & __GFP_ZERO) tests, such as:
> >>
> >> -       if (unlikely((flags & __GFP_ZERO) && objp))
> >> +       if ((flags & __GFP_ZERO) && objp)
> >>                memset(objp, 0, obj_size(cachep));
> >>
> >> Agreed?  If so, I'll send a patch...
> 
> On Mon, Jan 3, 2011 at 5:46 AM, Minchan Kim <minchan.kim@gmail.com> wrote:
> > I support it.
> 
> I guess the rationale here is that if you're going to take the hit of
> memset() you can take the hit of unlikely() as well. We're optimizing
> for hot call-sites that allocate a small amount of memory and
> initialize everything themselves. That said, I don't think the
> unlikely() annotation matters much either way and am for removing it
> unless people object to that.
> 
> On Mon, Jan 3, 2011 at 5:46 AM, Minchan Kim <minchan.kim@gmail.com> wrote:
> > Recently Steven tried to gather the information.
> > http://thread.gmane.org/gmane.linux.kernel/1072767
> > Maybe he might have a number for that.
> 
> That would be interesting, sure.

Note, you could do it yourself too. Just enable:

  Kernel Hacking -> Tracers -> Branch Profiling
    (Trace likely/unlikely profiler)

   CONFIG_PROFILE_ANNOTATED_BRANCHES

Then search /debug/tracing/trace_stats/branch_annotated.

(hmm, the help in Kconfig is wrong, I need to fix that)


Anyway, here's my box. I just started it an hour ago, and have not been
doing too much on it yet. But here's what I got (using SLUB)


 correct incorrect  %        Function                  File              Line
 ------- ---------  -        --------                  ----              ----
 6890998  2784830  28        slab_alloc                slub.c            1719

That's incorrect 28% of the time.

-- Steve


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Should we be using unlikely() around tests of GFP_ZERO?
  2011-01-03  7:40     ` Pekka Enberg
@ 2011-01-03 13:58       ` Ted Ts'o
  -1 siblings, 0 replies; 18+ messages in thread
From: Ted Ts'o @ 2011-01-03 13:58 UTC (permalink / raw)
  To: Pekka Enberg
  Cc: Minchan Kim, Christoph Lameter, Pekka Enberg, Matt Mackall,
	linux-mm, linux-kernel, Steven Rostedt, David Rientjes, npiggin

On Mon, Jan 03, 2011 at 09:40:57AM +0200, Pekka Enberg wrote:
> I guess the rationale here is that if you're going to take the hit of
> memset() you can take the hit of unlikely() as well. We're optimizing
> for hot call-sites that allocate a small amount of memory and
> initialize everything themselves. That said, I don't think the
> unlikely() annotation matters much either way and am for removing it
> unless people object to that.

I suspect for many slab caches, all of the slab allocations for a
given slab cache type will have the GFP_ZERO flag passed.  So maybe it
would be more efficient to zap the entire page when it is pressed into
service for a particular slab cache, so we can avoid needing to use
memset on a per-object basis?

						- Ted

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Should we be using unlikely() around tests of GFP_ZERO?
@ 2011-01-03 13:58       ` Ted Ts'o
  0 siblings, 0 replies; 18+ messages in thread
From: Ted Ts'o @ 2011-01-03 13:58 UTC (permalink / raw)
  To: Pekka Enberg
  Cc: Minchan Kim, Christoph Lameter, Pekka Enberg, Matt Mackall,
	linux-mm, linux-kernel, Steven Rostedt, David Rientjes, npiggin

On Mon, Jan 03, 2011 at 09:40:57AM +0200, Pekka Enberg wrote:
> I guess the rationale here is that if you're going to take the hit of
> memset() you can take the hit of unlikely() as well. We're optimizing
> for hot call-sites that allocate a small amount of memory and
> initialize everything themselves. That said, I don't think the
> unlikely() annotation matters much either way and am for removing it
> unless people object to that.

I suspect for many slab caches, all of the slab allocations for a
given slab cache type will have the GFP_ZERO flag passed.  So maybe it
would be more efficient to zap the entire page when it is pressed into
service for a particular slab cache, so we can avoid needing to use
memset on a per-object basis?

						- Ted

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Should we be using unlikely() around tests of GFP_ZERO?
  2011-01-03 13:58       ` Ted Ts'o
@ 2011-01-03 14:09         ` Pekka Enberg
  -1 siblings, 0 replies; 18+ messages in thread
From: Pekka Enberg @ 2011-01-03 14:09 UTC (permalink / raw)
  To: Ted Ts'o, Pekka Enberg, Minchan Kim, Christoph Lameter,
	Pekka Enberg, Matt Mackall, linux-mm, linux-kernel,
	Steven Rostedt, David Rientjes, npiggin

Hi Ted,

On Mon, Jan 03, 2011 at 09:40:57AM +0200, Pekka Enberg wrote:
>> I guess the rationale here is that if you're going to take the hit of
>> memset() you can take the hit of unlikely() as well. We're optimizing
>> for hot call-sites that allocate a small amount of memory and
>> initialize everything themselves. That said, I don't think the
>> unlikely() annotation matters much either way and am for removing it
>> unless people object to that.

On Mon, Jan 3, 2011 at 3:58 PM, Ted Ts'o <tytso@mit.edu> wrote:
> I suspect for many slab caches, all of the slab allocations for a
> given slab cache type will have the GFP_ZERO flag passed.  So maybe it
> would be more efficient to zap the entire page when it is pressed into
> service for a particular slab cache, so we can avoid needing to use
> memset on a per-object basis?

We'd then need to do memset() in kmem_cache_free() because callers are
not required to clean up after them. In general, we don't want to do
that because object cachelines are less likely to be touched after
free than they are after allocation (you usually use the memory
immediately after you allocate).

                      Pekka

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Should we be using unlikely() around tests of GFP_ZERO?
@ 2011-01-03 14:09         ` Pekka Enberg
  0 siblings, 0 replies; 18+ messages in thread
From: Pekka Enberg @ 2011-01-03 14:09 UTC (permalink / raw)
  To: Ted Ts'o, Pekka Enberg, Minchan Kim, Christoph Lameter,
	Pekka Enberg, Matt Mackall, linux-mm, linux-kernel,
	Steven Rostedt, David Rientjes, npiggin

Hi Ted,

On Mon, Jan 03, 2011 at 09:40:57AM +0200, Pekka Enberg wrote:
>> I guess the rationale here is that if you're going to take the hit of
>> memset() you can take the hit of unlikely() as well. We're optimizing
>> for hot call-sites that allocate a small amount of memory and
>> initialize everything themselves. That said, I don't think the
>> unlikely() annotation matters much either way and am for removing it
>> unless people object to that.

On Mon, Jan 3, 2011 at 3:58 PM, Ted Ts'o <tytso@mit.edu> wrote:
> I suspect for many slab caches, all of the slab allocations for a
> given slab cache type will have the GFP_ZERO flag passed.  So maybe it
> would be more efficient to zap the entire page when it is pressed into
> service for a particular slab cache, so we can avoid needing to use
> memset on a per-object basis?

We'd then need to do memset() in kmem_cache_free() because callers are
not required to clean up after them. In general, we don't want to do
that because object cachelines are less likely to be touched after
free than they are after allocation (you usually use the memory
immediately after you allocate).

                      Pekka

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Should we be using unlikely() around tests of GFP_ZERO?
  2011-01-03 13:45       ` Steven Rostedt
@ 2011-01-03 14:10         ` Pekka Enberg
  -1 siblings, 0 replies; 18+ messages in thread
From: Pekka Enberg @ 2011-01-03 14:10 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Minchan Kim, Theodore Ts'o, Christoph Lameter, Pekka Enberg,
	Matt Mackall, linux-mm, linux-kernel, David Rientjes, npiggin

On Mon, Jan 3, 2011 at 3:45 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Mon, 2011-01-03 at 09:40 +0200, Pekka Enberg wrote:
>> Hi,
>>
>> On Mon, Jan 3, 2011 at 8:48 AM, Theodore Ts'o <tytso@mit.edu> wrote:
>> >> Given the patches being busily submitted by trivial patch submitters to
>> >> make use kmem_cache_zalloc(), et. al, I believe we should remove the
>> >> unlikely() tests around the (gfp_flags & __GFP_ZERO) tests, such as:
>> >>
>> >> -       if (unlikely((flags & __GFP_ZERO) && objp))
>> >> +       if ((flags & __GFP_ZERO) && objp)
>> >>                memset(objp, 0, obj_size(cachep));
>> >>
>> >> Agreed?  If so, I'll send a patch...
>>
>> On Mon, Jan 3, 2011 at 5:46 AM, Minchan Kim <minchan.kim@gmail.com> wrote:
>> > I support it.
>>
>> I guess the rationale here is that if you're going to take the hit of
>> memset() you can take the hit of unlikely() as well. We're optimizing
>> for hot call-sites that allocate a small amount of memory and
>> initialize everything themselves. That said, I don't think the
>> unlikely() annotation matters much either way and am for removing it
>> unless people object to that.
>>
>> On Mon, Jan 3, 2011 at 5:46 AM, Minchan Kim <minchan.kim@gmail.com> wrote:
>> > Recently Steven tried to gather the information.
>> > http://thread.gmane.org/gmane.linux.kernel/1072767
>> > Maybe he might have a number for that.
>>
>> That would be interesting, sure.
>
> Note, you could do it yourself too. Just enable:
>
>  Kernel Hacking -> Tracers -> Branch Profiling
>    (Trace likely/unlikely profiler)
>
>   CONFIG_PROFILE_ANNOTATED_BRANCHES
>
> Then search /debug/tracing/trace_stats/branch_annotated.
>
> (hmm, the help in Kconfig is wrong, I need to fix that)
>
>
> Anyway, here's my box. I just started it an hour ago, and have not been
> doing too much on it yet. But here's what I got (using SLUB)
>
>
>  correct incorrect  %        Function                  File              Line
>  ------- ---------  -        --------                  ----              ----
>  6890998  2784830  28        slab_alloc                slub.c            1719
>
> That's incorrect 28% of the time.

Thanks! AFAICT, that number is high enough to justify removing the
unlikely() annotations, no?

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Should we be using unlikely() around tests of GFP_ZERO?
@ 2011-01-03 14:10         ` Pekka Enberg
  0 siblings, 0 replies; 18+ messages in thread
From: Pekka Enberg @ 2011-01-03 14:10 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Minchan Kim, Theodore Ts'o, Christoph Lameter, Pekka Enberg,
	Matt Mackall, linux-mm, linux-kernel, David Rientjes, npiggin

On Mon, Jan 3, 2011 at 3:45 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Mon, 2011-01-03 at 09:40 +0200, Pekka Enberg wrote:
>> Hi,
>>
>> On Mon, Jan 3, 2011 at 8:48 AM, Theodore Ts'o <tytso@mit.edu> wrote:
>> >> Given the patches being busily submitted by trivial patch submitters to
>> >> make use kmem_cache_zalloc(), et. al, I believe we should remove the
>> >> unlikely() tests around the (gfp_flags & __GFP_ZERO) tests, such as:
>> >>
>> >> -       if (unlikely((flags & __GFP_ZERO) && objp))
>> >> +       if ((flags & __GFP_ZERO) && objp)
>> >>                memset(objp, 0, obj_size(cachep));
>> >>
>> >> Agreed?  If so, I'll send a patch...
>>
>> On Mon, Jan 3, 2011 at 5:46 AM, Minchan Kim <minchan.kim@gmail.com> wrote:
>> > I support it.
>>
>> I guess the rationale here is that if you're going to take the hit of
>> memset() you can take the hit of unlikely() as well. We're optimizing
>> for hot call-sites that allocate a small amount of memory and
>> initialize everything themselves. That said, I don't think the
>> unlikely() annotation matters much either way and am for removing it
>> unless people object to that.
>>
>> On Mon, Jan 3, 2011 at 5:46 AM, Minchan Kim <minchan.kim@gmail.com> wrote:
>> > Recently Steven tried to gather the information.
>> > http://thread.gmane.org/gmane.linux.kernel/1072767
>> > Maybe he might have a number for that.
>>
>> That would be interesting, sure.
>
> Note, you could do it yourself too. Just enable:
>
>  Kernel Hacking -> Tracers -> Branch Profiling
>    (Trace likely/unlikely profiler)
>
>   CONFIG_PROFILE_ANNOTATED_BRANCHES
>
> Then search /debug/tracing/trace_stats/branch_annotated.
>
> (hmm, the help in Kconfig is wrong, I need to fix that)
>
>
> Anyway, here's my box. I just started it an hour ago, and have not been
> doing too much on it yet. But here's what I got (using SLUB)
>
>
>  correct incorrect  %        Function                  File              Line
>  ------- ---------  -        --------                  ----              ----
>  6890998  2784830  28        slab_alloc                slub.c            1719
>
> That's incorrect 28% of the time.

Thanks! AFAICT, that number is high enough to justify removing the
unlikely() annotations, no?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Should we be using unlikely() around tests of GFP_ZERO?
  2011-01-03 14:10         ` Pekka Enberg
@ 2011-01-03 14:26           ` Steven Rostedt
  -1 siblings, 0 replies; 18+ messages in thread
From: Steven Rostedt @ 2011-01-03 14:26 UTC (permalink / raw)
  To: Pekka Enberg
  Cc: Minchan Kim, Theodore Ts'o, Christoph Lameter, Pekka Enberg,
	Matt Mackall, linux-mm, linux-kernel, David Rientjes, npiggin

On Mon, 2011-01-03 at 16:10 +0200, Pekka Enberg wrote:

> >  correct incorrect  %        Function                  File              Line
> >  ------- ---------  -        --------                  ----              ----
> >  6890998  2784830  28        slab_alloc                slub.c            1719
> >
> > That's incorrect 28% of the time.
> 
> Thanks! AFAICT, that number is high enough to justify removing the
> unlikely() annotations, no?

Personally, I think anything that is incorrect more that 5% of the time
should not have any annotation.

My rule is to use the annotation when a branch goes one way 95% or more.
With the exception of times when we want a particular path to be the
faster path, because we know its in a more critical position (as there
are cases in the scheduler and the tracing infrastructure itself).

But here, I think removing it is the right decision.

-- Steve



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Should we be using unlikely() around tests of GFP_ZERO?
@ 2011-01-03 14:26           ` Steven Rostedt
  0 siblings, 0 replies; 18+ messages in thread
From: Steven Rostedt @ 2011-01-03 14:26 UTC (permalink / raw)
  To: Pekka Enberg
  Cc: Minchan Kim, Theodore Ts'o, Christoph Lameter, Pekka Enberg,
	Matt Mackall, linux-mm, linux-kernel, David Rientjes, npiggin

On Mon, 2011-01-03 at 16:10 +0200, Pekka Enberg wrote:

> >  correct incorrect  %        Function                  File              Line
> >  ------- ---------  -        --------                  ----              ----
> >  6890998  2784830  28        slab_alloc                slub.c            1719
> >
> > That's incorrect 28% of the time.
> 
> Thanks! AFAICT, that number is high enough to justify removing the
> unlikely() annotations, no?

Personally, I think anything that is incorrect more that 5% of the time
should not have any annotation.

My rule is to use the annotation when a branch goes one way 95% or more.
With the exception of times when we want a particular path to be the
faster path, because we know its in a more critical position (as there
are cases in the scheduler and the tracing infrastructure itself).

But here, I think removing it is the right decision.

-- Steve


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Should we be using unlikely() around tests of GFP_ZERO?
  2011-01-02 23:48 ` Theodore Ts'o
@ 2011-01-03 17:23   ` Matt Mackall
  -1 siblings, 0 replies; 18+ messages in thread
From: Matt Mackall @ 2011-01-03 17:23 UTC (permalink / raw)
  To: Theodore Ts'o; +Cc: Christoph Lameter, Pekka Enberg, linux-mm, linux-kernel

On Sun, 2011-01-02 at 18:48 -0500, Theodore Ts'o wrote:
> Given the patches being busily submitted by trivial patch submitters to
> make use kmem_cache_zalloc(), et. al, I believe we should remove the
> unlikely() tests around the (gfp_flags & __GFP_ZERO) tests, such as:
> 
> -	if (unlikely((flags & __GFP_ZERO) && objp))
> +	if ((flags & __GFP_ZERO) && objp)
> 		memset(objp, 0, obj_size(cachep));
> 
> Agreed?  If so, I'll send a patch...

Sounds good to me.

We might consider dropping this flag and making the decision statically
(ie alloc vs zalloc), at least for slab objects.

-- 
Mathematics is the supreme nostalgia of our time.



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Should we be using unlikely() around tests of GFP_ZERO?
@ 2011-01-03 17:23   ` Matt Mackall
  0 siblings, 0 replies; 18+ messages in thread
From: Matt Mackall @ 2011-01-03 17:23 UTC (permalink / raw)
  To: Theodore Ts'o; +Cc: Christoph Lameter, Pekka Enberg, linux-mm, linux-kernel

On Sun, 2011-01-02 at 18:48 -0500, Theodore Ts'o wrote:
> Given the patches being busily submitted by trivial patch submitters to
> make use kmem_cache_zalloc(), et. al, I believe we should remove the
> unlikely() tests around the (gfp_flags & __GFP_ZERO) tests, such as:
> 
> -	if (unlikely((flags & __GFP_ZERO) && objp))
> +	if ((flags & __GFP_ZERO) && objp)
> 		memset(objp, 0, obj_size(cachep));
> 
> Agreed?  If so, I'll send a patch...

Sounds good to me.

We might consider dropping this flag and making the decision statically
(ie alloc vs zalloc), at least for slab objects.

-- 
Mathematics is the supreme nostalgia of our time.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2011-01-03 17:23 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-01-02 23:48 Should we be using unlikely() around tests of GFP_ZERO? Theodore Ts'o
2011-01-02 23:48 ` Theodore Ts'o
2011-01-03  3:46 ` Minchan Kim
2011-01-03  3:46   ` Minchan Kim
2011-01-03  7:40   ` Pekka Enberg
2011-01-03  7:40     ` Pekka Enberg
2011-01-03 13:45     ` Steven Rostedt
2011-01-03 13:45       ` Steven Rostedt
2011-01-03 14:10       ` Pekka Enberg
2011-01-03 14:10         ` Pekka Enberg
2011-01-03 14:26         ` Steven Rostedt
2011-01-03 14:26           ` Steven Rostedt
2011-01-03 13:58     ` Ted Ts'o
2011-01-03 13:58       ` Ted Ts'o
2011-01-03 14:09       ` Pekka Enberg
2011-01-03 14:09         ` Pekka Enberg
2011-01-03 17:23 ` Matt Mackall
2011-01-03 17:23   ` Matt Mackall

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.