[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20070523132552.GA10067@sergelap.austin.ibm.com>
Date: Wed, 23 May 2007 08:25:52 -0500
From: "Serge E. Hallyn" <serue@...ibm.com>
To: Paul Dickson <paul@...manentmail.com>
Cc: Badari Pulavarty <pbadari@...ibm.com>, bharata@...ux.vnet.ibm.com,
lkml <linux-kernel@...r.kernel.org>,
linux-fsdevel <linux-fsdevel@...r.kernel.org>,
Jan Blunck <j.blunck@...harburg.de>
Subject: Re: [RFC][PATCH 5/14] Introduce union stack
Quoting Paul Dickson (paul@...manentmail.com):
> On Mon, 14 May 2007 13:23:06 -0700, Badari Pulavarty wrote:
>
> > > + while (fs) {
> > > + locked = union_trylock(fs->root);
> > > + if (!locked)
> > > + goto loop1;
> > > + locked = union_trylock(fs->altroot);
> > > + if (!locked)
> > > + goto loop2;
> > > + locked = union_trylock(fs->pwd);
> > > + if (!locked)
> > > + goto loop3;
> > > + break;
> > > + loop3:
> > > + union_unlock(fs->altroot);
> > > + loop2:
> > > + union_unlock(fs->root);
> > > + loop1:
> > > + read_unlock(&fs->lock);
> > > + UM_DEBUG_LOCK("Failed to get all semaphores in fs_struct!\n");
> > > + cpu_relax();
> > > + read_lock(&fs->lock);
> > > + continue;
> >
> > Nit.. why "continue" ?
> >
> > > + }
> > > + BUG_ON(!fs);
>
> How about getting rid of the gotos:
>
> while (fs) {
> locked = union_trylock(fs->root);
> if (locked) {
> locked = union_trylock(fs->altroot);
> if (locked) {
> locked = union_trylock(fs->pwd);
> if (locked)
> break;
> else {
> union_unlock(fs->altroot);
> union_unlock(fs->root);
> }
> else
> union_unlock(fs->root);
> }
> }
> read_unlock(&fs->lock);
> UM_DEBUG_LOCK("Failed to get all semaphores in fs_struct!\n");
> cpu_relax();
> read_lock(&fs->lock);
> }
> BUG_ON(!fs);
>
> It's the same number of lines. Shorter if you get rid of the "locked"
> variable.
I dunno, I thought the goto versoin was cleaner and easier to tell that
the right locks are getting unlocked. The worst part in the second
version is the break in the middle!
-serge
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists