Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1313255 > unrolled thread

Re: mm, vmstat: kernel BUG at mm/vmstat.c:1408!

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: mm, vmstat: kernel BUG at mm/vmstat.c:1408! Michal Hocko <mhocko@kernel.org> - 2016-01-20 15:40 +0100
    Re: mm, vmstat: kernel BUG at mm/vmstat.c:1408! Sasha Levin <sasha.levin@oracle.com> - 2016-01-20 16:00 +0100
      Re: mm, vmstat: kernel BUG at mm/vmstat.c:1408! Michal Hocko <mhocko@kernel.org> - 2016-01-20 16:20 +0100
        Re: mm, vmstat: kernel BUG at mm/vmstat.c:1408! Christoph Lameter <cl@linux.com> - 2016-01-20 16:30 +0100
          Re: mm, vmstat: kernel BUG at mm/vmstat.c:1408! Sasha Levin <sasha.levin@oracle.com> - 2016-01-20 17:00 +0100
            Re: mm, vmstat: kernel BUG at mm/vmstat.c:1408! Christoph Lameter <cl@linux.com> - 2016-01-20 17:00 +0100
              Re: mm, vmstat: kernel BUG at mm/vmstat.c:1408! Michal Hocko <mhocko@kernel.org> - 2016-01-20 22:30 +0100
                Re: mm, vmstat: kernel BUG at mm/vmstat.c:1408! Christoph Lameter <cl@linux.com> - 2016-01-20 23:00 +0100
                  Re: mm, vmstat: kernel BUG at mm/vmstat.c:1408! Michal Hocko <mhocko@kernel.org> - 2016-01-21 09:30 +0100
                    Re: mm, vmstat: kernel BUG at mm/vmstat.c:1408! Christoph Lameter <cl@linux.com> - 2016-01-21 16:50 +0100
                      Re: mm, vmstat: kernel BUG at mm/vmstat.c:1408! Michal Hocko <mhocko@kernel.org> - 2016-01-21 18:00 +0100
                        Re: mm, vmstat: kernel BUG at mm/vmstat.c:1408! Christoph Lameter <cl@linux.com> - 2016-01-21 18:40 +0100
                          Re: mm, vmstat: kernel BUG at mm/vmstat.c:1408! Shiraz Hashim <shiraz.linux.kernel@gmail.com> - 2016-01-22 12:10 +0100
                          Re: mm, vmstat: kernel BUG at mm/vmstat.c:1408! Michal Hocko <mhocko@kernel.org> - 2016-01-22 15:10 +0100
                            Re: mm, vmstat: kernel BUG at mm/vmstat.c:1408! Christoph Lameter <cl@linux.com> - 2016-01-22 17:10 +0100
                              Re: mm, vmstat: kernel BUG at mm/vmstat.c:1408! Michal Hocko <mhocko@kernel.org> - 2016-01-22 17:20 +0100
                                Re: mm, vmstat: kernel BUG at mm/vmstat.c:1408! Christoph Lameter <cl@linux.com> - 2016-01-22 17:50 +0100
                                  Re: mm, vmstat: kernel BUG at mm/vmstat.c:1408! Michal Hocko <mhocko@kernel.org> - 2016-01-22 18:20 +0100
                                  fast path cycle muncher (vmstat: make vmstat_updater deferrable  again and shut down on idle) Mike Galbraith <umgwanakikbuti@gmail.com> - 2016-01-23 17:30 +0100
                                    Re: fast path cycle muncher (vmstat: make vmstat_updater deferrable  again and shut down on idle) Christoph Lameter <cl@linux.com> - 2016-01-24 01:40 +0100
                                      Re: fast path cycle muncher (vmstat: make vmstat_updater deferrable  again and shut down on idle) Mike Galbraith <umgwanakikbuti@gmail.com> - 2016-01-24 03:50 +0100
                                        Re: fast path cycle muncher (vmstat: make vmstat_updater deferrable  again and shut down on idle) Christoph Lameter <cl@linux.com> - 2016-01-24 04:50 +0100
                                          Re: fast path cycle muncher (vmstat: make vmstat_updater deferrable  again and shut down on idle) Mike Galbraith <umgwanakikbuti@gmail.com> - 2016-01-24 06:40 +0100
                                    Re: fast path cycle muncher (vmstat: make vmstat_updater deferrable  again and shut down on idle) Michal Hocko <mhocko@kernel.org> - 2016-01-25 18:50 +0100
                                      Re: fast path cycle muncher (vmstat: make vmstat_updater deferrable  again and shut down on idle) Christoph Lameter <cl@linux.com> - 2016-01-25 19:10 +0100
                                        Re: fast path cycle muncher (vmstat: make vmstat_updater deferrable  again and shut down on idle) Michal Hocko <mhocko@kernel.org> - 2016-01-25 21:20 +0100
                                          Re: fast path cycle muncher (vmstat: make vmstat_updater deferrable  again and shut down on idle) Christoph Lameter <cl@linux.com> - 2016-01-26 17:30 +0100
                                            Re: fast path cycle muncher (vmstat: make vmstat_updater deferrable  again and shut down on idle) Christoph Lameter <cl@linux.com> - 2016-01-26 19:40 +0100
                                              Re: fast path cycle muncher (vmstat: make vmstat_updater deferrable  again and shut down on idle) Mike Galbraith <umgwanakikbuti@gmail.com> - 2016-01-26 19:50 +0100
                                                Re: fast path cycle muncher (vmstat: make vmstat_updater deferrable  again and shut down on idle) Christoph Lameter <cl@linux.com> - 2016-01-26 20:30 +0100
                                                  Re: fast path cycle muncher (vmstat: make vmstat_updater deferrable  again and shut down on idle) Mike Galbraith <umgwanakikbuti@gmail.com> - 2016-01-27 04:20 +0100
                                                    Re: fast path cycle muncher (vmstat: make vmstat_updater deferrable  again and shut down on idle) Mike Galbraith <umgwanakikbuti@gmail.com> - 2016-01-27 05:20 +0100
                                                    Re: fast path cycle muncher (vmstat: make vmstat_updater deferrable  again and shut down on idle) Christoph Lameter <cl@linux.com> - 2016-01-27 17:30 +0100
                                            Re: fast path cycle muncher (vmstat: make vmstat_updater deferrable  again and shut down on idle) Mike Galbraith <umgwanakikbuti@gmail.com> - 2016-01-26 19:40 +0100
                                        Re: fast path cycle muncher (vmstat: make vmstat_updater deferrable  again and shut down on idle) Mike Galbraith <umgwanakikbuti@gmail.com> - 2016-01-26 03:20 +0100
                                          Re: fast path cycle muncher (vmstat: make vmstat_updater deferrable  again and shut down on idle) Mike Galbraith <umgwanakikbuti@gmail.com> - 2016-01-26 03:30 +0100
                                            Re: fast path cycle muncher (vmstat: make vmstat_updater deferrable  again and shut down on idle) Christoph Lameter <cl@linux.com> - 2016-01-26 17:30 +0100
                                              Re: fast path cycle muncher (vmstat: make vmstat_updater deferrable  again and shut down on idle) Mike Galbraith <umgwanakikbuti@gmail.com> - 2016-01-26 18:40 +0100
                                                Re: fast path cycle muncher (vmstat: make vmstat_updater deferrable  again and shut down on idle) Christoph Lameter <cl@linux.com> - 2016-01-26 19:20 +0100
                                          Re: fast path cycle muncher (vmstat: make vmstat_updater deferrable  again and shut down on idle) Christoph Lameter <cl@linux.com> - 2016-01-26 17:30 +0100
                                            Re: fast path cycle muncher (vmstat: make vmstat_updater deferrable  again and shut down on idle) Mike Galbraith <umgwanakikbuti@gmail.com> - 2016-01-26 18:10 +0100
                                              Re: fast path cycle muncher (vmstat: make vmstat_updater deferrable  again and shut down on idle) Christoph Lameter <cl@linux.com> - 2016-01-26 19:30 +0100
                                                Re: fast path cycle muncher (vmstat: make vmstat_updater deferrable  again and shut down on idle) Mike Galbraith <umgwanakikbuti@gmail.com> - 2016-01-26 20:10 +0100
                                                  Re: fast path cycle muncher (vmstat: make vmstat_updater deferrable  again and shut down on idle) Christoph Lameter <cl@linux.com> - 2016-01-26 20:30 +0100
                                    [PATCH] mm, vmstat: make quiet_vmstat lighter (was: Re: fast path  cycle muncher (vmstat: make vmstat_updater deferrable) again and shut down  on idle) Michal Hocko <mhocko@kernel.org> - 2016-01-27 17:50 +0100
                                      Re: [PATCH] mm, vmstat: make quiet_vmstat lighter (was: Re: fast  path cycle muncher (vmstat: make vmstat_updater deferrable) again and shut  down on idle) Mike Galbraith <umgwanakikbuti@gmail.com> - 2016-01-27 18:10 +0100
                                      Re: [PATCH] mm, vmstat: make quiet_vmstat lighter (was: Re: fast  path cycle muncher (vmstat: make vmstat_updater deferrable) again and shut  down on idle) Christoph Lameter <cl@linux.com> - 2016-01-27 19:30 +0100
                                  Re: mm, vmstat: kernel BUG at mm/vmstat.c:1408! Linus Torvalds <torvalds@linux-foundation.org> - 2016-01-24 18:00 +0100
    Re: mm, vmstat: kernel BUG at mm/vmstat.c:1408! Christoph Lameter <cl@linux.com> - 2016-01-20 16:20 +0100
      Re: mm, vmstat: kernel BUG at mm/vmstat.c:1408! Michal Hocko <mhocko@kernel.org> - 2016-01-20 16:30 +0100

