[PATCH] md: avoid potential long delay under pers_lock

[PATCH] md: avoid potential long delay under pers_lock

[Chao Yu]

printk may cause long time lapse if value of printk_delay in sysctl is
configured large by user. If register_md_personality takes long time to print in
spinlock pers_lock, we may encounter high CPU usage rate when there are other
pers_lock competitors who may be blocked to spin.
We can avoid this condition by moving printk out of coverage of pers_lock
spinlock.

Signed-off-by: Chao Yu <chao2.yu@samsung.com>
---
 drivers/md/md.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index e02de05..5fcf215 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -7200,9 +7200,10 @@ static const struct file_operations md_seq_fops = {
 
 int register_md_personality(struct md_personality *p)
 {
+	printk(KERN_INFO "md: %s personality registered for level %d\n",
+						p->name, p->level);
 	spin_lock(&pers_lock);
 	list_add_tail(&p->list, &pers_list);
-	printk(KERN_INFO "md: %s personality registered for level %d\n", p->name, p->level);
 	spin_unlock(&pers_lock);
 	return 0;
 }
-- 
2.0.1.474.g72c7794

Re: [PATCH] md: avoid potential long delay under pers_lock

[Henrique de Moraes Holschuh]

On Thu, 25 Sep 2014, Chao Yu wrote:
> printk may cause long time lapse if value of printk_delay in sysctl is
> configured large by user. If register_md_personality takes long time to print in
> spinlock pers_lock, we may encounter high CPU usage rate when there are other
> pers_lock competitors who may be blocked to spin.
> We can avoid this condition by moving printk out of coverage of pers_lock
> spinlock.
> 
> Signed-off-by: Chao Yu <chao2.yu@samsung.com>
> ---
>  drivers/md/md.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index e02de05..5fcf215 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -7200,9 +7200,10 @@ static const struct file_operations md_seq_fops = {
>  
>  int register_md_personality(struct md_personality *p)
>  {
> +	printk(KERN_INFO "md: %s personality registered for level %d\n",
> +						p->name, p->level);
>  	spin_lock(&pers_lock);
>  	list_add_tail(&p->list, &pers_list);
> -	printk(KERN_INFO "md: %s personality registered for level %d\n", p->name, p->level);
>  	spin_unlock(&pers_lock);
>  	return 0;
>  }

Wouldn't it make more sense to move the printk after the spin_unlock ?

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

RE: [PATCH] md: avoid potential long delay under pers_lock

[Chao Yu]

> -----Original Message-----
> From: Henrique de Moraes Holschuh [mailto:hmh@hmh.eng.br]
> Sent: Friday, September 26, 2014 12:51 AM
> To: Chao Yu
> Cc: neilb@suse.de; linux-raid@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] md: avoid potential long delay under pers_lock
> 
> On Thu, 25 Sep 2014, Chao Yu wrote:
> > printk may cause long time lapse if value of printk_delay in sysctl is
> > configured large by user. If register_md_personality takes long time to print in
> > spinlock pers_lock, we may encounter high CPU usage rate when there are other
> > pers_lock competitors who may be blocked to spin.
> > We can avoid this condition by moving printk out of coverage of pers_lock
> > spinlock.
> >
> > Signed-off-by: Chao Yu <chao2.yu@samsung.com>
> > ---
> >  drivers/md/md.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > index e02de05..5fcf215 100644
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -7200,9 +7200,10 @@ static const struct file_operations md_seq_fops = {
> >
> >  int register_md_personality(struct md_personality *p)
> >  {
> > +	printk(KERN_INFO "md: %s personality registered for level %d\n",
> > +						p->name, p->level);
> >  	spin_lock(&pers_lock);
> >  	list_add_tail(&p->list, &pers_list);
> > -	printk(KERN_INFO "md: %s personality registered for level %d\n", p->name, p->level);
> >  	spin_unlock(&pers_lock);
> >  	return 0;
> >  }
> 
> Wouldn't it make more sense to move the printk after the spin_unlock ?

As original printk msg in {,un}register_md_personality shows '{,un}registered'
which means the main job is done before printk, it's better to move the printk
after spin_unlock in both {,un}register_md_personality. But IMHO, it's another
minor issue, should we fix in this patch?

Thanks,
Yu

> 
> --
>   "One disk to rule them all, One disk to find them. One disk to bring
>   them all and in the darkness grind them. In the Land of Redmond
>   where the shadows lie." -- The Silicon Valley Tarot
>   Henrique Holschuh

Re: [PATCH] md: avoid potential long delay under pers_lock

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1352 bytes --]

On Thu, 25 Sep 2014 15:28:34 +0800 Chao Yu <chao2.yu@samsung.com> wrote:

> printk may cause long time lapse if value of printk_delay in sysctl is
> configured large by user. If register_md_personality takes long time to print in
> spinlock pers_lock, we may encounter high CPU usage rate when there are other
> pers_lock competitors who may be blocked to spin.
> We can avoid this condition by moving printk out of coverage of pers_lock
> spinlock.
> 
> Signed-off-by: Chao Yu <chao2.yu@samsung.com>
> ---
>  drivers/md/md.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index e02de05..5fcf215 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -7200,9 +7200,10 @@ static const struct file_operations md_seq_fops = {
>  
>  int register_md_personality(struct md_personality *p)
>  {
> +	printk(KERN_INFO "md: %s personality registered for level %d\n",
> +						p->name, p->level);
>  	spin_lock(&pers_lock);
>  	list_add_tail(&p->list, &pers_list);
> -	printk(KERN_INFO "md: %s personality registered for level %d\n", p->name, p->level);
>  	spin_unlock(&pers_lock);
>  	return 0;
>  }

I'm not sure I see the pressing need for this - have you noticed actual
problems?
However it seems to make sense so I've applied it.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

RE: [PATCH] md: avoid potential long delay under pers_lock

[Chao Yu]

> -----Original Message-----
> From: NeilBrown [mailto:neilb@suse.de]
> Sent: Friday, September 26, 2014 8:36 AM
> To: Chao Yu
> Cc: linux-raid@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] md: avoid potential long delay under pers_lock
> 
> On Thu, 25 Sep 2014 15:28:34 +0800 Chao Yu <chao2.yu@samsung.com> wrote:
> 
> > printk may cause long time lapse if value of printk_delay in sysctl is
> > configured large by user. If register_md_personality takes long time to print in
> > spinlock pers_lock, we may encounter high CPU usage rate when there are other
> > pers_lock competitors who may be blocked to spin.
> > We can avoid this condition by moving printk out of coverage of pers_lock
> > spinlock.
> >
> > Signed-off-by: Chao Yu <chao2.yu@samsung.com>
> > ---
> >  drivers/md/md.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > index e02de05..5fcf215 100644
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -7200,9 +7200,10 @@ static const struct file_operations md_seq_fops = {
> >
> >  int register_md_personality(struct md_personality *p)
> >  {
> > +	printk(KERN_INFO "md: %s personality registered for level %d\n",
> > +						p->name, p->level);
> >  	spin_lock(&pers_lock);
> >  	list_add_tail(&p->list, &pers_list);
> > -	printk(KERN_INFO "md: %s personality registered for level %d\n", p->name, p->level);
> >  	spin_unlock(&pers_lock);
> >  	return 0;
> >  }
> 
> I'm not sure I see the pressing need for this - have you noticed actual
> problems?

Haven't, I found this by code review.

Thanks,
Yu

> However it seems to make sense so I've applied it.
> 
> Thanks,
> NeilBrown