kjournald wasting CPU in invert_lock fs/jbd/commit.c

kjournald wasting CPU in invert_lock fs/jbd/commit.c

[Steven Rostedt]

I noticed that the code in commit.c of the jbd system can waste CPU
cycles. The offending code is as follows.

static int inverted_lock(journal_t *journal, struct buffer_head *bh)
{
        if (!jbd_trylock_bh_state(bh)) {
                spin_unlock(&journal->j_list_lock);
                schedule();
                return 0;
        }
        return 1;
}

[...]

void journal_commit_transaction(journal_t *journal)
{

[...]

write_out_data:
        cond_resched();
        spin_lock(&journal->j_list_lock);

        while (commit_transaction->t_sync_datalist) {
                struct buffer_head *bh;

                jh = commit_transaction->t_sync_datalist;
                commit_transaction->t_sync_datalist = jh->b_tnext;
                bh = jh2bh(jh);
                if (buffer_locked(bh)) {
                        BUFFER_TRACE(bh, "locked");
                        if (!inverted_lock(journal, bh))
                                goto write_out_data;


This code makes a loop if the jbd_trylock_bh_state fails. This code will
wait till whoever owns the lock releases it. But it is really in a busy
loop and will only be interrupted when the kjournald uses up all its
quota.  So it's basically just wasting CPU cycles here.  The following
patch should fix this.

Signed-off-by: Steven Rostedt rostedt@goodmis.org
---
--- a/fs/jbd/commit.c	2005-07-11 17:51:37.000000000 -0400
+++ b/fs/jbd/commit.c	2005-07-11 17:51:58.000000000 -0400
@@ -87,7 +87,7 @@ static int inverted_lock(journal_t *jour
 {
 	if (!jbd_trylock_bh_state(bh)) {
 		spin_unlock(&journal->j_list_lock);
-		schedule();
+		yield();
 		return 0;
 	}
 	return 1;

Re: kjournald wasting CPU in invert_lock fs/jbd/commit.c

[Andrew Morton]

Steven Rostedt <rostedt@goodmis.org> wrote:
>
> I noticed that the code in commit.c of the jbd system can waste CPU
> cycles.

How did you notice?  By code inspection or by runtime observation?  If the
latter, please share.

> The offending code is as follows.
> 
> static int inverted_lock(journal_t *journal, struct buffer_head *bh)
> {
>         if (!jbd_trylock_bh_state(bh)) {
>                 spin_unlock(&journal->j_list_lock);
>                 schedule();
>                 return 0;
>         }
>         return 1;
> }

"offending" is a good description.  That code sucks.  But it sits on the
edge between two subsystems which really really want to take those locks in
opposite order.


> This code makes a loop if the jbd_trylock_bh_state fails. This code will
> wait till whoever owns the lock releases it. But it is really in a busy
> loop and will only be interrupted when the kjournald uses up all its
> quota.  So it's basically just wasting CPU cycles here.

Yeah.  But these _are_ spinlocks, so spinning is what's supposed to happen.
 Maybe we should dump that silly schedule() and just do cpu_relax(). 
Although I do recall once theorising that the time we spend in the
schedule() might be preventing livelocks.

>  The following
> patch should fix this.
> 
> Signed-off-by: Steven Rostedt rostedt@goodmis.org

Please put "<>" around the email address.

> ---
> --- a/fs/jbd/commit.c	2005-07-11 17:51:37.000000000 -0400
> +++ b/fs/jbd/commit.c	2005-07-11 17:51:58.000000000 -0400
> @@ -87,7 +87,7 @@ static int inverted_lock(journal_t *jour
>  {
>  	if (!jbd_trylock_bh_state(bh)) {
>  		spin_unlock(&journal->j_list_lock);
> -		schedule();
> +		yield();
>  		return 0;
>  	}
>  	return 1;

Nope, yield() can cause terribly long delays when other tasks are cpu-bound.

Re: kjournald wasting CPU in invert_lock fs/jbd/commit.c

[Steven Rostedt]

On Mon, 2005-07-11 at 15:41 -0700, Andrew Morton wrote:
> Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > I noticed that the code in commit.c of the jbd system can waste CPU
> > cycles.
> 
> How did you notice?  By code inspection or by runtime observation?  If the
> latter, please share.

Argh! I just realize that this problem is really more in Ingo's RT
kernel, but I assumed that it was a problem in vanilla since the code is
more from the vanilla kernel.  With Ingo's spin_locks as mutexes, this
creates a problem on UP, but your are right, this is not a problem for
vanilla UP.


> Yeah.  But these _are_ spinlocks, so spinning is what's supposed to happen.
>  Maybe we should dump that silly schedule() and just do cpu_relax(). 
> Although I do recall once theorising that the time we spend in the
> schedule() might be preventing livelocks.
> 

As mentioned above, this was a confusion of paradigms. I just got back
from Europe, so I'm blaming this on jetlag!  

OK a cpu_relax() may be better. So here it is :-)

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
--- a/fs/jbd/commit.c	2005-07-11 17:51:37.000000000 -0400
+++ b/fs/jbd/commit.c	2005-07-11 19:05:35.000000000 -0400
@@ -87,7 +87,7 @@ static int inverted_lock(journal_t *jour
 {
 	if (!jbd_trylock_bh_state(bh)) {
 		spin_unlock(&journal->j_list_lock);
-		schedule();
+		cpu_relax();
 		return 0;
 	}
 	return 1;