Page 1 of 3  [1] 2 3  Next page →

#1313255 — Re: mm, vmstat: kernel BUG at mm/vmstat.c:1408!

[CCing Andrew]

I am just reading through this old discussion again because "vmstat:
make vmstat_updater deferrable again and shut down on idle" which seems
to be the culprit AFAIU has been merged as 0eb77e988032 and I do not see
any follow up fix merged to linus tree

On Fri 18-12-15 19:33:07, Sasha Levin wrote:
> Hi all,
> 
> I've started seeing the following in the latest -next kernel.
> 
> [  531.127489] kernel BUG at mm/vmstat.c:1408!
> [  531.128157] invalid opcode: 0000 [#1] PREEMPT SMP KASAN
> [  531.128872] Modules linked in:
> [  531.129324] CPU: 6 PID: 407 Comm: kworker/6:1 Not tainted 4.4.0-rc5-next-20151218-sasha-00021-gaba8d84-dirty #2750
> [  531.130939] Workqueue: vmstat vmstat_update
> [  531.131741] task: ffff880204070000 ti: ffff880204078000 task.ti: ffff880204078000
> [  531.133189] RIP: vmstat_update (mm/vmstat.c:1408)
> [  531.134466] RSP: 0018:ffff88020407fbf8  EFLAGS: 00010293
> [  531.135132] RAX: 0000000000000006 RBX: ffff8800418e2fd8 RCX: 0000000000000000
> [  531.135995] RDX: 0000000000000007 RSI: ffffffff8c0982a0 RDI: ffffffff9b8bd6e4
> [  531.137475] RBP: ffff88020407fc18 R08: 0000000000000000 R09: ffff880204070230
> [  531.138304] R10: ffffffff8c0982a0 R11: 00000000e272b4f2 R12: ffff880204c1bf60
> [  531.139329] R13: ffff880204ab09c8 R14: ffff880204ab09b8 R15: ffff880204ab09b0
> [  531.140261] FS:  0000000000000000(0000) GS:ffff880204c00000(0000) knlGS:0000000000000000
> [  531.141218] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> [  531.142036] CR2: 00007f039a8c1944 CR3: 000000000ea28000 CR4: 00000000000006a0
> [  531.142752] Stack:
> [  531.142963]  ffff880204c21000 ffff880204c21000 ffff880204c1bf60 ffff880204ab09c8
> [  531.144095]  ffff88020407fd40 ffffffff813a8fea 0000000041b58ab3 ffffffff8e667cdb
> [  531.145258]  ffff880204ab09f8 ffff880204c1bf68 ffff880204ab09c0 ffff880200000000
> [  531.146475] Call Trace:
> [  531.147037] process_one_work (./arch/x86/include/asm/preempt.h:22 kernel/workqueue.c:2045)
> [  531.150790] worker_thread (include/linux/compiler.h:218 include/linux/list.h:206 kernel/workqueue.c:2171)
> [  531.155176] kthread (kernel/kthread.c:209)
> [  531.158941] ret_from_fork (arch/x86/entry/entry_64.S:469)
> [ 531.160654] Code: 75 1e be 79 00 00 00 48 c7 c7 80 0f 10 8c 89 45 e4 e8 cd 92 cd ff 8b 45 e4 c6 05 e1 c4 13 1a 01 89 c0 f0 48 0f ab 03 72 02 eb 0e <0f> 0b 48 c7 c7 c0 f1 47 90 e8 3d 03 ae 01 48 83 c4 08 5b 41 5c
> All code
> ========
>    0:   75 1e                   jne    0x20
>    2:   be 79 00 00 00          mov    $0x79,%esi
>    7:   48 c7 c7 80 0f 10 8c    mov    $0xffffffff8c100f80,%rdi
>    e:   89 45 e4                mov    %eax,-0x1c(%rbp)
>   11:   e8 cd 92 cd ff          callq  0xffffffffffcd92e3
>   16:   8b 45 e4                mov    -0x1c(%rbp),%eax
>   19:   c6 05 e1 c4 13 1a 01    movb   $0x1,0x1a13c4e1(%rip)        # 0x1a13c501
>   20:   89 c0                   mov    %eax,%eax
>   22:   f0 48 0f ab 03          lock bts %rax,(%rbx)
>   27:   72 02                   jb     0x2b
>   29:   eb 0e                   jmp    0x39
>   2b:*  0f 0b                   ud2             <-- trapping instruction
>   2d:   48 c7 c7 c0 f1 47 90    mov    $0xffffffff9047f1c0,%rdi
>   34:   e8 3d 03 ae 01          callq  0x1ae0376
>   39:   48 83 c4 08             add    $0x8,%rsp
>   3d:   5b                      pop    %rbx
>   3e:   41 5c                   pop    %r12
>         ...
> 
> Code starting with the faulting instruction
> ===========================================
>    0:   0f 0b                   ud2
>    2:   48 c7 c7 c0 f1 47 90    mov    $0xffffffff9047f1c0,%rdi
>    9:   e8 3d 03 ae 01          callq  0x1ae034b
>    e:   48 83 c4 08             add    $0x8,%rsp
>   12:   5b                      pop    %rbx
>   13:   41 5c                   pop    %r12
>         ...
> [  531.164630] RIP vmstat_update (mm/vmstat.c:1408)
> [  531.165523]  RSP <ffff88020407fbf8>
> 
> 
> Thanks,
> Sasha

-- 
Michal Hocko
SUSE Labs

[toc] | [next] | [standalone]

#1313272

On 01/20/2016 09:37 AM, Michal Hocko wrote:
> I am just reading through this old discussion again because "vmstat:
> make vmstat_updater deferrable again and shut down on idle" which seems
> to be the culprit AFAIU has been merged as 0eb77e988032 and I do not see
> any follow up fix merged to linus tree

So this isn't an "old" discussion - the bug is very much there and I can
hit it easily. As a workaround I've "disabled" vmstat.


Thanks,
Sasha

[toc] | [prev] | [next] | [standalone]

#1313288

On Wed 20-01-16 09:56:26, Sasha Levin wrote:
> On 01/20/2016 09:37 AM, Michal Hocko wrote:
> > I am just reading through this old discussion again because "vmstat:
> > make vmstat_updater deferrable again and shut down on idle" which seems
> > to be the culprit AFAIU has been merged as 0eb77e988032 and I do not see
> > any follow up fix merged to linus tree
> 
> So this isn't an "old" discussion - the bug is very much there and I can
> hit it easily. As a workaround I've "disabled" vmstat.

Well the report is since 18th Dec which is over month old. Should we
revert 0eb77e988032 as a pre caution and make sure this is done properly
in -mm tree. AFAIR none of the proposed fix worked without other
fallouts?
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1313298

On Wed, 20 Jan 2016, Michal Hocko wrote:

> On Wed 20-01-16 09:56:26, Sasha Levin wrote:
> > On 01/20/2016 09:37 AM, Michal Hocko wrote:
> > > I am just reading through this old discussion again because "vmstat:
> > > make vmstat_updater deferrable again and shut down on idle" which seems
> > > to be the culprit AFAIU has been merged as 0eb77e988032 and I do not see
> > > any follow up fix merged to linus tree
> >
> > So this isn't an "old" discussion - the bug is very much there and I can
> > hit it easily. As a workaround I've "disabled" vmstat.
>
> Well the report is since 18th Dec which is over month old. Should we
> revert 0eb77e988032 as a pre caution and make sure this is done properly
> in -mm tree. AFAIR none of the proposed fix worked without other
> fallouts?

Seems that we are unable to get enough information to reproduce the issue?

[toc] | [prev] | [next] | [standalone]

#1313311

On 01/20/2016 10:20 AM, Christoph Lameter wrote:
> On Wed, 20 Jan 2016, Michal Hocko wrote:
> 
>> > On Wed 20-01-16 09:56:26, Sasha Levin wrote:
>>> > > On 01/20/2016 09:37 AM, Michal Hocko wrote:
>>>> > > > I am just reading through this old discussion again because "vmstat:
>>>> > > > make vmstat_updater deferrable again and shut down on idle" which seems
>>>> > > > to be the culprit AFAIU has been merged as 0eb77e988032 and I do not see
>>>> > > > any follow up fix merged to linus tree
>>> > >
>>> > > So this isn't an "old" discussion - the bug is very much there and I can
>>> > > hit it easily. As a workaround I've "disabled" vmstat.
>> >
>> > Well the report is since 18th Dec which is over month old. Should we
>> > revert 0eb77e988032 as a pre caution and make sure this is done properly
>> > in -mm tree. AFAIR none of the proposed fix worked without other
>> > fallouts?
> Seems that we are unable to get enough information to reproduce the issue?

As I've mentioned - this reproduces frequently. I'd be happy to add in debug
information into the kernel that might help you reproduce it, but as it seems
like a timing issue, I can't provide a simple reproducer.


Thanks,
Sasha

[toc] | [prev] | [next] | [standalone]

#1313313

On Wed, 20 Jan 2016, Sasha Levin wrote:

>
> As I've mentioned - this reproduces frequently. I'd be happy to add in debug
> information into the kernel that might help you reproduce it, but as it seems
> like a timing issue, I can't provide a simple reproducer.

This isnt really important I think. Lets remove it.


Subject: vmstat: Remove BUG_ON from vmstat_update

If we detect that there is nothing to do just set the flag and do not check
if it was already set before. Races really do not matter. If the flag is
set by any code then the shepherd will start dealing with the situation
and reenable the vmstat workers when necessary again.

Concurrent actions could be onlining and offlining of processors or be a
result of concurrency issues when updating the cpumask from multiple
processors.

Signed-off-by: Christoph Lameter <cl@linux.com>

Index: linux/mm/vmstat.c
===================================================================
--- linux.orig/mm/vmstat.c
+++ linux/mm/vmstat.c
@@ -1408,17 +1408,7 @@ static void vmstat_update(struct work_st
 		 * Defer the checking for differentials to the
 		 * shepherd thread on a different processor.
 		 */
-		int r;
-		/*
-		 * Shepherd work thread does not race since it never
-		 * changes the bit if its zero but the cpu
-		 * online / off line code may race if
-		 * worker threads are still allowed during
-		 * shutdown / startup.
-		 */
-		r = cpumask_test_and_set_cpu(smp_processor_id(),
-			cpu_stat_off);
-		VM_BUG_ON(r);
+		cpumask_set_cpu(smp_processor_id(), cpu_stat_off);
 	}
 }

