[PATCH] mm, vmstat: Use cond_resched only when !CONFIG_PREEMPT

[PATCH] mm, vmstat: Use cond_resched only when !CONFIG_PREEMPT

[Rakib Mullick]

commit 468fd62ed9 (vmstats: add cond_resched() to refresh_cpu_vm_stats()) added cond_resched() in refresh_cpu_vm_stats. Purpose of that patch was to allow other threads to run in non-preemptive case. This patch, makes sure that cond_resched() gets called when !CONFIG_PREEMPT is set. In a preemptiable kernel we don't need to call cond_resched().

Signed-off-by: Rakib Mullick <rakib.mullick@gmail.com>
---

diff --git a/mm/vmstat.c b/mm/vmstat.c
index 20c18b7..72cf857 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -461,7 +461,11 @@ void refresh_cpu_vm_stats(int cpu)
 				p->expire = 3;
 #endif
 			}
+
+#ifndef CONFIG_PREEMPT
 		cond_resched();
+#endif
+
 #ifdef CONFIG_NUMA
 		/*
 		 * Deal with draining the remote pageset of this


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm, vmstat: Use cond_resched only when !CONFIG_PREEMPT

[KAMEZAWA Hiroyuki]

On Mon, 30 May 2011 22:59:04 +0600
Rakib Mullick <rakib.mullick@gmail.com> wrote:

> commit 468fd62ed9 (vmstats: add cond_resched() to refresh_cpu_vm_stats()) added cond_resched() in refresh_cpu_vm_stats. Purpose of that patch was to allow other threads to run in non-preemptive case. This patch, makes sure that cond_resched() gets called when !CONFIG_PREEMPT is set. In a preemptiable kernel we don't need to call cond_resched().
> 
> Signed-off-by: Rakib Mullick <rakib.mullick@gmail.com>

Hmm, what benefit do we get by adding this extra #ifdef in the code directly ?
Other cond_resched() callers are not guilty in !CONFIG_PREEMPT ?

Thanks,
-Kame

> ---
> 
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 20c18b7..72cf857 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -461,7 +461,11 @@ void refresh_cpu_vm_stats(int cpu)
>  				p->expire = 3;
>  #endif
>  			}
> +
> +#ifndef CONFIG_PREEMPT
>  		cond_resched();
> +#endif
> +
>  #ifdef CONFIG_NUMA
>  		/*
>  		 * Deal with draining the remote pageset of this
> 
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm, vmstat: Use cond_resched only when !CONFIG_PREEMPT

[KOSAKI Motohiro]

> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 20c18b7..72cf857 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -461,7 +461,11 @@ void refresh_cpu_vm_stats(int cpu)
>  				p->expire = 3;
>  #endif
>  			}
> +
> +#ifndef CONFIG_PREEMPT
>  		cond_resched();
> +#endif
> +

In general, we should avoid #ifdef CONFIG_PREEMPT for maintainancebility as far as possible.
Is there any observable benefit? Can you please demonstrate it?



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm, vmstat: Use cond_resched only when !CONFIG_PREEMPT

[Rakib Mullick]

On Tue, May 31, 2011 at 5:38 AM, KAMEZAWA Hiroyuki
<kamezawa.hiroyu@jp.fujitsu.com> wrote:
> On Mon, 30 May 2011 22:59:04 +0600
> Rakib Mullick <rakib.mullick@gmail.com> wrote:
>
>> commit 468fd62ed9 (vmstats: add cond_resched() to refresh_cpu_vm_stats()) added cond_resched() in refresh_cpu_vm_stats. Purpose of that patch was to allow other threads to run in non-preemptive case. This patch, makes sure that cond_resched() gets called when !CONFIG_PREEMPT is set. In a preemptiable kernel we don't need to call cond_resched().
>>
>> Signed-off-by: Rakib Mullick <rakib.mullick@gmail.com>
>
> Hmm, what benefit do we get by adding this extra #ifdef in the code directly ?
> Other cond_resched() callers are not guilty in !CONFIG_PREEMPT ?
>
Well, in preemptible kernel this context will get preempted if
requires, so we don't need cond_resched(). If you checkout the git log
of the mentioned commit, you'll find the explanation. It says:
        "Adding a cond_resched() to allow other threads to run in the
non-preemptive
    case."

So, let cond_resched() be in non-preemptive case.

Thanks,
Rakib

> Thanks,
> -Kame
>
>> ---
>>
>> diff --git a/mm/vmstat.c b/mm/vmstat.c
>> index 20c18b7..72cf857 100644
>> --- a/mm/vmstat.c
>> +++ b/mm/vmstat.c
>> @@ -461,7 +461,11 @@ void refresh_cpu_vm_stats(int cpu)
>>                               p->expire = 3;
>>  #endif
>>                       }
>> +
>> +#ifndef CONFIG_PREEMPT
>>               cond_resched();
>> +#endif
>> +
>>  #ifdef CONFIG_NUMA
>>               /*
>>                * Deal with draining the remote pageset of this
>>
>>
>
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm, vmstat: Use cond_resched only when !CONFIG_PREEMPT

[KAMEZAWA Hiroyuki]

On Tue, 31 May 2011 09:13:47 +0600
Rakib Mullick <rakib.mullick@gmail.com> wrote:

> On Tue, May 31, 2011 at 5:38 AM, KAMEZAWA Hiroyuki
> <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> > On Mon, 30 May 2011 22:59:04 +0600
> > Rakib Mullick <rakib.mullick@gmail.com> wrote:
> >
> >> commit 468fd62ed9 (vmstats: add cond_resched() to refresh_cpu_vm_stats()) added cond_resched() in refresh_cpu_vm_stats. Purpose of that patch was to allow other threads to run in non-preemptive case. This patch, makes sure that cond_resched() gets called when !CONFIG_PREEMPT is set. In a preemptiable kernel we don't need to call cond_resched().
> >>
> >> Signed-off-by: Rakib Mullick <rakib.mullick@gmail.com>
> >
> > Hmm, what benefit do we get by adding this extra #ifdef in the code directly ?
> > Other cond_resched() callers are not guilty in !CONFIG_PREEMPT ?
> >
> Well, in preemptible kernel this context will get preempted if
> requires, so we don't need cond_resched(). If you checkout the git log
> of the mentioned commit, you'll find the explanation. It says:
>         "Adding a cond_resched() to allow other threads to run in the
> non-preemptive
>     case."
> 

IOW, my question is "why only this cond_resched() should be fixed ?"
What's bad with all cond_resched() in the kernel as no-op in CONFIG_PREEMPT ?

Thanks,
-Kame


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm, vmstat: Use cond_resched only when !CONFIG_PREEMPT

[Rakib Mullick]

On Tue, May 31, 2011 at 8:23 AM, KOSAKI Motohiro
<kosaki.motohiro@jp.fujitsu.com> wrote:
>> diff --git a/mm/vmstat.c b/mm/vmstat.c
>> index 20c18b7..72cf857 100644
>> --- a/mm/vmstat.c
>> +++ b/mm/vmstat.c
>> @@ -461,7 +461,11 @@ void refresh_cpu_vm_stats(int cpu)
>>                               p->expire = 3;
>>  #endif
>>                       }
>> +
>> +#ifndef CONFIG_PREEMPT
>>               cond_resched();
>> +#endif
>> +
>
> In general, we should avoid #ifdef CONFIG_PREEMPT for maintainancebility as far as possible.
> Is there any observable benefit? Can you please demonstrate it?
>
On my system I'm not sure whether it shows demonstratable benefit or
not. Although, I try. It will be helpful if you give me a hint about
how to measure its benefit.

Thanks,
Rakib
>
>
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm, vmstat: Use cond_resched only when !CONFIG_PREEMPT

[Rakib Mullick]

On Tue, May 31, 2011 at 9:18 AM, KAMEZAWA Hiroyuki
<kamezawa.hiroyu@jp.fujitsu.com> wrote:
> On Tue, 31 May 2011 09:13:47 +0600
> Rakib Mullick <rakib.mullick@gmail.com> wrote:
>
>> On Tue, May 31, 2011 at 5:38 AM, KAMEZAWA Hiroyuki
>> <kamezawa.hiroyu@jp.fujitsu.com> wrote:
>> > On Mon, 30 May 2011 22:59:04 +0600
>> > Rakib Mullick <rakib.mullick@gmail.com> wrote:
>> >
>> >> commit 468fd62ed9 (vmstats: add cond_resched() to refresh_cpu_vm_stats()) added cond_resched() in refresh_cpu_vm_stats. Purpose of that patch was to allow other threads to run in non-preemptive case. This patch, makes sure that cond_resched() gets called when !CONFIG_PREEMPT is set. In a preemptiable kernel we don't need to call cond_resched().
>> >>
>> >> Signed-off-by: Rakib Mullick <rakib.mullick@gmail.com>
>> >
>> > Hmm, what benefit do we get by adding this extra #ifdef in the code directly ?
>> > Other cond_resched() callers are not guilty in !CONFIG_PREEMPT ?
>> >
>> Well, in preemptible kernel this context will get preempted if
>> requires, so we don't need cond_resched(). If you checkout the git log
>> of the mentioned commit, you'll find the explanation. It says:
>>         "Adding a cond_resched() to allow other threads to run in the
>> non-preemptive
>>     case."
>>
>
> IOW, my question is "why only this cond_resched() should be fixed ?"

cond_resched() forces this thread to be scheduled. I'm just trying
pointing out the use of cond_resched(), until unless I'm not missing
anything.

> What's bad with all cond_resched() in the kernel as no-op in CONFIG_PREEMPT ?
>
cond_resched() basically checks whether it needs to be scheduled or
not. But, we know in advance that we don't need cond_resched in
CONFIG_PREEMPT.

Thanks,
Rakib

> Thanks,
> -Kame
>
>
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm, vmstat: Use cond_resched only when !CONFIG_PREEMPT

[KAMEZAWA Hiroyuki]

On Tue, 31 May 2011 09:58:40 +0600
Rakib Mullick <rakib.mullick@gmail.com> wrote:

> On Tue, May 31, 2011 at 9:18 AM, KAMEZAWA Hiroyuki
> <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> > On Tue, 31 May 2011 09:13:47 +0600
> > Rakib Mullick <rakib.mullick@gmail.com> wrote:
> >
> >> On Tue, May 31, 2011 at 5:38 AM, KAMEZAWA Hiroyuki
> >> <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> >> > On Mon, 30 May 2011 22:59:04 +0600
> >> > Rakib Mullick <rakib.mullick@gmail.com> wrote:
> >> >
> >> >> commit 468fd62ed9 (vmstats: add cond_resched() to refresh_cpu_vm_stats()) added cond_resched() in refresh_cpu_vm_stats. Purpose of that patch was to allow other threads to run in non-preemptive case. This patch, makes sure that cond_resched() gets called when !CONFIG_PREEMPT is set. In a preemptiable kernel we don't need to call cond_resched().
> >> >>
> >> >> Signed-off-by: Rakib Mullick <rakib.mullick@gmail.com>
> >> >
> >> > Hmm, what benefit do we get by adding this extra #ifdef in the code directly ?
> >> > Other cond_resched() callers are not guilty in !CONFIG_PREEMPT ?
> >> >
> >> Well, in preemptible kernel this context will get preempted if
> >> requires, so we don't need cond_resched(). If you checkout the git log
> >> of the mentioned commit, you'll find the explanation. It says:
> >> A  A  A  A  "Adding a cond_resched() to allow other threads to run in the
> >> non-preemptive
> >> A  A  case."
> >>
> >
> > IOW, my question is "why only this cond_resched() should be fixed ?"
> 
> cond_resched() forces this thread to be scheduled. I'm just trying
> pointing out the use of cond_resched(), until unless I'm not missing
> anything.
> 
> > What's bad with all cond_resched() in the kernel as no-op in CONFIG_PREEMPT ?
> >
> cond_resched() basically checks whether it needs to be scheduled or
> not. But, we know in advance that we don't need cond_resched in
> CONFIG_PREEMPT.
> 
> Thanks,

Then just remove _all_ cond_resched() by defining noop function in header.
Please don't fix in ugly way.

#ifdef CONFIG_PEERMPT
static void cond_resched()
{
}
#endif

Thanks,
-Kame




--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm, vmstat: Use cond_resched only when !CONFIG_PREEMPT

[Minchan Kim]

On Mon, May 30, 2011 at 10:59:04PM +0600, Rakib Mullick wrote:
> commit 468fd62ed9 (vmstats: add cond_resched() to refresh_cpu_vm_stats()) added cond_resched() in refresh_cpu_vm_stats. Purpose of that patch was to allow other threads to run in non-preemptive case. This patch, makes sure that cond_resched() gets called when !CONFIG_PREEMPT is set. In a preemptiable kernel we don't need to call cond_resched().
> 
> Signed-off-by: Rakib Mullick <rakib.mullick@gmail.com>

Let me ask questions.

1. What's bad if we call cond_resched on CONFIG_PREEMPT?
   Is refresh_cpu_vm_stats a hot path?
2. There is no help to call explicit scheduling point on CONFIG_PREEMPTION?
 
We used cond_resched without any ifdef/endif of CONFIG_PREEMPT.
In addtion, cond_resched includes __might_sleep which is debugging help for lock.
So I hope let it be if you have a big concern.

-- 
Kind regards
Meinchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm, vmstat: Use cond_resched only when !CONFIG_PREEMPT

[Minchan Kim]

On Tue, May 31, 2011 at 02:55:28PM +0900, Minchan Kim wrote:
> On Mon, May 30, 2011 at 10:59:04PM +0600, Rakib Mullick wrote:
> > commit 468fd62ed9 (vmstats: add cond_resched() to refresh_cpu_vm_stats()) added cond_resched() in refresh_cpu_vm_stats. Purpose of that patch was to allow other threads to run in non-preemptive case. This patch, makes sure that cond_resched() gets called when !CONFIG_PREEMPT is set. In a preemptiable kernel we don't need to call cond_resched().
> > 
> > Signed-off-by: Rakib Mullick <rakib.mullick@gmail.com>
> 
> Let me ask questions.
> 
> 1. What's bad if we call cond_resched on CONFIG_PREEMPT?
>    Is refresh_cpu_vm_stats a hot path?
> 2. There is no help to call explicit scheduling point on CONFIG_PREEMPTION?
>  
> We used cond_resched without any ifdef/endif of CONFIG_PREEMPT.
> In addtion, cond_resched includes __might_sleep which is debugging help for lock.
> So I hope let it be if you have a big concern.
		typo  ^^
		      unless

-- 
Kind regards
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>