[PATCH] sound_core.c: Remove BKL from soundcore_open

[PATCH] sound_core.c: Remove BKL from soundcore_open

[John Kacur]

>From 030af455d4f54482130c8eccb47fe90aaba8808c Mon Sep 17 00:00:00 2001
From: John Kacur <jkacur@redhat.com>
Date: Sat, 10 Oct 2009 23:39:56 +0200
Subject: [PATCH] This code is already protected by spin_lock, and doesn't require the bkl

Signed-off-by: John Kacur <jkacur@redhat.com>
---
 sound/sound_core.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/sound/sound_core.c b/sound/sound_core.c
index 49c9981..03bb943 100644
--- a/sound/sound_core.c
+++ b/sound/sound_core.c
@@ -576,8 +576,6 @@ static int soundcore_open(struct inode *inode, struct file *file)
 	struct sound_unit *s;
 	const struct file_operations *new_fops = NULL;
 
-	lock_kernel ();
-
 	chain=unit&0x0F;
 	if(chain==4 || chain==5)	/* dsp/audio/dsp16 */
 	{
@@ -637,11 +635,9 @@ static int soundcore_open(struct inode *inode, struct file *file)
 			file->f_op = fops_get(old_fops);
 		}
 		fops_put(old_fops);
-		unlock_kernel();
 		return err;
 	}
 	spin_unlock(&sound_loader_lock);
-	unlock_kernel();
 	return -ENODEV;
 }
 
-- 
1.6.0.6

Re: [PATCH] sound_core.c: Remove BKL from soundcore_open

[Alan Cox]

On Sun, 11 Oct 2009 01:24:14 +0200 (CEST)
John Kacur <jkacur@redhat.com> wrote:

> >From 030af455d4f54482130c8eccb47fe90aaba8808c Mon Sep 17 00:00:00 2001
> From: John Kacur <jkacur@redhat.com>
> Date: Sat, 10 Oct 2009 23:39:56 +0200
> Subject: [PATCH] This code is already protected by spin_lock, and doesn't require the bkl

Sorry but I don't think that is true becaue of:

           spin_unlock(&sound_loader_lock);
                if(file->f_op->open)
                        err = file->f_op->open(inode,file);


So the underlying driver open method expects lock_kernel status and you
don't propogate it down. You really need to track down each thing that
can be called into here and fix it, or maybe just punt for the moment and
push it down to

	{
		lock_kernel()
		err = file-f_op->open ...
		unlock_kernel()
	}

so its obvious to the next person who takes up the war on the BKL what is
to be tackled.

Re: [PATCH] sound_core.c: Remove BKL from soundcore_open

[John Kacur]

On Sun, 11 Oct 2009, Alan Cox wrote:

> On Sun, 11 Oct 2009 01:24:14 +0200 (CEST)
> John Kacur <jkacur@redhat.com> wrote:
> 
> > >From 030af455d4f54482130c8eccb47fe90aaba8808c Mon Sep 17 00:00:00 2001
> > From: John Kacur <jkacur@redhat.com>
> > Date: Sat, 10 Oct 2009 23:39:56 +0200
> > Subject: [PATCH] This code is already protected by spin_lock, and doesn't require the bkl
> 
> Sorry but I don't think that is true becaue of:
> 
>            spin_unlock(&sound_loader_lock);
>                 if(file->f_op->open)
>                         err = file->f_op->open(inode,file);
> 
> 
> So the underlying driver open method expects lock_kernel status and you
> don't propogate it down. You really need to track down each thing that
> can be called into here and fix it, or maybe just punt for the moment and
> push it down to
> 
> 	{
> 		lock_kernel()
> 		err = file-f_op->open ...
> 		unlock_kernel()
> 	}
> 
> so its obvious to the next person who takes up the war on the BKL what is
> to be tackled.
> 

Yikes, I missed that. Still I'm loath to just push it down like that. I 
wonder if I can use a mutex there. What about the following patch?

>From 8b0b91523ee2fcf60ccd82dba44b8da8bad34ce4 Mon Sep 17 00:00:00 2001
From: John Kacur <jkacur@redhat.com>
Date: Sun, 11 Oct 2009 02:14:44 +0200
Subject: [PATCH] Remove the bkl in soundcore_open

Remove the bkl in soundcore_open since it is mostly covered by the sound_loader_lock spin_lock

Protect the underlying driver open method with a mutex.

Signed-off-by: John Kacur <jkacur@redhat.com>
---
 sound/sound_core.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/sound_core.c b/sound/sound_core.c
index 49c9981..6afb6f1 100644
--- a/sound/sound_core.c
+++ b/sound/sound_core.c
@@ -14,6 +14,8 @@
 #include <linux/major.h>
 #include <sound/core.h>
 
+static DEFINE_MUTEX(osc_mutex);
+
 #ifdef CONFIG_SOUND_OSS_CORE
 static int __init init_oss_soundcore(void);
 static void cleanup_oss_soundcore(void);
@@ -576,8 +578,6 @@ static int soundcore_open(struct inode *inode, struct file *file)
 	struct sound_unit *s;
 	const struct file_operations *new_fops = NULL;
 
