Please see: e6022603b9aa7d61d20b392e69edcdbbc1789969
Having a look at the LKML archives this was raised back in 2006:
http://lkml.org/lkml/2006/6/23/337
I'm not interested in whether unlikely() actually helps here.
I'm still missing _why_ rsv is mostly NULL at this callsite, as Andrew
asserted here:
http://lkml.org/lkml/2006/6/23/400
And then Steve here: http://lkml.org/lkml/2006/6/24/76
Where he said:
"The problem is that in these cases the pointer is NULL several thousands
of times for every time it is not NULL (if ever). The non-NULL case is
where an error occurred or something very special. So I don't see how
the if here is a problem?"
I'm missing which error or what "something very special" is the
unlikely() reason for having rsv be NULL.
Looking at the code; ext3_clear_inode() is _the_ place where the
i_block_alloc_info is cleaned up. In my testing the rsv is _never_
NULL if the file was open for writing. Are we saying that reads are
much more common than writes? May be a reasonable assumption but
saying as much is very different than what Steve seemed to be eluding
to...
Anyway, I'd appreciate some clarification here.
thanks,
Mike
On Mon, 27 Oct 2008, Mike Snitzer wrote:
> Please see: e6022603b9aa7d61d20b392e69edcdbbc1789969
>
> Having a look at the LKML archives this was raised back in 2006:
> http://lkml.org/lkml/2006/6/23/337
>
> I'm not interested in whether unlikely() actually helps here.
>
> I'm still missing _why_ rsv is mostly NULL at this callsite, as Andrew
> asserted here:
> http://lkml.org/lkml/2006/6/23/400
>
> And then Steve here: http://lkml.org/lkml/2006/6/24/76
> Where he said:
> "The problem is that in these cases the pointer is NULL several thousands
> of times for every time it is not NULL (if ever). The non-NULL case is
> where an error occurred or something very special. So I don't see how
> the if here is a problem?"
>
> I'm missing which error or what "something very special" is the
> unlikely() reason for having rsv be NULL.
>
> Looking at the code; ext3_clear_inode() is _the_ place where the
> i_block_alloc_info is cleaned up. In my testing the rsv is _never_
> NULL if the file was open for writing. Are we saying that reads are
> much more common than writes? May be a reasonable assumption but
> saying as much is very different than what Steve seemed to be eluding
> to...
>
> Anyway, I'd appreciate some clarification here.
I barely remember this. But I do remember it none-the-less ;-)
At the time, my tracing showed that the value was always NULL. But
perhaps, I was only doing reads :-/
I'll make a little ftrace test and record the number of times it is NULL
compared to the number of times that it is not. I'll have an answer
shortly. Perhaps we can nuke that condition.
-- Steve
On Mon, 27 Oct 2008, Mike Snitzer wrote:
> Please see: e6022603b9aa7d61d20b392e69edcdbbc1789969
>
> Having a look at the LKML archives this was raised back in 2006:
> http://lkml.org/lkml/2006/6/23/337
>
> I'm not interested in whether unlikely() actually helps here.
>
> I'm still missing _why_ rsv is mostly NULL at this callsite, as Andrew
> asserted here:
> http://lkml.org/lkml/2006/6/23/400
>
> And then Steve here: http://lkml.org/lkml/2006/6/24/76
> Where he said:
> "The problem is that in these cases the pointer is NULL several thousands
> of times for every time it is not NULL (if ever). The non-NULL case is
> where an error occurred or something very special. So I don't see how
> the if here is a problem?"
>
> I'm missing which error or what "something very special" is the
> unlikely() reason for having rsv be NULL.
>
> Looking at the code; ext3_clear_inode() is _the_ place where the
> i_block_alloc_info is cleaned up. In my testing the rsv is _never_
> NULL if the file was open for writing. Are we saying that reads are
> much more common than writes? May be a reasonable assumption but
> saying as much is very different than what Steve seemed to be eluding
> to...
>
> Anyway, I'd appreciate some clarification here.
Attached is a patch that I used for counting.
Here's my results:
# cat /debug/tracing/ftrace_null
45
# cat /debug/tracing/ftrace_nonnull
7
Ah, seems that there is cases where it is nonnull more often. Anyway, it
obviously is not a fast path (total of 52). Even if it was all null, it is
not big enough to call for the confusion.
I'd suggest removing the if conditional, and just calling kfree.
-- Steve
On Mon, 27 Oct 2008 19:32:11 -0400 (EDT)
Steven Rostedt <[email protected]> wrote:
> Attached is a patch that I used for counting.
If we could get something like that into mainline then I can
stop maintaining profile-likely-unlikely-macros.patch, which would
be welcome.
在 2008-10-27一的 18:29 -0400，Mike Snitzer写道：
> Please see: e6022603b9aa7d61d20b392e69edcdbbc1789969
>
> Having a look at the LKML archives this was raised back in 2006:
> http://lkml.org/lkml/2006/6/23/337
>
> I'm not interested in whether unlikely() actually helps here.
>
> I'm still missing _why_ rsv is mostly NULL at this callsite, as Andrew
> asserted here:
> http://lkml.org/lkml/2006/6/23/400
>
> And then Steve here: http://lkml.org/lkml/2006/6/24/76
> Where he said:
> "The problem is that in these cases the pointer is NULL several thousands
> of times for every time it is not NULL (if ever). The non-NULL case is
> where an error occurred or something very special. So I don't see how
> the if here is a problem?"
>
> I'm missing which error or what "something very special" is the
> unlikely() reason for having rsv be NULL.
>
> Looking at the code; ext3_clear_inode() is _the_ place where the
> i_block_alloc_info is cleaned up. In my testing the rsv is _never_
> NULL if the file was open for writing. Are we saying that reads are
> much more common than writes? May be a reasonable assumption but
> saying as much is very different than what Steve seemed to be eluding
> to...
>
i_block_alloc_info as the structure to keep track of block
reservation/allocation, is dynamically allocated when file does need
blocks. So rsv remains NULL even if file is open for rewrite, until
file is about to do block allocation.
Mingming
> Anyway, I'd appreciate some clarification here.
>
> thanks,
> Mike
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 27, 2008 at 7:52 PM, Mingming Cao <[email protected]> wrote:
>
> $B:_(B 2008-10-27$B0lE*(B 18:29 -0400$B!$(BMike Snitzer$B<LF;!'(B
>> Please see: e6022603b9aa7d61d20b392e69edcdbbc1789969
>>
>> Having a look at the LKML archives this was raised back in 2006:
>> http://lkml.org/lkml/2006/6/23/337
>>
>> I'm not interested in whether unlikely() actually helps here.
>>
>> I'm still missing _why_ rsv is mostly NULL at this callsite, as Andrew
>> asserted here:
>> http://lkml.org/lkml/2006/6/23/400
>>
>> And then Steve here: http://lkml.org/lkml/2006/6/24/76
>> Where he said:
>> "The problem is that in these cases the pointer is NULL several thousands
>> of times for every time it is not NULL (if ever). The non-NULL case is
>> where an error occurred or something very special. So I don't see how
>> the if here is a problem?"
>>
>> I'm missing which error or what "something very special" is the
>> unlikely() reason for having rsv be NULL.
>>
>> Looking at the code; ext3_clear_inode() is _the_ place where the
>> i_block_alloc_info is cleaned up. In my testing the rsv is _never_
>> NULL if the file was open for writing. Are we saying that reads are
>> much more common than writes? May be a reasonable assumption but
>> saying as much is very different than what Steve seemed to be eluding
>> to...
>>
>
> i_block_alloc_info as the structure to keep track of block
> reservation/allocation, is dynamically allocated when file does need
> blocks. So rsv remains NULL even if file is open for rewrite, until
> file is about to do block allocation.
Yes, i_block_alloc_info is only allocated when a block must be allocated...
I over simplified this by making the distinction of the file open for
writing. My intent was to point out that allocating blocks for writes
isn't that uncommon.
I was mainly just looking for clarification on i_block_alloc_info's
life-cycle... based on Steve's comment from 2006 I thought I might be
missing something. It doesn't really look like I was.
regards,
Mike
On Mon, Oct 27, 2008 at 07:32:11PM -0400, Steven Rostedt wrote:
> Attached is a patch that I used for counting.
>
> Here's my results:
> # cat /debug/tracing/ftrace_null
> 45
> # cat /debug/tracing/ftrace_nonnull
> 7
>
> Ah, seems that there is cases where it is nonnull more often. Anyway, it
> obviously is not a fast path (total of 52). Even if it was all null, it is
> not big enough to call for the confusion.
Silly question --- what was your test workload?
- Ted
On Mon, Oct 27, 2008 at 7:32 PM, Steven Rostedt <[email protected]> wrote:
>
> On Mon, 27 Oct 2008, Mike Snitzer wrote:
>
>> Please see: e6022603b9aa7d61d20b392e69edcdbbc1789969
>>
>> Having a look at the LKML archives this was raised back in 2006:
>> http://lkml.org/lkml/2006/6/23/337
>>
>> I'm not interested in whether unlikely() actually helps here.
>>
>> I'm still missing _why_ rsv is mostly NULL at this callsite, as Andrew
>> asserted here:
>> http://lkml.org/lkml/2006/6/23/400
>>
>> And then Steve here: http://lkml.org/lkml/2006/6/24/76
>> Where he said:
>> "The problem is that in these cases the pointer is NULL several thousands
>> of times for every time it is not NULL (if ever). The non-NULL case is
>> where an error occurred or something very special. So I don't see how
>> the if here is a problem?"
>>
>> I'm missing which error or what "something very special" is the
>> unlikely() reason for having rsv be NULL.
>>
>> Looking at the code; ext3_clear_inode() is _the_ place where the
>> i_block_alloc_info is cleaned up. In my testing the rsv is _never_
>> NULL if the file was open for writing. Are we saying that reads are
>> much more common than writes? May be a reasonable assumption but
>> saying as much is very different than what Steve seemed to be eluding
>> to...
>>
>> Anyway, I'd appreciate some clarification here.
>
> Attached is a patch that I used for counting.
>
> Here's my results:
> # cat /debug/tracing/ftrace_null
> 45
> # cat /debug/tracing/ftrace_nonnull
> 7
>
> Ah, seems that there is cases where it is nonnull more often. Anyway, it
> obviously is not a fast path (total of 52). Even if it was all null, it is
> not big enough to call for the confusion.
What was your workload that resulted in this break-down? AFAIK you'd
have 100% in ftrace_nonnull if you simply opened new files and wrote
to them.
> I'd suggest removing the if conditional, and just calling kfree.
Yes, probably.
thanks,
Mike
On Mon, 27 Oct 2008, Theodore Tso wrote:
> On Mon, Oct 27, 2008 at 07:32:11PM -0400, Steven Rostedt wrote:
> > Attached is a patch that I used for counting.
> >
> > Here's my results:
> > # cat /debug/tracing/ftrace_null
> > 45
> > # cat /debug/tracing/ftrace_nonnull
> > 7
> >
> > Ah, seems that there is cases where it is nonnull more often. Anyway, it
> > obviously is not a fast path (total of 52). Even if it was all null, it is
> > not big enough to call for the confusion.
>
> Silly question --- what was your test workload?
Hehe, I just booted the box. The counting started right away, so I just
looked at the work load.
Anyway, I'm writing a generic "unlikely" profiler that should make Andrew
happy. And this will also catch this case as well.
-- Steve