Re: [PATCH 09/13] remove race between use and initialization of locks
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" <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 09/13] remove race between use and initialization of locks
On Thu, 20 Dec 2007 17:15:57 -0500 "Ed L. Cashin" <[EMAIL PROTECTED]> wrote:
> Alexey Dobriyan noticed a race in the initialization of the dynamic
> locks in ...
>
> Message-ID: <[EMAIL PROTECTED]>
>
> Andrew Morton commented that these locks should be initialized at
> compile time, so this patch does that.
>
> Signed-off-by: Ed L. Cashin <[EMAIL PROTECTED]>
> ---
> drivers/block/aoe/aoechr.c |6 ++
> 1 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/block/aoe/aoechr.c b/drivers/block/aoe/aoechr.c
> index 1bc85aa..670bba6 100644
> --- a/drivers/block/aoe/aoechr.c
> +++ b/drivers/block/aoe/aoechr.c
> @@ -35,8 +35,8 @@ struct ErrMsg {
>
> static struct ErrMsg emsgs[NMSG];
> static int emsgs_head_idx, emsgs_tail_idx;
> -static struct semaphore emsgs_sema;
> -static spinlock_t emsgs_lock;
> +static __DECLARE_SEMAPHORE_GENERIC(emsgs_sema, 0);
That's not very attractive. This would be the only user of
__DECLARE_SEMAPHORE_GENERIC() in the tree. It seems that all architectures
(apart from uml) do implement this (although I didn't verify that they all
offer the same interface for it). But still...
And no, we don't appear to have a proper interface to statically declare a
locked semaphore. Nor do we appear to have one for mutexes(!).
> +static DEFINE_SPINLOCK(emsgs_lock);
> static int nblocked_emsgs_readers;
> static struct class *aoe_class;
> static struct aoe_chardev chardevs[] = {
> @@ -264,8 +264,6 @@ aoechr_init(void)
> printk(KERN_ERR "aoe: can't register char device\n");
> return n;
> }
> - 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.
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?
It appears to me that nblocked_emsgs_readers gets incorrectly decremented
if the down_interruptible() got interrupted, btw.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 09/13] remove race between use and initialization of locks
Alexey Dobriyan noticed a race in the initialization of the dynamic
locks in ...
Message-ID: <[EMAIL PROTECTED]>
Andrew Morton commented that these locks should be initialized at
compile time, so this patch does that.
Signed-off-by: Ed L. Cashin <[EMAIL PROTECTED]>
---
drivers/block/aoe/aoechr.c |6 ++
1 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/block/aoe/aoechr.c b/drivers/block/aoe/aoechr.c
index 1bc85aa..670bba6 100644
--- a/drivers/block/aoe/aoechr.c
+++ b/drivers/block/aoe/aoechr.c
@@ -35,8 +35,8 @@ struct ErrMsg {
static struct ErrMsg emsgs[NMSG];
static int emsgs_head_idx, emsgs_tail_idx;
-static struct semaphore emsgs_sema;
-static spinlock_t emsgs_lock;
+static __DECLARE_SEMAPHORE_GENERIC(emsgs_sema, 0);
+static DEFINE_SPINLOCK(emsgs_lock);
static int nblocked_emsgs_readers;
static struct class *aoe_class;
static struct aoe_chardev chardevs[] = {
@@ -264,8 +264,6 @@ aoechr_init(void)
printk(KERN_ERR "aoe: can't register char device\n");
return n;
}
- 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");
--
1.5.3.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 09/13] remove race between use and initialization of locks
Alexey Dobriyan noticed a race in the initialization of the dynamic
locks in ...
Message-ID: <[EMAIL PROTECTED]>
Andrew Morton commented that these locks should be initialized at
compile time, so this patch does that.
Signed-off-by: Ed L. Cashin <[EMAIL PROTECTED]>
---
drivers/block/aoe/aoechr.c |6 ++
1 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/block/aoe/aoechr.c b/drivers/block/aoe/aoechr.c
index 166f54f..0ce9bda 100644
--- a/drivers/block/aoe/aoechr.c
+++ b/drivers/block/aoe/aoechr.c
@@ -35,8 +35,8 @@ struct ErrMsg {
static struct ErrMsg emsgs[NMSG];
static int emsgs_head_idx, emsgs_tail_idx;
-static struct semaphore emsgs_sema;
-static spinlock_t emsgs_lock;
+static __DECLARE_SEMAPHORE_GENERIC(emsgs_sema, 0);
+static DEFINE_SPINLOCK(emsgs_lock);
static int nblocked_emsgs_readers;
static struct class *aoe_class;
static struct aoe_chardev chardevs[] = {
@@ -264,8 +264,6 @@ aoechr_init(void)
printk(KERN_ERR "aoe: can't register char device\n");
return n;
}
- 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");
--
1.5.3.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
