Message ID | 20230605190132.087124739@redhat.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:994d:0:b0:3d9:f83d:47d9 with SMTP id k13csp2896692vqr; Mon, 5 Jun 2023 12:07:31 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ4xzNOURsE0FvsqacauiaMmwz1rfnXfqsDwhFqNUhmZ8WNHDlPFsXWoyMKxOLjRrQbFkKRh X-Received: by 2002:a05:6a00:189a:b0:64f:4d1d:32ba with SMTP id x26-20020a056a00189a00b0064f4d1d32bamr783011pfh.5.1685992051007; Mon, 05 Jun 2023 12:07:31 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1685992050; cv=none; d=google.com; s=arc-20160816; b=v0HevIwr5e3FDNjklAPP+aqKt5vL7j658vKfliHzP/bVDnwEQYm9wj0MkAM30EohRT o3vHEM4SOFWZ5gXNE8TUloJdlj9bYlavAq6IEqcIBqYGybUazS4Lx7s+O3tX5EkKxixt g59v7aEtvBsM9NIGOiV1/nY8qa8v0sJAl2lBpBhFiKMGIWXETbqbAhvpyFJH1HlXWLSp tgUayXquWhEj6IypRj7c6WZiycnAwK0/1nfqYSpWB2ZB+u0f2E6qQTAJD4aySLe0gI7h xkCCM6/IKbQ00Jv3MTRHOp7KUEIrs8GdhVW2riZqyJiUnSHzvvMBhqtolBAIJR618c1q m4og== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:mime-version:references:subject:cc:to:from:date :user-agent:message-id:dkim-signature; bh=HgdXzA8bdDwPq3PKBOP9VV5rvUGTehn183zDvLMapls=; b=RFS4Yz0/8RMwx8LriLy98je8J8N2V/M+hkWLTtW2FeeQ4RgTm8A8LlatfXrc08qeyf iZFdeX85CNGGAcpM/65UBcZciQgzjc1vRk3hQ/oQjcigkb6Vg8wt2fKs/9NiCFLlpRcd +CWs+qzdCWcIMXs5BSAZjZRchAi8Mw605YXV4um5Igd41UOC41R3Clb0SHO+qFdPzCSe umNPOqKg62JjlA5qfAcSuItYqgXT26Hc0HN84pItNkPTujxeaENS8ixALBPQAkzI5Qw9 izGcJeGir5wmq+FtghYaHNW5nryZv+051O9d/eg3lTKCO+SS2HoHwXFkUmDcNk2SNmRX h5eA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=YpC4X5ip; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id t189-20020a6381c6000000b0053fb69cc365si5710060pgd.238.2023.06.05.12.07.18; Mon, 05 Jun 2023 12:07:30 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=YpC4X5ip; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232963AbjFETEE (ORCPT <rfc822;xxoosimple@gmail.com> + 99 others); Mon, 5 Jun 2023 15:04:04 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:59338 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233045AbjFETDl (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 5 Jun 2023 15:03:41 -0400 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4CB84EA for <linux-kernel@vger.kernel.org>; Mon, 5 Jun 2023 12:02:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1685991772; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: references:references; bh=HgdXzA8bdDwPq3PKBOP9VV5rvUGTehn183zDvLMapls=; b=YpC4X5ipFKv8JRCPLYSxzkpnGFqUXu4qbjrYQT0p8Cuc7/nRwTLGhpaHropJ0wTC50CVL+ MKntsWFc3KdPUGusARU+/keohodtGhITnsFP9qzJJ+PoqxVoInYQLrTFF48Mc8WOjyFYQU UXtHMsNfZpYGXsRqEtApSF/U43nGTHU= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-517-PsIrh79mM7mEmfoGH_DiIw-1; Mon, 05 Jun 2023 15:02:51 -0400 X-MC-Unique: PsIrh79mM7mEmfoGH_DiIw-1 Received: from smtp.corp.redhat.com (int-mx09.intmail.prod.int.rdu2.redhat.com [10.11.54.9]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 6EFB63C0BE39; Mon, 5 Jun 2023 19:02:50 +0000 (UTC) Received: from tpad.localdomain (ovpn-112-3.gru2.redhat.com [10.97.112.3]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 31999492B00; Mon, 5 Jun 2023 19:02:50 +0000 (UTC) Received: by tpad.localdomain (Postfix, from userid 1000) id 3C0C140103220; Mon, 5 Jun 2023 16:02:31 -0300 (-03) Message-ID: <20230605190132.087124739@redhat.com> User-Agent: quilt/0.67 Date: Mon, 05 Jun 2023 15:56:30 -0300 From: Marcelo Tosatti <mtosatti@redhat.com> To: Christoph Lameter <cl@linux.com> Cc: Aaron Tomlin <atomlin@atomlin.com>, Frederic Weisbecker <frederic@kernel.org>, Andrew Morton <akpm@linux-foundation.org>, linux-kernel@vger.kernel.org, linux-mm@kvack.org, Vlastimil Babka <vbabka@suse.cz>, Michal Hocko <mhocko@suse.com>, Marcelo Tosatti <mtosatti@redhat.com> Subject: [PATCH v3 3/3] mm/vmstat: do not refresh stats for isolated CPUs References: <20230605185627.923698377@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.9 X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1767890800569999312?= X-GMAIL-MSGID: =?utf-8?q?1767890800569999312?= |
Series |
vmstat bug fixes for nohz_full and isolated CPUs
|
|
Commit Message
Marcelo Tosatti
June 5, 2023, 6:56 p.m. UTC
schedule_work_on API uses the workqueue mechanism to
queue a work item on a queue. A kernel thread, which
runs on the target CPU, executes those work items.
Therefore, when using the schedule_work_on API,
it is necessary for the kworker kernel thread to
be scheduled in, for the work function to be executed.
Time sensitive applications such as SoftPLCs
(https://tum-esi.github.io/publications-list/PDF/2022-ETFA-How_Real_Time_Are_Virtual_PLCs.pdf),
have their response times affected by such interruptions.
The /proc/sys/vm/stat_refresh file was originally introduced
with the goal to:
"Provide /proc/sys/vm/stat_refresh to force an immediate update of
per-cpu into global vmstats: useful to avoid a sleep(2) or whatever
before checking counts when testing. Originally added to work around a
bug which left counts stranded indefinitely on a cpu going idle (an
inaccuracy magnified when small below-batch numbers represent "huge"
amounts of memory), but I believe that bug is now fixed: nonetheless,
this is still a useful knob."
Other than the potential interruption to a time sensitive application,
if using SCHED_FIFO or SCHED_RR priority on the isolated CPU, then
system hangs can occur:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=978688
To avoid the problems above, do not schedule the work to synchronize
per-CPU mm counters on isolated CPUs. Given the possibility for
breaking existing userspace applications, avoid returning
errors from access to /proc/sys/vm/stat_refresh.
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
---
v3: improve changelog (Michal Hocko)
v2: opencode schedule_on_each_cpu (Michal Hocko)
Comments
On Mon 05-06-23 15:56:30, Marcelo Tosatti wrote: > schedule_work_on API uses the workqueue mechanism to > queue a work item on a queue. A kernel thread, which > runs on the target CPU, executes those work items. > > Therefore, when using the schedule_work_on API, > it is necessary for the kworker kernel thread to > be scheduled in, for the work function to be executed. > > Time sensitive applications such as SoftPLCs > (https://tum-esi.github.io/publications-list/PDF/2022-ETFA-How_Real_Time_Are_Virtual_PLCs.pdf), > have their response times affected by such interruptions. > > The /proc/sys/vm/stat_refresh file was originally introduced > with the goal to: > > "Provide /proc/sys/vm/stat_refresh to force an immediate update of > per-cpu into global vmstats: useful to avoid a sleep(2) or whatever > before checking counts when testing. Originally added to work around a > bug which left counts stranded indefinitely on a cpu going idle (an > inaccuracy magnified when small below-batch numbers represent "huge" > amounts of memory), but I believe that bug is now fixed: nonetheless, > this is still a useful knob." > > Other than the potential interruption to a time sensitive application, > if using SCHED_FIFO or SCHED_RR priority on the isolated CPU, then > system hangs can occur: The same thing can happen without isolated CPUs and this patch doesn't help at all. > https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=978688 And this is an example of that... > To avoid the problems above, do not schedule the work to synchronize > per-CPU mm counters on isolated CPUs. Given the possibility for > breaking existing userspace applications, avoid returning > errors from access to /proc/sys/vm/stat_refresh. > > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com> It would be really helpful to not post new versions while discussion of the previous one is still not done. Anyway Nacked-by: Michal Hocko <mhocko@suse.com> This is silently changing semantic and I do not think you have actually shown this is a real life problem. To me it sounds like a theoretical issue at most and it can be worked around by disalowing to use this interface from userspace. stat_refresh is mostly for debugging purposes and I strongly doubt it is ever used in environments you refer to in this series. > > --- > v3: improve changelog (Michal Hocko) > v2: opencode schedule_on_each_cpu (Michal Hocko) > > Index: linux-vmstat-remote/mm/vmstat.c > =================================================================== > --- linux-vmstat-remote.orig/mm/vmstat.c > +++ linux-vmstat-remote/mm/vmstat.c > @@ -1881,8 +1881,13 @@ int vmstat_refresh(struct ctl_table *tab > void *buffer, size_t *lenp, loff_t *ppos) > { > long val; > - int err; > int i; > + int cpu; > + struct work_struct __percpu *works; > + > + works = alloc_percpu(struct work_struct); > + if (!works) > + return -ENOMEM; > > /* > * The regular update, every sysctl_stat_interval, may come later > @@ -1896,9 +1901,24 @@ int vmstat_refresh(struct ctl_table *tab > * transiently negative values, report an error here if any of > * the stats is negative, so we know to go looking for imbalance. > */ > - err = schedule_on_each_cpu(refresh_vm_stats); > - if (err) > - return err; > + cpus_read_lock(); > + for_each_online_cpu(cpu) { > + struct work_struct *work; > + > + if (cpu_is_isolated(cpu)) > + continue; > + work = per_cpu_ptr(works, cpu); > + INIT_WORK(work, refresh_vm_stats); > + schedule_work_on(cpu, work); > + } > + > + for_each_online_cpu(cpu) { > + if (cpu_is_isolated(cpu)) > + continue; > + flush_work(per_cpu_ptr(works, cpu)); > + } > + cpus_read_unlock(); > + free_percpu(works); > for (i = 0; i < NR_VM_ZONE_STAT_ITEMS; i++) { > /* > * Skip checking stats known to go negative occasionally. >
On Mon, Jun 05, 2023 at 09:20:12PM +0200, Michal Hocko wrote: > On Mon 05-06-23 15:56:30, Marcelo Tosatti wrote: > > schedule_work_on API uses the workqueue mechanism to > > queue a work item on a queue. A kernel thread, which > > runs on the target CPU, executes those work items. > > > > Therefore, when using the schedule_work_on API, > > it is necessary for the kworker kernel thread to > > be scheduled in, for the work function to be executed. > > > > Time sensitive applications such as SoftPLCs > > (https://tum-esi.github.io/publications-list/PDF/2022-ETFA-How_Real_Time_Are_Virtual_PLCs.pdf), > > have their response times affected by such interruptions. > > > > The /proc/sys/vm/stat_refresh file was originally introduced > > with the goal to: > > > > "Provide /proc/sys/vm/stat_refresh to force an immediate update of > > per-cpu into global vmstats: useful to avoid a sleep(2) or whatever > > before checking counts when testing. Originally added to work around a > > bug which left counts stranded indefinitely on a cpu going idle (an > > inaccuracy magnified when small below-batch numbers represent "huge" > > amounts of memory), but I believe that bug is now fixed: nonetheless, > > this is still a useful knob." > > > > Other than the potential interruption to a time sensitive application, > > if using SCHED_FIFO or SCHED_RR priority on the isolated CPU, then > > system hangs can occur: > > The same thing can happen without isolated CPUs and this patch doesn't > help at all. > > > https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=978688 > > And this is an example of that... > > > To avoid the problems above, do not schedule the work to synchronize > > per-CPU mm counters on isolated CPUs. Given the possibility for > > breaking existing userspace applications, avoid returning > > errors from access to /proc/sys/vm/stat_refresh. > > > > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com> > > It would be really helpful to not post new versions while discussion of > the previous one is still not done. > > Anyway > Nacked-by: Michal Hocko <mhocko@suse.com> > > This is silently changing semantic and I do not think you have actually > shown this is a real life problem. https://bugzilla.redhat.com/show_bug.cgi?id=1921601 https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=978688 > To me it sounds like a theoretical > issue Its not (see data above). > at most and it can be worked around by disalowing to use this > interface from userspace. stat_refresh is mostly for debugging purposes > and I strongly doubt it is ever used in environments you refer to in > this series. Based on experience, I strongly believe people will run latency sensitive apps and end up reading/writing this file.
On Mon 05-06-23 16:53:51, Marcelo Tosatti wrote: > On Mon, Jun 05, 2023 at 09:20:12PM +0200, Michal Hocko wrote: > > On Mon 05-06-23 15:56:30, Marcelo Tosatti wrote: > > > schedule_work_on API uses the workqueue mechanism to > > > queue a work item on a queue. A kernel thread, which > > > runs on the target CPU, executes those work items. > > > > > > Therefore, when using the schedule_work_on API, > > > it is necessary for the kworker kernel thread to > > > be scheduled in, for the work function to be executed. > > > > > > Time sensitive applications such as SoftPLCs > > > (https://tum-esi.github.io/publications-list/PDF/2022-ETFA-How_Real_Time_Are_Virtual_PLCs.pdf), > > > have their response times affected by such interruptions. > > > > > > The /proc/sys/vm/stat_refresh file was originally introduced > > > with the goal to: > > > > > > "Provide /proc/sys/vm/stat_refresh to force an immediate update of > > > per-cpu into global vmstats: useful to avoid a sleep(2) or whatever > > > before checking counts when testing. Originally added to work around a > > > bug which left counts stranded indefinitely on a cpu going idle (an > > > inaccuracy magnified when small below-batch numbers represent "huge" > > > amounts of memory), but I believe that bug is now fixed: nonetheless, > > > this is still a useful knob." > > > > > > Other than the potential interruption to a time sensitive application, > > > if using SCHED_FIFO or SCHED_RR priority on the isolated CPU, then > > > system hangs can occur: > > > > The same thing can happen without isolated CPUs and this patch doesn't > > help at all. > > > > > https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=978688 > > > > And this is an example of that... > > > > > To avoid the problems above, do not schedule the work to synchronize > > > per-CPU mm counters on isolated CPUs. Given the possibility for > > > breaking existing userspace applications, avoid returning > > > errors from access to /proc/sys/vm/stat_refresh. > > > > > > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com> > > > > It would be really helpful to not post new versions while discussion of > > the previous one is still not done. > > > > Anyway > > Nacked-by: Michal Hocko <mhocko@suse.com> > > > > This is silently changing semantic and I do not think you have actually > > shown this is a real life problem. > > https://bugzilla.redhat.com/show_bug.cgi?id=1921601 Yet anothe new reference. Is the story complete now? > https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=978688 Again, nothing isolcpu related. Simple RR/realtime cpu hoggers that this patch has not way to deal with. Why do you keep referencing it? > > To me it sounds like a theoretical > > issue > > Its not (see data above). No it't not. Either your sosreport need precise counters and stat_refresh is exactly what it should be doing or it can live with imprecision and then it should avoid it. Full stop. There is no reason to tweak the existing semantic to half bake isolcpu case while the fundamental problem is still there.
Index: linux-vmstat-remote/mm/vmstat.c =================================================================== --- linux-vmstat-remote.orig/mm/vmstat.c +++ linux-vmstat-remote/mm/vmstat.c @@ -1881,8 +1881,13 @@ int vmstat_refresh(struct ctl_table *tab void *buffer, size_t *lenp, loff_t *ppos) { long val; - int err; int i; + int cpu; + struct work_struct __percpu *works; + + works = alloc_percpu(struct work_struct); + if (!works) + return -ENOMEM; /* * The regular update, every sysctl_stat_interval, may come later @@ -1896,9 +1901,24 @@ int vmstat_refresh(struct ctl_table *tab * transiently negative values, report an error here if any of * the stats is negative, so we know to go looking for imbalance. */ - err = schedule_on_each_cpu(refresh_vm_stats); - if (err) - return err; + cpus_read_lock(); + for_each_online_cpu(cpu) { + struct work_struct *work; + + if (cpu_is_isolated(cpu)) + continue; + work = per_cpu_ptr(works, cpu); + INIT_WORK(work, refresh_vm_stats); + schedule_work_on(cpu, work); + } + + for_each_online_cpu(cpu) { + if (cpu_is_isolated(cpu)) + continue; + flush_work(per_cpu_ptr(works, cpu)); + } + cpus_read_unlock(); + free_percpu(works); for (i = 0; i < NR_VM_ZONE_STAT_ITEMS; i++) { /* * Skip checking stats known to go negative occasionally.