Date: Wed, 26 Dec 2007 14:28:22 -0500
From: "Ed L. Cashin" <ecashin@coraid.com>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-kernel@vger.kernel.org
Subject: Re: [PATCH 09/13] remove race between use and initialization of
	locks
Message-ID: <20071226192822.GG9411@coraid.com>
References: <1198188961-25147-1-git-send-email-ecashin@coraid.com> <1198188961-25147-9-git-send-email-ecashin@coraid.com> <20071221220040.5862b43b.akpm@linux-foundation.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20071221220040.5862b43b.akpm@linux-foundation.org>
User-Agent: Mutt/1.5.16 (2007-06-11)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2225
Lines: 55

On Fri, Dec 21, 2007 at 10:00:40PM -0800, Andrew Morton wrote:
> On Thu, 20 Dec 2007 17:15:57 -0500 "Ed L. Cashin" <ecashin@coraid.com> wrote:
...
> > +static __DECLARE_SEMAPHORE_GENERIC(emsgs_sema, 0);
...
> > -	sema_init(&emsgs_sema, 0);
> > -	spin_lock_init(&emsgs_lock);
> >  	aoe_class = class_create(THIS_MODULE, "aoe");
> >  	if (IS_ERR(aoe_class)) {
> >  		unregister_chrdev(AOE_MAJOR, "aoechr");
> 
> I think it would be better to go back to initialising emsgs_lock at runtime
> rather than fattening the exported semaphore API like this.

I don't think there is anything wrong with having a complete set of
initialization routines for a semaphore, but it's certainly easy
enough to go back to Alexey Dobriyan's original suggestion, which was
to simply move the initialization calls before register_chardev.

I will follow this email with a patch that does that.

> emssgs_sema is a weird-looking thing.  There really should be some comments
> in there because it is unobvious what the code is attempting to do.
> 
> What is the code attempting to do?

There is a ring buffer of error messages.  Userland processes can read
these error messages by reading /dev/etherd/err, blocking if there are
no error messages to read yet.

The emsgs_sema semaphore is used to manage the reader(s) waiting for
error messages.  When there are sleepers waiting, "up" is used to wake
one up when a new error message is produced.  A reader gets a single
message, not just some text with a mixture of different errors.

If I do,

  cat /dev/etherd/err > /my/log/file

... then I can hit control-c or send a SIGTERM to stop it.

> It appears to me that nblocked_emsgs_readers gets incorrectly
> decremented if the down_interruptible() got interrupted, btw.

The counter will be incremented again if the process goes back to
sleep waiting for an error message, but the process might be getting
killed.  The counter is really just for sleeping readers.

-- 
  Ed L Cashin <ecashin@coraid.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/