[toc] | [prev] | [next] | [standalone]

#1313530

On Wed 20-01-16 09:55:22, Christoph Lameter wrote:
[...]
> Subject: vmstat: Remove BUG_ON from vmstat_update
> 
> If we detect that there is nothing to do just set the flag and do not check
> if it was already set before. Races really do not matter. If the flag is
> set by any code then the shepherd will start dealing with the situation
> and reenable the vmstat workers when necessary again.
> 
> Concurrent actions could be onlining and offlining of processors or be a
> result of concurrency issues when updating the cpumask from multiple
> processors.

Now that 7e988032 ("vmstat: make vmstat_updater deferrable again and
shut down on idle) is merged the VM_BUG_ON is simply bogus because
vmstat_update might "race" with quiet_vmstat. The changelog should
reflect that. What about the following wording?

"
Since 0eb77e988032 ("vmstat: make vmstat_updater deferrable again and
shut down on idle") quiet_vmstat might update cpu_stat_off and mark a
particular cpu to be handled by vmstat_shepherd. This might trigger
a VM_BUG_ON in vmstat_update because the work item might have been
sleeping during the idle period and see the cpu_stat_off updated after
the wake up. The VM_BUG_ON is therefore misleading and no more
appropriate. Moreover it doesn't really suite any protection from real
bugs because vmstat_shepherd will simply reschedule the vmstat_work
anytime it sees a particular cpu set or vmstat_update would do the same
from the worker context directly. Even when the two would race the
result wouldn't be incorrect as the counters update is fully idempotent.

Fixes: 0eb77e988032 ("vmstat: make vmstat_updater deferrable again and
shut down on idle")
CC: stable # 4.4+
"

> Signed-off-by: Christoph Lameter <cl@linux.com>
> 
> Index: linux/mm/vmstat.c
> ===================================================================
> --- linux.orig/mm/vmstat.c
> +++ linux/mm/vmstat.c
> @@ -1408,17 +1408,7 @@ static void vmstat_update(struct work_st
>  		 * Defer the checking for differentials to the
>  		 * shepherd thread on a different processor.
>  		 */
> -		int r;
> -		/*
> -		 * Shepherd work thread does not race since it never
> -		 * changes the bit if its zero but the cpu
> -		 * online / off line code may race if
> -		 * worker threads are still allowed during
> -		 * shutdown / startup.
> -		 */
> -		r = cpumask_test_and_set_cpu(smp_processor_id(),
> -			cpu_stat_off);
> -		VM_BUG_ON(r);
> +		cpumask_set_cpu(smp_processor_id(), cpu_stat_off);
>  	}
>  }

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1313540

On Wed, 20 Jan 2016, Michal Hocko wrote:

> On Wed 20-01-16 09:55:22, Christoph Lameter wrote:
> [...]
> > Subject: vmstat: Remove BUG_ON from vmstat_update
> >
> > If we detect that there is nothing to do just set the flag and do not check
> > if it was already set before. Races really do not matter. If the flag is
> > set by any code then the shepherd will start dealing with the situation
> > and reenable the vmstat workers when necessary again.
> >
> > Concurrent actions could be onlining and offlining of processors or be a
> > result of concurrency issues when updating the cpumask from multiple
> > processors.
>
> Now that 7e988032 ("vmstat: make vmstat_updater deferrable again and
> shut down on idle) is merged the VM_BUG_ON is simply bogus because
> vmstat_update might "race" with quiet_vmstat. The changelog should
> reflect that. What about the following wording?

How can it race if preemption is off?

> Since 0eb77e988032 ("vmstat: make vmstat_updater deferrable again and
> shut down on idle") quiet_vmstat might update cpu_stat_off and mark a
> particular cpu to be handled by vmstat_shepherd. This might trigger
> a VM_BUG_ON in vmstat_update because the work item might have been
> sleeping during the idle period and see the cpu_stat_off updated after
> the wake up. The VM_BUG_ON is therefore misleading and no more
> appropriate. Moreover it doesn't really suite any protection from real
> bugs because vmstat_shepherd will simply reschedule the vmstat_work
> anytime it sees a particular cpu set or vmstat_update would do the same
> from the worker context directly. Even when the two would race the
> result wouldn't be incorrect as the counters update is fully idempotent.


Hmmm... the vmstat_update can be interrupted while running and the cpu put
into idle mode? If vmstat_update is running then the cpu is not idle but
running code. If this is really going on then there is other stuff wrong
with the idling logic.

> Fixes: 0eb77e988032 ("vmstat: make vmstat_updater deferrable again and
> shut down on idle")
> CC: stable # 4.4+

?? There has not been an upstream release with this yet.

[toc] | [prev] | [next] | [standalone]

#1313992

On Wed 20-01-16 15:57:43, Christoph Lameter wrote:
> On Wed, 20 Jan 2016, Michal Hocko wrote:
> 
> > On Wed 20-01-16 09:55:22, Christoph Lameter wrote:
> > [...]
> > > Subject: vmstat: Remove BUG_ON from vmstat_update
> > >
> > > If we detect that there is nothing to do just set the flag and do not check
> > > if it was already set before. Races really do not matter. If the flag is
> > > set by any code then the shepherd will start dealing with the situation
> > > and reenable the vmstat workers when necessary again.
> > >
> > > Concurrent actions could be onlining and offlining of processors or be a
> > > result of concurrency issues when updating the cpumask from multiple
> > > processors.
> >
> > Now that 7e988032 ("vmstat: make vmstat_updater deferrable again and
> > shut down on idle) is merged the VM_BUG_ON is simply bogus because
> > vmstat_update might "race" with quiet_vmstat. The changelog should
> > reflect that. What about the following wording?
> 
> How can it race if preemption is off?

See below.

> > Since 0eb77e988032 ("vmstat: make vmstat_updater deferrable again and
> > shut down on idle") quiet_vmstat might update cpu_stat_off and mark a
> > particular cpu to be handled by vmstat_shepherd. This might trigger
> > a VM_BUG_ON in vmstat_update because the work item might have been
> > sleeping during the idle period and see the cpu_stat_off updated after
> > the wake up. The VM_BUG_ON is therefore misleading and no more
> > appropriate. Moreover it doesn't really suite any protection from real
> > bugs because vmstat_shepherd will simply reschedule the vmstat_work
> > anytime it sees a particular cpu set or vmstat_update would do the same
> > from the worker context directly. Even when the two would race the
> > result wouldn't be incorrect as the counters update is fully idempotent.
> 
> 
> Hmmm... the vmstat_update can be interrupted while running and the cpu put
> into idle mode? If vmstat_update is running then the cpu is not idle but
> running code. If this is really going on then there is other stuff wrong
> with the idling logic.

The vmstat update might be still waiting for its timer, idle mode started
and kick vmstat_update which might cpumask_test_and_set_cpu. Once the
idle terminates and the originally schedule vmstate_update executes it
sees the bit set and BUG_ON.

> > Fixes: 0eb77e988032 ("vmstat: make vmstat_updater deferrable again and
> > shut down on idle")
> > CC: stable # 4.4+
> 
> ?? There has not been an upstream release with this yet.

Ohh, I thought it made it into 4.4 but you are right it is post 4.4 so
no CC: stable required.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1314281

On Thu, 21 Jan 2016, Michal Hocko wrote:

> > > Since 0eb77e988032 ("vmstat: make vmstat_updater deferrable again and
> > > shut down on idle") quiet_vmstat might update cpu_stat_off and mark a
> > > particular cpu to be handled by vmstat_shepherd. This might trigger
> > > a VM_BUG_ON in vmstat_update because the work item might have been
> > > sleeping during the idle period and see the cpu_stat_off updated after
> > > the wake up. The VM_BUG_ON is therefore misleading and no more
> > > appropriate. Moreover it doesn't really suite any protection from real
> > > bugs because vmstat_shepherd will simply reschedule the vmstat_work
> > > anytime it sees a particular cpu set or vmstat_update would do the same
> > > from the worker context directly. Even when the two would race the
> > > result wouldn't be incorrect as the counters update is fully idempotent.
> >
> >
> > Hmmm... the vmstat_update can be interrupted while running and the cpu put
> > into idle mode? If vmstat_update is running then the cpu is not idle but
> > running code. If this is really going on then there is other stuff wrong
> > with the idling logic.
>
> The vmstat update might be still waiting for its timer, idle mode started
> and kick vmstat_update which might cpumask_test_and_set_cpu. Once the
> idle terminates and the originally schedule vmstate_update executes it
> sees the bit set and BUG_ON.

Ok so we are going into idle mode and the vmstat_update timer is pending.
Then the timer will not fire since going idle switches preemption off.
quiet_vmstat will run without the chance of running vmstat_update

We could be going idle and not have disabled preemption yet. Then
vmstat_update will run. On return to the idling operation preemption will
be disabled and quiet_vmstat() will be run.

I do not see how these two things could race.

[toc] | [prev] | [next] | [standalone]

#1314350

On Thu 21-01-16 09:45:12, Christoph Lameter wrote:
> On Thu, 21 Jan 2016, Michal Hocko wrote:
[...]
> > The vmstat update might be still waiting for its timer, idle mode started
> > and kick vmstat_update which might cpumask_test_and_set_cpu. Once the
> > idle terminates and the originally schedule vmstate_update executes it
> > sees the bit set and BUG_ON.
> 
> Ok so we are going into idle mode and the vmstat_update timer is pending.
> Then the timer will not fire since going idle switches preemption off.
> quiet_vmstat will run without the chance of running vmstat_update
> 
> We could be going idle and not have disabled preemption yet. Then
> vmstat_update will run. On return to the idling operation preemption will
> be disabled and quiet_vmstat() will be run.
> 
> I do not see how these two things could race.

It goes like this:
CPU0:						CPU1
vmstat_update
  cpumask_test_and_set_cpu (0->1)
[...]
						vmstat_shepherd
<enter idle>					  cpumask_test_and_clear_cpu(CPU0) (1->0)
quiet_vmstat
  cpumask_test_and_set_cpu (0->1)
  						  queue_delayed_work_on(CPU0)
refresh_cpu_vm_stats()
[...]
vmstat_update
  nothing_to_do
  cpumask_test_and_set_cpu (1->1)
  VM_BUG_ON

Or am I missing something?
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1314373

On Thu, 21 Jan 2016, Michal Hocko wrote:

> It goes like this:
> CPU0:						CPU1
> vmstat_update
>   cpumask_test_and_set_cpu (0->1)
> [...]
> 						vmstat_shepherd
> <enter idle>					  cpumask_test_and_clear_cpu(CPU0) (1->0)
> quiet_vmstat
>   cpumask_test_and_set_cpu (0->1)
>   						  queue_delayed_work_on(CPU0)
> refresh_cpu_vm_stats()
> [...]
> vmstat_update
>   nothing_to_do
>   cpumask_test_and_set_cpu (1->1)
>   VM_BUG_ON
>
> Or am I missing something?

Ok then the following should fix it:



Subject: vmstat: Queue work before clearing cpu_stat_off

There is a race between vmstat_shepherd and quiet_vmstat() because
the responsibility for checking for counter updates changes depending
on the state of teh bit in cpu_stat_off. So queue the work before
changing state of the bit in vmstat_shepherd. That way quiet_vmstat
is guaranteed to remove the work request when clearing the bit and the
bug in vmstat_update wont trigger anymore.

Signed-off-by: Christoph Lameter <cl@linux.com>

Index: linux/mm/vmstat.c
===================================================================
--- linux.orig/mm/vmstat.c
+++ linux/mm/vmstat.c
@@ -1480,12 +1480,14 @@ static void vmstat_shepherd(struct work_
 	get_online_cpus();
 	/* Check processors whose vmstat worker threads have been disabled */
 	for_each_cpu(cpu, cpu_stat_off)
-		if (need_update(cpu) &&
-			cpumask_test_and_clear_cpu(cpu, cpu_stat_off))
+		if (need_update(cpu)) {

 			queue_delayed_work_on(cpu, vmstat_wq,
 				&per_cpu(vmstat_work, cpu), 0);

+			cpumask_clear_cpu(smp_processor_id(), cpu_stat_off);
+		}
+
 	put_online_cpus();

 	schedule_delayed_work(&shepherd,

[toc] | [prev] | [next] | [standalone]

#1314894

On Thu, Jan 21, 2016 at 11:08 PM, Christoph Lameter <cl@linux.com> wrote:
> Subject: vmstat: Queue work before clearing cpu_stat_off
>
> There is a race between vmstat_shepherd and quiet_vmstat() because
> the responsibility for checking for counter updates changes depending
> on the state of teh bit in cpu_stat_off. So queue the work before
> changing state of the bit in vmstat_shepherd. That way quiet_vmstat
> is guaranteed to remove the work request when clearing the bit and the
> bug in vmstat_update wont trigger anymore.
>
> Signed-off-by: Christoph Lameter <cl@linux.com>
>
> Index: linux/mm/vmstat.c
> ===================================================================
> --- linux.orig/mm/vmstat.c
> +++ linux/mm/vmstat.c
> @@ -1480,12 +1480,14 @@ static void vmstat_shepherd(struct work_
>         get_online_cpus();
>         /* Check processors whose vmstat worker threads have been disabled */
>         for_each_cpu(cpu, cpu_stat_off)
> -               if (need_update(cpu) &&
> -                       cpumask_test_and_clear_cpu(cpu, cpu_stat_off))
> +               if (need_update(cpu)) {
>
>                         queue_delayed_work_on(cpu, vmstat_wq,
>                                 &per_cpu(vmstat_work, cpu), 0);
>
> +                       cpumask_clear_cpu(smp_processor_id(), cpu_stat_off);
> +               }
> +
>         put_online_cpus();
>
>         schedule_delayed_work(&shepherd,


This can alternatively lead to following where vmstat may not be
scheduled for cpu  when it is back from idle.

CPU0:                                            CPU1:
                                                       vmstat_shepherd
<enter idle>                                    queue_delayed_work_on(CPU0)
quiet_vmstat
  cancel_delayed_work
  cpumask_test_and_set_cpu (0->1)

cpumask_clear_cpu(CPU0) (1->0)

-- 
regards
Shiraz Hashim

[toc] | [prev] | [next] | [standalone]

#1315008

On Thu 21-01-16 11:38:46, Christoph Lameter wrote:
> On Thu, 21 Jan 2016, Michal Hocko wrote:
> 
> > It goes like this:
> > CPU0:						CPU1
> > vmstat_update
> >   cpumask_test_and_set_cpu (0->1)
> > [...]
> > 						vmstat_shepherd
> > <enter idle>					  cpumask_test_and_clear_cpu(CPU0) (1->0)
> > quiet_vmstat
> >   cpumask_test_and_set_cpu (0->1)
> >   						  queue_delayed_work_on(CPU0)
> > refresh_cpu_vm_stats()
> > [...]
> > vmstat_update
> >   nothing_to_do
> >   cpumask_test_and_set_cpu (1->1)
> >   VM_BUG_ON
> >
> > Or am I missing something?
> 
> Ok then the following should fix it:

Wouldn't it be much more easier and simply get rid of the VM_BUG_ON?
What is the point of keeping it in the first place. The code can
perfectly cope with the race.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1315096

On Fri, 22 Jan 2016, Michal Hocko wrote:

> Wouldn't it be much more easier and simply get rid of the VM_BUG_ON?
> What is the point of keeping it in the first place. The code can
> perfectly cope with the race.

Ok then lets do that.

[toc] | [prev] | [next] | [standalone]

#1315107

On Fri 22-01-16 10:07:01, Christoph Lameter wrote:
> On Fri, 22 Jan 2016, Michal Hocko wrote:
> 
> > Wouldn't it be much more easier and simply get rid of the VM_BUG_ON?
> > What is the point of keeping it in the first place. The code can
> > perfectly cope with the race.
> 
> Ok then lets do that.

Could you repost the patch with the updated description?

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1315126

On Fri, 22 Jan 2016, Michal Hocko wrote:

> Could you repost the patch with the updated description?

Subject: vmstat: Remove BUG_ON from vmstat_update

If we detect that there is nothing to do just set the flag and do not check
if it was already set before. Races really do not matter. If the flag is
set by any code then the shepherd will start dealing with the situation
and reenable the vmstat workers when necessary again.

Since 0eb77e988032 ("vmstat: make vmstat_updater deferrable again and
shut down on idle") quiet_vmstat might update cpu_stat_off and mark a
particular cpu to be handled by vmstat_shepherd. This might trigger
a VM_BUG_ON in vmstat_update because the work item might have been
sleeping during the idle period and see the cpu_stat_off updated after
the wake up. The VM_BUG_ON is therefore misleading and no more
appropriate. Moreover it doesn't really suite any protection from real
bugs because vmstat_shepherd will simply reschedule the vmstat_work
anytime it sees a particular cpu set or vmstat_update would do the same
from the worker context directly. Even when the two would race the
result wouldn't be incorrect as the counters update is fully idempotent.

Signed-off-by: Christoph Lameter <cl@linux.com>

Index: linux/mm/vmstat.c
===================================================================
--- linux.orig/mm/vmstat.c
+++ linux/mm/vmstat.c
@@ -1408,17 +1408,7 @@ static void vmstat_update(struct work_st
 		 * Defer the checking for differentials to the
 		 * shepherd thread on a different processor.
 		 */
-		int r;
-		/*
-		 * Shepherd work thread does not race since it never
-		 * changes the bit if its zero but the cpu
-		 * online / off line code may race if
-		 * worker threads are still allowed during
-		 * shutdown / startup.
-		 */
-		r = cpumask_test_and_set_cpu(smp_processor_id(),
-			cpu_stat_off);
-		VM_BUG_ON(r);
+		cpumask_set_cpu(smp_processor_id(), cpu_stat_off);
 	}
 }

[toc] | [prev] | [next] | [standalone]

#1315144

On Fri 22-01-16 10:46:14, Christoph Lameter wrote:
> On Fri, 22 Jan 2016, Michal Hocko wrote:
> 
> > Could you repost the patch with the updated description?
> 
> Subject: vmstat: Remove BUG_ON from vmstat_update
> 
> If we detect that there is nothing to do just set the flag and do not check
> if it was already set before. Races really do not matter. If the flag is
> set by any code then the shepherd will start dealing with the situation
> and reenable the vmstat workers when necessary again.
> 
> Since 0eb77e988032 ("vmstat: make vmstat_updater deferrable again and
> shut down on idle") quiet_vmstat might update cpu_stat_off and mark a
> particular cpu to be handled by vmstat_shepherd. This might trigger
> a VM_BUG_ON in vmstat_update because the work item might have been
> sleeping during the idle period and see the cpu_stat_off updated after
> the wake up. The VM_BUG_ON is therefore misleading and no more
> appropriate. Moreover it doesn't really suite any protection from real
> bugs because vmstat_shepherd will simply reschedule the vmstat_work
> anytime it sees a particular cpu set or vmstat_update would do the same
> from the worker context directly. Even when the two would race the
> result wouldn't be incorrect as the counters update is fully idempotent.
>

Fixes: 0eb77e988032 ("vmstat: make vmstat_updater deferrable again and shut down on idle"
Reported-by: Sasha Levin <sasha.levin@oracle.com>

Would be appropriate IMO

> Signed-off-by: Christoph Lameter <cl@linux.com>

Acked-by: Michal Hocko <mhocko@suse.com>

Thanks!

> 
> Index: linux/mm/vmstat.c
> ===================================================================
> --- linux.orig/mm/vmstat.c
> +++ linux/mm/vmstat.c
> @@ -1408,17 +1408,7 @@ static void vmstat_update(struct work_st
>  		 * Defer the checking for differentials to the
>  		 * shepherd thread on a different processor.
>  		 */
> -		int r;
> -		/*
> -		 * Shepherd work thread does not race since it never
> -		 * changes the bit if its zero but the cpu
> -		 * online / off line code may race if
> -		 * worker threads are still allowed during
> -		 * shutdown / startup.
> -		 */
> -		r = cpumask_test_and_set_cpu(smp_processor_id(),
> -			cpu_stat_off);
> -		VM_BUG_ON(r);
> +		cpumask_set_cpu(smp_processor_id(), cpu_stat_off);
>  	}
>  }

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1315680 — fast path cycle muncher (vmstat: make vmstat_updater deferrable again and shut down on idle)

Hi Christoph,

While you're fixing that commit up, can you perhaps find a better home
for quiet_vmstat()?  It not only munches cycles when switching cross
-core mightily, for -rt it injects a sleeping lock into the idle task.

    12.89%  [kernel]       [k] refresh_cpu_vm_stats.isra.12
     4.75%  [kernel]       [k] __schedule                  
     4.70%  [kernel]       [k] mutex_unlock                
     3.14%  [kernel]       [k] __switch_to                 

	-Mike

[toc] | [prev] | [next] | [standalone]

#1315795 — Re: fast path cycle muncher (vmstat: make vmstat_updater deferrable again and shut down on idle)

On Sat, 23 Jan 2016, Mike Galbraith wrote:

> While you're fixing that commit up, can you perhaps find a better home
> for quiet_vmstat()?  It not only munches cycles when switching cross
> -core mightily, for -rt it injects a sleeping lock into the idle task.

Not sure what you are talking about. No sleeping locks are used in
quiet_vmstat() nor does it switch across cores. It would be broken if it
would do so.

[toc] | [prev] | [next] | [standalone]

Page 1 of 3  [1] 2 3  Next page →

Back to top | Article view | linux.kernel

csiph-web