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 <[email protected]>
---
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
On Mon, 30 May 2011 22:59:04 +0600
Rakib Mullick <[email protected]> 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 <[email protected]>
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
>
>
> 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 Tue, May 31, 2011 at 5:38 AM, KAMEZAWA Hiroyuki
<[email protected]> wrote:
> On Mon, 30 May 2011 22:59:04 +0600
> Rakib Mullick <[email protected]> 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 <[email protected]>
>
> 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
>>
>>
>
>
On Tue, 31 May 2011 09:13:47 +0600
Rakib Mullick <[email protected]> wrote:
> On Tue, May 31, 2011 at 5:38 AM, KAMEZAWA Hiroyuki
> <[email protected]> wrote:
> > On Mon, 30 May 2011 22:59:04 +0600
> > Rakib Mullick <[email protected]> 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 <[email protected]>
> >
> > 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
On Tue, May 31, 2011 at 8:23 AM, KOSAKI Motohiro
<[email protected]> 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
>
>
>
On Tue, May 31, 2011 at 9:18 AM, KAMEZAWA Hiroyuki
<[email protected]> wrote:
> On Tue, 31 May 2011 09:13:47 +0600
> Rakib Mullick <[email protected]> wrote:
>
>> On Tue, May 31, 2011 at 5:38 AM, KAMEZAWA Hiroyuki
>> <[email protected]> wrote:
>> > On Mon, 30 May 2011 22:59:04 +0600
>> > Rakib Mullick <[email protected]> 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 <[email protected]>
>> >
>> > 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
>
>
>
On Tue, 31 May 2011 09:58:40 +0600
Rakib Mullick <[email protected]> wrote:
> On Tue, May 31, 2011 at 9:18 AM, KAMEZAWA Hiroyuki
> <[email protected]> wrote:
> > On Tue, 31 May 2011 09:13:47 +0600
> > Rakib Mullick <[email protected]> wrote:
> >
> >> On Tue, May 31, 2011 at 5:38 AM, KAMEZAWA Hiroyuki
> >> <[email protected]> wrote:
> >> > On Mon, 30 May 2011 22:59:04 +0600
> >> > Rakib Mullick <[email protected]> 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 <[email protected]>
> >> >
> >> > 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,
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
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 <[email protected]>
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
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 <[email protected]>
>
> 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