-	lock_kernel ();
-
 	chain=unit&0x0F;
 	if(chain==4 || chain==5)	/* dsp/audio/dsp16 */
 	{
@@ -631,17 +631,17 @@ static int soundcore_open(struct inode *inode, struct file *file)
 		file->f_op = new_fops;
 		spin_unlock(&sound_loader_lock);
 		if(file->f_op->open)
+			mutex_lock(&osc_mutex);
 			err = file->f_op->open(inode,file);
+			mutex_unlock(&osc_mutex);
 		if (err) {
 			fops_put(file->f_op);
 			file->f_op = fops_get(old_fops);
 		}
 		fops_put(old_fops);
-		unlock_kernel();
 		return err;
 	}
 	spin_unlock(&sound_loader_lock);
-	unlock_kernel();
 	return -ENODEV;
 }
 
-- 
1.6.0.6

Re: [PATCH] sound_core.c: Remove BKL from soundcore_open

[Frederic Weisbecker]

On Sun, Oct 11, 2009 at 02:25:53AM +0200, John Kacur wrote:
> 
> 
> On Sun, 11 Oct 2009, Alan Cox wrote:
> 
> > On Sun, 11 Oct 2009 01:24:14 +0200 (CEST)
> > John Kacur <jkacur@redhat.com> wrote:
> > 
> > > >From 030af455d4f54482130c8eccb47fe90aaba8808c Mon Sep 17 00:00:00 2001
> > > From: John Kacur <jkacur@redhat.com>
> > > Date: Sat, 10 Oct 2009 23:39:56 +0200
> > > Subject: [PATCH] This code is already protected by spin_lock, and doesn't require the bkl
> > 
> > Sorry but I don't think that is true becaue of:
> > 
> >            spin_unlock(&sound_loader_lock);
> >                 if(file->f_op->open)
> >                         err = file->f_op->open(inode,file);
> > 
> > 
> > So the underlying driver open method expects lock_kernel status and you
> > don't propogate it down. You really need to track down each thing that
> > can be called into here and fix it, or maybe just punt for the moment and
> > push it down to
> > 
> > 	{
> > 		lock_kernel()
> > 		err = file-f_op->open ...
> > 		unlock_kernel()
> > 	}
> > 
> > so its obvious to the next person who takes up the war on the BKL what is
> > to be tackled.
> > 
> 
> Yikes, I missed that. Still I'm loath to just push it down like that. I 
> wonder if I can use a mutex there. What about the following patch?
> 
> From 8b0b91523ee2fcf60ccd82dba44b8da8bad34ce4 Mon Sep 17 00:00:00 2001
> From: John Kacur <jkacur@redhat.com>
> Date: Sun, 11 Oct 2009 02:14:44 +0200
> Subject: [PATCH] Remove the bkl in soundcore_open
> 
> Remove the bkl in soundcore_open since it is mostly covered by the sound_loader_lock spin_lock
> 
> Protect the underlying driver open method with a mutex.
> 
> Signed-off-by: John Kacur <jkacur@redhat.com>
> ---
>  sound/sound_core.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/sound_core.c b/sound/sound_core.c
> index 49c9981..6afb6f1 100644
> --- a/sound/sound_core.c
> +++ b/sound/sound_core.c
> @@ -14,6 +14,8 @@
>  #include <linux/major.h>
>  #include <sound/core.h>
>  
> +static DEFINE_MUTEX(osc_mutex);
> +
>  #ifdef CONFIG_SOUND_OSS_CORE
>  static int __init init_oss_soundcore(void);
>  static void cleanup_oss_soundcore(void);
> @@ -576,8 +578,6 @@ static int soundcore_open(struct inode *inode, struct file *file)
>  	struct sound_unit *s;
>  	const struct file_operations *new_fops = NULL;
>  
> -	lock_kernel ();
> -
>  	chain=unit&0x0F;
>  	if(chain==4 || chain==5)	/* dsp/audio/dsp16 */
>  	{
> @@ -631,17 +631,17 @@ static int soundcore_open(struct inode *inode, struct file *file)
>  		file->f_op = new_fops;
>  		spin_unlock(&sound_loader_lock);
>  		if(file->f_op->open)
> +			mutex_lock(&osc_mutex);
>  			err = file->f_op->open(inode,file);
> +			mutex_unlock(&osc_mutex);


Yeah that's tempting, but I fear that also means this mutex will
never be removed....

Re: [PATCH] sound_core.c: Remove BKL from soundcore_open

[John Kacur]

On Sun, 11 Oct 2009, Frederic Weisbecker wrote:

> On Sun, Oct 11, 2009 at 02:25:53AM +0200, John Kacur wrote:
> > 
> > 
> > On Sun, 11 Oct 2009, Alan Cox wrote:
> > 
> > > On Sun, 11 Oct 2009 01:24:14 +0200 (CEST)
> > > John Kacur <jkacur@redhat.com> wrote:
> > > 
> > > > >From 030af455d4f54482130c8eccb47fe90aaba8808c Mon Sep 17 00:00:00 2001
> > > > From: John Kacur <jkacur@redhat.com>
> > > > Date: Sat, 10 Oct 2009 23:39:56 +0200
> > > > Subject: [PATCH] This code is already protected by spin_lock, and doesn't require the bkl
> > > 
> > > Sorry but I don't think that is true becaue of:
> > > 
> > >            spin_unlock(&sound_loader_lock);
> > >                 if(file->f_op->open)
> > >                         err = file->f_op->open(inode,file);
> > > 
> > > 
> > > So the underlying driver open method expects lock_kernel status and you
> > > don't propogate it down. You really need to track down each thing that
> > > can be called into here and fix it, or maybe just punt for the moment and
> > > push it down to
> > > 
> > > 	{
> > > 		lock_kernel()
> > > 		err = file-f_op->open ...
> > > 		unlock_kernel()
> > > 	}
> > > 
> > > so its obvious to the next person who takes up the war on the BKL what is
> > > to be tackled.
> > > 
> > 
> > Yikes, I missed that. Still I'm loath to just push it down like that. I 
> > wonder if I can use a mutex there. What about the following patch?
> > 
> > From 8b0b91523ee2fcf60ccd82dba44b8da8bad34ce4 Mon Sep 17 00:00:00 2001
> > From: John Kacur <jkacur@redhat.com>
> > Date: Sun, 11 Oct 2009 02:14:44 +0200
> > Subject: [PATCH] Remove the bkl in soundcore_open
> > 
> > Remove the bkl in soundcore_open since it is mostly covered by the sound_loader_lock spin_lock
> > 
> > Protect the underlying driver open method with a mutex.
> > 
> > Signed-off-by: John Kacur <jkacur@redhat.com>
> > ---
> >  sound/sound_core.c |    8 ++++----
> >  1 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/sound/sound_core.c b/sound/sound_core.c
> > index 49c9981..6afb6f1 100644
> > --- a/sound/sound_core.c
> > +++ b/sound/sound_core.c
> > @@ -14,6 +14,8 @@
> >  #include <linux/major.h>
> >  #include <sound/core.h>
> >  
> > +static DEFINE_MUTEX(osc_mutex);
> > +
> >  #ifdef CONFIG_SOUND_OSS_CORE
> >  static int __init init_oss_soundcore(void);
> >  static void cleanup_oss_soundcore(void);
> > @@ -576,8 +578,6 @@ static int soundcore_open(struct inode *inode, struct file *file)
> >  	struct sound_unit *s;
> >  	const struct file_operations *new_fops = NULL;
> >  
> > -	lock_kernel ();
> > -
> >  	chain=unit&0x0F;
> >  	if(chain==4 || chain==5)	/* dsp/audio/dsp16 */
> >  	{
> > @@ -631,17 +631,17 @@ static int soundcore_open(struct inode *inode, struct file *file)
> >  		file->f_op = new_fops;
> >  		spin_unlock(&sound_loader_lock);
> >  		if(file->f_op->open)
> > +			mutex_lock(&osc_mutex);
> >  			err = file->f_op->open(inode,file);
> > +			mutex_unlock(&osc_mutex);
> 
> 
> Yeah that's tempting, but I fear that also means this mutex will
> never be removed....
> 

Sigh... I do see your point - but on the otherhand if measurements don't
show that mutex as being too coarse grained, then is it a problem?

Never-the-less here is version 3 of the patch - like Alan suggested, 
punting, but at least reducing the area covered by the BKL.
>From ac9bdbdd192149e2498b6e16dc71f0a3933e1554 Mon Sep 17 00:00:00 2001
From: John Kacur <jkacur@redhat.com>
Date: Sun, 11 Oct 2009 14:25:46 +0200
Subject: [PATCH] soundcore_open: Reduce the area BKL coverage in this function.

Most of this function is protected by the sound_loader_lock.
We can push down the BKL to this call out err = file->f_op->open(inode,file);

Signed-off-by: John Kacur <jkacur@redhat.com>
---
 sound/sound_core.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/sound/sound_core.c b/sound/sound_core.c
index 49c9981..a7d6956 100644
--- a/sound/sound_core.c
+++ b/sound/sound_core.c
@@ -576,8 +576,6 @@ static int soundcore_open(struct inode *inode, struct file *file)
 	struct sound_unit *s;
 	const struct file_operations *new_fops = NULL;
 
-	lock_kernel ();
-
 	chain=unit&0x0F;
 	if(chain==4 || chain==5)	/* dsp/audio/dsp16 */
 	{
@@ -631,17 +629,17 @@ static int soundcore_open(struct inode *inode, struct file *file)
 		file->f_op = new_fops;
 		spin_unlock(&sound_loader_lock);
 		if(file->f_op->open)
+			lock_kernel();
 			err = file->f_op->open(inode,file);
+			unlock_kernel();
 		if (err) {
 			fops_put(file->f_op);
 			file->f_op = fops_get(old_fops);
 		}
 		fops_put(old_fops);
-		unlock_kernel();
 		return err;
 	}
 	spin_unlock(&sound_loader_lock);
-	unlock_kernel();
 	return -ENODEV;
 }
 
-- 
1.6.0.6

Re: [PATCH] sound_core.c: Remove BKL from soundcore_open

[Oliver Neukum]

Am Sonntag, 11. Oktober 2009 14:41:15 schrieb John Kacur:> @@ -576,8 +576,6 @@ static int soundcore_open(struct inode *inode, struct> file *file) struct sound_unit *s;>         const struct file_operations *new_fops = NULL;>  > -       lock_kernel ();> ->         chain=unit&0x0F;>         if(chain==4 || chain==5)        /* dsp/audio/dsp16 */>         {> @@ -631,17 +629,17 @@ static int soundcore_open(struct inode *inode, struct> file *file) file->f_op = new_fops;>                 spin_unlock(&sound_loader_lock);>                 if(file->f_op->open)> +                       lock_kernel();>                         err = file->f_op->open(inode,file);> +                       unlock_kernel();>                 if (err) {>                         fops_put(file->f_op);>                         file->f_op = fops_get(old_fops);
Is that just me, or is file->f_op unguarded in this version?
	Regards		Oliver
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] sound_core.c: Remove BKL from soundcore_open

[Frederic Weisbecker]

On Sun, Oct 11, 2009 at 04:12:40PM +0200, Oliver Neukum wrote:
> Am Sonntag, 11. Oktober 2009 14:41:15 schrieb John Kacur:
> > @@ -576,8 +576,6 @@ static int soundcore_open(struct inode *inode, struct
> > file *file) struct sound_unit *s;
> >         const struct file_operations *new_fops = NULL;
> >  
> > -       lock_kernel ();
> > -
> >         chain=unit&0x0F;
> >         if(chain==4 || chain==5)        /* dsp/audio/dsp16 */
> >         {
> > @@ -631,17 +629,17 @@ static int soundcore_open(struct inode *inode, struct
> > file *file) file->f_op = new_fops;
> >                 spin_unlock(&sound_loader_lock);
> >                 if(file->f_op->open)
> > +                       lock_kernel();
> >                         err = file->f_op->open(inode,file);
> > +                       unlock_kernel();
> >                 if (err) {
> >                         fops_put(file->f_op);
> >                         file->f_op = fops_get(old_fops);
> 
> Is that just me, or is file->f_op unguarded in this version?
> 
> 	Regards
> 		Oliver


Once assigned to file, the new fops won't move again.
And won't be freed until fops_put is called.

Re: [PATCH] sound_core.c: Remove BKL from soundcore_open

[John Kacur]

On Sun, 11 Oct 2009, John Kacur wrote:

> 
> 
> On Sun, 11 Oct 2009, Frederic Weisbecker wrote:
> 
> > On Sun, Oct 11, 2009 at 02:25:53AM +0200, John Kacur wrote:
> > > 
> > > 
> > > On Sun, 11 Oct 2009, Alan Cox wrote:
> > > 
> > > > On Sun, 11 Oct 2009 01:24:14 +0200 (CEST)
> > > > John Kacur <jkacur@redhat.com> wrote:
> > > > 
> > > > > >From 030af455d4f54482130c8eccb47fe90aaba8808c Mon Sep 17 00:00:00 2001
> > > > > From: John Kacur <jkacur@redhat.com>
> > > > > Date: Sat, 10 Oct 2009 23:39:56 +0200
> > > > > Subject: [PATCH] This code is already protected by spin_lock, and doesn't require the bkl
> > > > 
> > > > Sorry but I don't think that is true becaue of:
> > > > 
> > > >            spin_unlock(&sound_loader_lock);
> > > >                 if(file->f_op->open)
> > > >                         err = file->f_op->open(inode,file);
> > > > 
> > > > 
> > > > So the underlying driver open method expects lock_kernel status and you
> > > > don't propogate it down. You really need to track down each thing that
> > > > can be called into here and fix it, or maybe just punt for the moment and
> > > > push it down to
> > > > 
> > > > 	{
> > > > 		lock_kernel()
> > > > 		err = file-f_op->open ...
> > > > 		unlock_kernel()
> > > > 	}
> > > > 
> > > > so its obvious to the next person who takes up the war on the BKL what is
> > > > to be tackled.
> > > > 
> > > 
> > > Yikes, I missed that. Still I'm loath to just push it down like that. I 
> > > wonder if I can use a mutex there. What about the following patch?
> > > 
> > > From 8b0b91523ee2fcf60ccd82dba44b8da8bad34ce4 Mon Sep 17 00:00:00 2001
> > > From: John Kacur <jkacur@redhat.com>
> > > Date: Sun, 11 Oct 2009 02:14:44 +0200
> > > Subject: [PATCH] Remove the bkl in soundcore_open
> > > 
> > > Remove the bkl in soundcore_open since it is mostly covered by the sound_loader_lock spin_lock
> > > 
> > > Protect the underlying driver open method with a mutex.
> > > 
> > > Signed-off-by: John Kacur <jkacur@redhat.com>
> > > ---
> > >  sound/sound_core.c |    8 ++++----
> > >  1 files changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/sound/sound_core.c b/sound/sound_core.c
> > > index 49c9981..6afb6f1 100644
> > > --- a/sound/sound_core.c
> > > +++ b/sound/sound_core.c
> > > @@ -14,6 +14,8 @@
> > >  #include <linux/major.h>
> > >  #include <sound/core.h>
> > >  
> > > +static DEFINE_MUTEX(osc_mutex);
> > > +
> > >  #ifdef CONFIG_SOUND_OSS_CORE
> > >  static int __init init_oss_soundcore(void);
> > >  static void cleanup_oss_soundcore(void);
> > > @@ -576,8 +578,6 @@ static int soundcore_open(struct inode *inode, struct file *file)
> > >  	struct sound_unit *s;
> > >  	const struct file_operations *new_fops = NULL;
> > >  
> > > -	lock_kernel ();
> > > -
> > >  	chain=unit&0x0F;
> > >  	if(chain==4 || chain==5)	/* dsp/audio/dsp16 */
> > >  	{
> > > @@ -631,17 +631,17 @@ static int soundcore_open(struct inode *inode, struct file *file)
> > >  		file->f_op = new_fops;
> > >  		spin_unlock(&sound_loader_lock);
> > >  		if(file->f_op->open)
> > > +			mutex_lock(&osc_mutex);
> > >  			err = file->f_op->open(inode,file);
> > > +			mutex_unlock(&osc_mutex);
> > 
> > 
> > Yeah that's tempting, but I fear that also means this mutex will
> > never be removed....
> > 
> 
> Sigh... I do see your point - but on the otherhand if measurements don't
> show that mutex as being too coarse grained, then is it a problem?
> 
> Never-the-less here is version 3 of the patch - like Alan suggested, 
> punting, but at least reducing the area covered by the BKL.
> From ac9bdbdd192149e2498b6e16dc71f0a3933e1554 Mon Sep 17 00:00:00 2001
> From: John Kacur <jkacur@redhat.com>
> Date: Sun, 11 Oct 2009 14:25:46 +0200
> Subject: [PATCH] soundcore_open: Reduce the area BKL coverage in this function.
> 
> Most of this function is protected by the sound_loader_lock.
> We can push down the BKL to this call out err = file->f_op->open(inode,file);
> 
> Signed-off-by: John Kacur <jkacur@redhat.com>
> ---
>  sound/sound_core.c |    6 ++----
>  1 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/sound_core.c b/sound/sound_core.c
> index 49c9981..a7d6956 100644
> --- a/sound/sound_core.c
> +++ b/sound/sound_core.c
> @@ -576,8 +576,6 @@ static int soundcore_open(struct inode *inode, struct file *file)
>  	struct sound_unit *s;
>  	const struct file_operations *new_fops = NULL;
>  
> -	lock_kernel ();
> -
>  	chain=unit&0x0F;
>  	if(chain==4 || chain==5)	/* dsp/audio/dsp16 */
>  	{
> @@ -631,17 +629,17 @@ static int soundcore_open(struct inode *inode, struct file *file)
>  		file->f_op = new_fops;
>  		spin_unlock(&sound_loader_lock);
>  		if(file->f_op->open)
> +			lock_kernel();
>  			err = file->f_op->open(inode,file);
> +			unlock_kernel();
>  		if (err) {
>  			fops_put(file->f_op);
>  			file->f_op = fops_get(old_fops);
>  		}
>  		fops_put(old_fops);
> -		unlock_kernel();
>  		return err;
>  	}
>  	spin_unlock(&sound_loader_lock);
> -	unlock_kernel();
>  	return -ENODEV;
>  }
>  
> -- 
> 1.6.0.6
> 

@Alan

Are you okay with this 3rd version of the patch that pushes the bkl lock
further down into the function so that it is only around the 
err = file->f_op->open(inode,file);

Not ideal - but an improvement and step in the right direction.

If so, maybe I can get an ack, so that Thomas might include it in his new
kill-the-bkl tree.

Thanks

Re: [PATCH] sound_core.c: Remove BKL from soundcore_open

[Takashi Iwai]

At Sun, 11 Oct 2009 14:41:15 +0200 (CEST),
John Kacur wrote:
> 
> @@ -631,17 +629,17 @@ static int soundcore_open(struct inode *inode, struct file *file)
>  		file->f_op = new_fops;
>  		spin_unlock(&sound_loader_lock);
>  		if(file->f_op->open)
> +			lock_kernel();
>  			err = file->f_op->open(inode,file);
> +			unlock_kernel();

You certainly want braces around here, no?


Takashi

Re: [PATCH] sound_core.c: Remove BKL from soundcore_open

[John Kacur]

On Mon, 12 Oct 2009, Takashi Iwai wrote:

> At Sun, 11 Oct 2009 14:41:15 +0200 (CEST),
> John Kacur wrote:
> > 
> > @@ -631,17 +629,17 @@ static int soundcore_open(struct inode *inode, struct file *file)
> >  		file->f_op = new_fops;
> >  		spin_unlock(&sound_loader_lock);
> >  		if(file->f_op->open)
> > +			lock_kernel();
> >  			err = file->f_op->open(inode,file);
> > +			unlock_kernel();
> 
> You certainly want braces around here, no?
> 

Oh, I don't know, I was kinda hoping that the spaces would magically 
impart bracketnishish to the whole thing. My God yes we want the brackets -
Thank you Takashi!

What follows is version four.

>From 90f527d2ae660a3a0e712075479a4cc24504ad45 Mon Sep 17 00:00:00 2001
From: John Kacur <jkacur@redhat.com>
Date: Sun, 11 Oct 2009 14:25:46 +0200
Subject: [PATCH] soundcore_open: Reduce the area BKL coverage in this function.

Most of this function is protected by the sound_loader_lock.
We can push down the BKL to this call out err = file->f_op->open(inode,file);

Signed-off-by: John Kacur <jkacur@redhat.com>
Acked-by: Alan Cox <alan@linux.intel.com>
---
 sound/sound_core.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/sound/sound_core.c b/sound/sound_core.c
index 49c9981..643a61f 100644
--- a/sound/sound_core.c
+++ b/sound/sound_core.c
@@ -576,8 +576,6 @@ static int soundcore_open(struct inode *inode, struct file *file)
 	struct sound_unit *s;
 	const struct file_operations *new_fops = NULL;
 
-	lock_kernel ();
-
 	chain=unit&0x0F;
 	if(chain==4 || chain==5)	/* dsp/audio/dsp16 */
 	{
@@ -630,18 +628,18 @@ static int soundcore_open(struct inode *inode, struct file *file)
 		const struct file_operations *old_fops = file->f_op;
 		file->f_op = new_fops;
 		spin_unlock(&sound_loader_lock);
-		if(file->f_op->open)
+		if(file->f_op->open) {
+			lock_kernel();
 			err = file->f_op->open(inode,file);
-		if (err) {
+			unlock_kernel();
+		} if (err) {
 			fops_put(file->f_op);
 			file->f_op = fops_get(old_fops);
 		}
 		fops_put(old_fops);
-		unlock_kernel();
 		return err;
 	}
 	spin_unlock(&sound_loader_lock);
-	unlock_kernel();
 	return -ENODEV;
 }
 
-- 
1.6.0.6

Re: [PATCH] sound_core.c: Remove BKL from soundcore_open

[Takashi Iwai]

At Mon, 12 Oct 2009 10:37:05 +0200 (CEST),
John Kacur wrote:
> 
> On Mon, 12 Oct 2009, Takashi Iwai wrote:
> 
> > At Sun, 11 Oct 2009 14:41:15 +0200 (CEST),
> > John Kacur wrote:
> > > 
> > > @@ -631,17 +629,17 @@ static int soundcore_open(struct inode *inode, struct file *file)
> > >  		file->f_op = new_fops;
> > >  		spin_unlock(&sound_loader_lock);
> > >  		if(file->f_op->open)
> > > +			lock_kernel();
> > >  			err = file->f_op->open(inode,file);
> > > +			unlock_kernel();
> > 
> > You certainly want braces around here, no?
> > 
> 
> Oh, I don't know, I was kinda hoping that the spaces would magically 
> impart bracketnishish to the whole thing. My God yes we want the brackets -
> Thank you Takashi!
> 
> What follows is version four.
> 
> >From 90f527d2ae660a3a0e712075479a4cc24504ad45 Mon Sep 17 00:00:00 2001
> From: John Kacur <jkacur@redhat.com>
> Date: Sun, 11 Oct 2009 14:25:46 +0200
> Subject: [PATCH] soundcore_open: Reduce the area BKL coverage in this function.
> 
> Most of this function is protected by the sound_loader_lock.
> We can push down the BKL to this call out err = file->f_op->open(inode,file);
> 
> Signed-off-by: John Kacur <jkacur@redhat.com>
> Acked-by: Alan Cox <alan@linux.intel.com>
> ---
>  sound/sound_core.c |   10 ++++------
>  1 files changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/sound/sound_core.c b/sound/sound_core.c
> index 49c9981..643a61f 100644
> --- a/sound/sound_core.c
> +++ b/sound/sound_core.c
> @@ -576,8 +576,6 @@ static int soundcore_open(struct inode *inode, struct file *file)
>  	struct sound_unit *s;
>  	const struct file_operations *new_fops = NULL;
>  
> -	lock_kernel ();
> -
>  	chain=unit&0x0F;
>  	if(chain==4 || chain==5)	/* dsp/audio/dsp16 */
>  	{
> @@ -630,18 +628,18 @@ static int soundcore_open(struct inode *inode, struct file *file)
>  		const struct file_operations *old_fops = file->f_op;
>  		file->f_op = new_fops;
>  		spin_unlock(&sound_loader_lock);
> -		if(file->f_op->open)
> +		if(file->f_op->open) {
> +			lock_kernel();
>  			err = file->f_op->open(inode,file);
> -		if (err) {
> +			unlock_kernel();
> +		} if (err) {

It's better to break the line after the closing brace here.


Takashi

Re: [PATCH] sound_core.c: Remove BKL from soundcore_open

[John Kacur]

On Mon, 12 Oct 2009, Takashi Iwai wrote:

> At Mon, 12 Oct 2009 10:37:05 +0200 (CEST),
> John Kacur wrote:
> > 
> > On Mon, 12 Oct 2009, Takashi Iwai wrote:
> > 
> > > At Sun, 11 Oct 2009 14:41:15 +0200 (CEST),
> > > John Kacur wrote:
> > > > 
> > > > @@ -631,17 +629,17 @@ static int soundcore_open(struct inode *inode, struct file *file)
> > > >  		file->f_op = new_fops;
> > > >  		spin_unlock(&sound_loader_lock);
> > > >  		if(file->f_op->open)
> > > > +			lock_kernel();
> > > >  			err = file->f_op->open(inode,file);
> > > > +			unlock_kernel();
> > > 
> > > You certainly want braces around here, no?
> > > 
> > 
> > Oh, I don't know, I was kinda hoping that the spaces would magically 
> > impart bracketnishish to the whole thing. My God yes we want the brackets -
> > Thank you Takashi!
> > 
> > What follows is version four.
> > 
> > >From 90f527d2ae660a3a0e712075479a4cc24504ad45 Mon Sep 17 00:00:00 2001
> > From: John Kacur <jkacur@redhat.com>
> > Date: Sun, 11 Oct 2009 14:25:46 +0200
> > Subject: [PATCH] soundcore_open: Reduce the area BKL coverage in this function.
> > 
> > Most of this function is protected by the sound_loader_lock.
> > We can push down the BKL to this call out err = file->f_op->open(inode,file);
> > 
> > Signed-off-by: John Kacur <jkacur@redhat.com>
> > Acked-by: Alan Cox <alan@linux.intel.com>
> > ---
> >  sound/sound_core.c |   10 ++++------
> >  1 files changed, 4 insertions(+), 6 deletions(-)
> > 
> > diff --git a/sound/sound_core.c b/sound/sound_core.c
> > index 49c9981..643a61f 100644
> > --- a/sound/sound_core.c
> > +++ b/sound/sound_core.c
> > @@ -576,8 +576,6 @@ static int soundcore_open(struct inode *inode, struct file *file)
> >  	struct sound_unit *s;
> >  	const struct file_operations *new_fops = NULL;
> >  
> > -	lock_kernel ();
> > -
> >  	chain=unit&0x0F;
> >  	if(chain==4 || chain==5)	/* dsp/audio/dsp16 */
> >  	{
> > @@ -630,18 +628,18 @@ static int soundcore_open(struct inode *inode, struct file *file)
> >  		const struct file_operations *old_fops = file->f_op;
> >  		file->f_op = new_fops;
> >  		spin_unlock(&sound_loader_lock);
> > -		if(file->f_op->open)
> > +		if(file->f_op->open) {
> > +			lock_kernel();
> >  			err = file->f_op->open(inode,file);
> > -		if (err) {
> > +			unlock_kernel();
> > +		} if (err) {
> 
> It's better to break the line after the closing brace here.
> 

Thank you for your review Takashi.

I corrected the style problems, and added a bit of spacing to make the 
code more readable.

Version 5 follows

>From 8007707e02f68b6315ff80aa3ca471af12eeb491 Mon Sep 17 00:00:00 2001
From: John Kacur <jkacur@redhat.com>
Date: Sun, 11 Oct 2009 14:25:46 +0200
Subject: [PATCH] soundcore_open: Reduce the area BKL coverage in this function.

Most of this function is protected by the sound_loader_lock.
We can push down the BKL to this call out err = file->f_op->open(inode,file);

Signed-off-by: John Kacur <jkacur@redhat.com>
Acked-by: Alan Cox <alan@linux.intel.com>
---
 sound/sound_core.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/sound/sound_core.c b/sound/sound_core.c
index 49c9981..2ca17ec 100644
--- a/sound/sound_core.c
+++ b/sound/sound_core.c
@@ -576,8 +576,6 @@ static int soundcore_open(struct inode *inode, struct file *file)
 	struct sound_unit *s;
 	const struct file_operations *new_fops = NULL;
 
-	lock_kernel ();
-
 	chain=unit&0x0F;
 	if(chain==4 || chain==5)	/* dsp/audio/dsp16 */
 	{
@@ -630,18 +628,22 @@ static int soundcore_open(struct inode *inode, struct file *file)
 		const struct file_operations *old_fops = file->f_op;
 		file->f_op = new_fops;
 		spin_unlock(&sound_loader_lock);
-		if(file->f_op->open)
+
+		if (file->f_op->open) {
+			lock_kernel();
 			err = file->f_op->open(inode,file);
+			unlock_kernel();
+		}
+
 		if (err) {
 			fops_put(file->f_op);
 			file->f_op = fops_get(old_fops);
 		}
+
 		fops_put(old_fops);
-		unlock_kernel();
 		return err;
 	}
 	spin_unlock(&sound_loader_lock);
-	unlock_kernel();
 	return -ENODEV;
 }
 
-- 
1.6.0.6

Re: [PATCH] sound_core.c: Remove BKL from soundcore_open

[Jonathan Corbet]

On Sun, 11 Oct 2009 02:25:53 +0200 (CEST)
John Kacur <jkacur@redhat.com> wrote:

> Yikes, I missed that. Still I'm loath to just push it down like that. I 
> wonder if I can use a mutex there. What about the following patch?

Unfortunately, it's often not quite that simple.  What if, say, there's
an ioctl() function somewhere which is depending on the BKL for
exclusion here?  This change would then introduce races.  Changing the
BKL to a mutex is a real semantic change which requires a real survey
of the code affected.

That's why the pushdown approach has been taken so often.  It's a pain,
but it eventually shines a spotlight on every bit of affected code.

jon

Re: [PATCH] sound_core.c: Remove BKL from soundcore_open

[Jonathan Corbet]

On Sun, 11 Oct 2009 09:20:15 -0600
Jonathan Corbet <corbet@lwn.net> wrote:

> Changing the
> BKL to a mutex is a real semantic change which requires a real survey
> of the code affected.

One other aspect of this I forgot to mention...it's actually possible
(if unlikely) that one of those lower-level open routines depends on
the BKL's release-on-sleep semantics.  Swapping in a mutex would change
that behavior, possibly resulting in deadlocks.

I think it was Alan who once pointed out that the BKL is badly
misnamed.  It isn't really a lock, it's a modified execution
environment designed to let naive kernel code think it's still running
in a uniprocessor, no-preemption situation.  Replacing the BKL with a
different lock changes that environment, so one has to be *really*
careful about looking for any assumptions which may remain in the code.

That's why BKL-hunting is harder than it looks - and why the BKL has
hung around for all these years.

jon

Re: [PATCH] sound_core.c: Remove BKL from soundcore_open

[Arjan van de Ven]

On Sun, 11 Oct 2009 11:15:43 -0600
Jonathan Corbet <corbet@lwn.net> wrote:

> On Sun, 11 Oct 2009 09:20:15 -0600
> Jonathan Corbet <corbet@lwn.net> wrote:
> 
> > Changing the
> > BKL to a mutex is a real semantic change which requires a real
> > survey of the code affected.
> 
> One other aspect of this I forgot to mention...it's actually possible
> (if unlikely) that one of those lower-level open routines depends on
> the BKL's release-on-sleep semantics.  Swapping in a mutex would
> change that behavior, possibly resulting in deadlocks.
> 
> I think it was Alan who once pointed out that the BKL is badly
> misnamed.  It isn't really a lock, it's a modified execution
> environment designed to let naive kernel code think it's still running
> in a uniprocessor, no-preemption situation.  Replacing the BKL with a
> different lock changes that environment, so one has to be *really*
> careful about looking for any assumptions which may remain in the
> code.
> 

it's getting time though to bite the bullet and make it a real normal
mutex. Lockdep will then flag a bunch of sh*t we'll need to fix, but
without doing that we're never going to really make progress. 

The BKL has outlived its usefulness. Many of the things it used to
protect (device open, module load etc) no longer take the BKL since
quite a while, making the notion of "if you see the BKL you have a bug" 
more and more true.

Going from rather obscure semantics to more defined, and more
importantly, checkable-by-lockdep semantics is going to be a step
forward in this sense; while we can't check that it protects what it
should, we can at least start treating it like a real lock...


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH] sound_core.c: Remove BKL from soundcore_open

[Alan Cox]

> it's getting time though to bite the bullet and make it a real normal
> mutex. Lockdep will then flag a bunch of sh*t we'll need to fix, but
> without doing that we're never going to really make progress. 

It won't. Instead you get situations like one ioctl blocking another to
an unrelated device that just causes weird failures and performance
problems, or in some cases deadlocks.

Open routines block so it takes about 5 seconds of thought to realise
that using a mutex here is brain dead and doesn't work.

You push the BKL down. Next step from the call point is to replicate it
inside each of the drivers that path can call, then in those drivers push
the lock_kernel down and the internal locking up. Eventually they meet in
the middle.

If you try and do botch jobs with bogus mutex hacks it breaks, it breaks
in ways lockdep doesn't understand and it doesn't even *help* fix the
problem proper.

Alan

Re: [PATCH] sound_core.c: Remove BKL from soundcore_open

[Arjan van de Ven]

On Sun, 11 Oct 2009 20:17:59 +0100
Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > it's getting time though to bite the bullet and make it a real
> > normal mutex. Lockdep will then flag a bunch of sh*t we'll need to
> > fix, but without doing that we're never going to really make
> > progress. 
> 
> It won't. Instead you get situations like one ioctl blocking another
> to an unrelated device that just causes weird failures and performance
> problems, or in some cases deadlocks.

yes the bkl using code will be slower because it'll now hit contention.

The deadlocks we need to catch imo; those are the behaviors that are
the worst offenders in terms of BKL weird behavior.

> 
> Open routines block so it takes about 5 seconds of thought to realise
> that using a mutex here is brain dead and doesn't work.

it also takes 5 seconds to realize "uh oh. they block. BKL is rather
limited in what it provides".


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH] sound_core.c: Remove BKL from soundcore_open

[Alan Cox]

> > It won't. Instead you get situations like one ioctl blocking another
> > to an unrelated device that just causes weird failures and performance
> > problems, or in some cases deadlocks.
> 
> yes the bkl using code will be slower because it'll now hit contention.

No - the mutex using ioctls that sleep now block each other out - this
mistake was made in some video drivers.

> > Open routines block so it takes about 5 seconds of thought to realise
> > that using a mutex here is brain dead and doesn't work.
> 
> it also takes 5 seconds to realize "uh oh. they block. BKL is rather
> limited in what it provides".

Which is what the code was written for.

This is why you can't just slap in a mutex but have to push it down.

Chances are that for a lot of small drivers you go

lock_kernel
foo->op()
unlock_kernel


to

foo->op()

	op()
	lock_kernel
	blah
	unlock_kernel

correctly on to

	op()
	{
		mutex_lock(instance->lock);
		blah
		mutex_unlock(instance->lock);

but you can't jump those steps and hope to get it right.

Alan