Re: [PATCH 02/25] sched/vtime: Protect idle accounting under vtime seqcount
On Wed, Nov 14, 2018 at 03:45:46AM +0100, Frederic Weisbecker wrote:
> Locking the seqcount on idle vtime accounting wasn't thought to be
> necessary because the readers of idle cputime don't use vtime (yet).
>
> Now updating vtime expect the related seqcount to be locked so do it
> for locking coherency purposes.
>
> Also idle cputime updates use vtime, but idle cputime readers use the
> traditional ad-hoc nohz time delta. We may want to consider moving
> readers to use vtime to consolidate the overall accounting scheme. The
> seqcount will be a functional requirement for it.
>
> Signed-off-by: Frederic Weisbecker
> Cc: Yauheni Kaliuta
> Cc: Thomas Gleixner
> Cc: Rik van Riel
> Cc: Peter Zijlstra
> Cc: Wanpeng Li
> Cc: Ingo Molnar
> ---
> kernel/sched/cputime.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
> index 54eb945..6e74ec2 100644
> --- a/kernel/sched/cputime.c
> +++ b/kernel/sched/cputime.c
> @@ -800,7 +800,11 @@ EXPORT_SYMBOL_GPL(vtime_guest_exit);
>
> void vtime_account_idle(struct task_struct *tsk)
> {
> + struct vtime *vtime = >vtime;
> +
> + write_seqcount_begin(>seqcount);
> account_idle_time(get_vtime_delta(>vtime));
> + write_seqcount_end(>seqcount);
> }
So this makes switching away from idle more expensive ? Also,
vtime_account_system() has this fast-path check in there before taking
that lock, should we not do the same? Or should it be removed from
vtime_account_system() ?
Re: [PATCH 02/25] sched/vtime: Protect idle accounting under vtime seqcount
On Wed, Nov 14, 2018 at 03:45:46AM +0100, Frederic Weisbecker wrote:
> Locking the seqcount on idle vtime accounting wasn't thought to be
> necessary because the readers of idle cputime don't use vtime (yet).
>
> Now updating vtime expect the related seqcount to be locked so do it
> for locking coherency purposes.
>
> Also idle cputime updates use vtime, but idle cputime readers use the
> traditional ad-hoc nohz time delta. We may want to consider moving
> readers to use vtime to consolidate the overall accounting scheme. The
> seqcount will be a functional requirement for it.
>
> Signed-off-by: Frederic Weisbecker
> Cc: Yauheni Kaliuta
> Cc: Thomas Gleixner
> Cc: Rik van Riel
> Cc: Peter Zijlstra
> Cc: Wanpeng Li
> Cc: Ingo Molnar
> ---
> kernel/sched/cputime.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
> index 54eb945..6e74ec2 100644
> --- a/kernel/sched/cputime.c
> +++ b/kernel/sched/cputime.c
> @@ -800,7 +800,11 @@ EXPORT_SYMBOL_GPL(vtime_guest_exit);
>
> void vtime_account_idle(struct task_struct *tsk)
> {
> + struct vtime *vtime = >vtime;
> +
> + write_seqcount_begin(>seqcount);
> account_idle_time(get_vtime_delta(>vtime));
> + write_seqcount_end(>seqcount);
> }
So this makes switching away from idle more expensive ? Also,
vtime_account_system() has this fast-path check in there before taking
that lock, should we not do the same? Or should it be removed from
vtime_account_system() ?
[PATCH 02/25] sched/vtime: Protect idle accounting under vtime seqcount
Locking the seqcount on idle vtime accounting wasn't thought to be
necessary because the readers of idle cputime don't use vtime (yet).
Now updating vtime expect the related seqcount to be locked so do it
for locking coherency purposes.
Also idle cputime updates use vtime, but idle cputime readers use the
traditional ad-hoc nohz time delta. We may want to consider moving
readers to use vtime to consolidate the overall accounting scheme. The
seqcount will be a functional requirement for it.
Signed-off-by: Frederic Weisbecker
Cc: Yauheni Kaliuta
Cc: Thomas Gleixner
Cc: Rik van Riel
Cc: Peter Zijlstra
Cc: Wanpeng Li
Cc: Ingo Molnar
---
kernel/sched/cputime.c | 4
1 file changed, 4 insertions(+)
diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
index 54eb945..6e74ec2 100644
--- a/kernel/sched/cputime.c
+++ b/kernel/sched/cputime.c
@@ -800,7 +800,11 @@ EXPORT_SYMBOL_GPL(vtime_guest_exit);
void vtime_account_idle(struct task_struct *tsk)
{
+ struct vtime *vtime = >vtime;
+
+ write_seqcount_begin(>seqcount);
account_idle_time(get_vtime_delta(>vtime));
+ write_seqcount_end(>seqcount);
}
void arch_vtime_task_switch(struct task_struct *prev)
--
2.7.4
[PATCH 02/25] sched/vtime: Protect idle accounting under vtime seqcount
Locking the seqcount on idle vtime accounting wasn't thought to be
necessary because the readers of idle cputime don't use vtime (yet).
Now updating vtime expect the related seqcount to be locked so do it
for locking coherency purposes.
Also idle cputime updates use vtime, but idle cputime readers use the
traditional ad-hoc nohz time delta. We may want to consider moving
readers to use vtime to consolidate the overall accounting scheme. The
seqcount will be a functional requirement for it.
Signed-off-by: Frederic Weisbecker
Cc: Yauheni Kaliuta
Cc: Thomas Gleixner
Cc: Rik van Riel
Cc: Peter Zijlstra
Cc: Wanpeng Li
Cc: Ingo Molnar
---
kernel/sched/cputime.c | 4
1 file changed, 4 insertions(+)
diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
index 54eb945..6e74ec2 100644
--- a/kernel/sched/cputime.c
+++ b/kernel/sched/cputime.c
@@ -800,7 +800,11 @@ EXPORT_SYMBOL_GPL(vtime_guest_exit);
void vtime_account_idle(struct task_struct *tsk)
{
+ struct vtime *vtime = >vtime;
+
+ write_seqcount_begin(>seqcount);
account_idle_time(get_vtime_delta(>vtime));
+ write_seqcount_end(>seqcount);
}
void arch_vtime_task_switch(struct task_struct *prev)
--
2.7.4
