From: Michal Hocko <mhocko@kernel.org>
To: Christoph Lameter <cl@linux.com>
Cc: akpm@linux-foundation.org, linux-mm@kvack.org,
Tejun Heo <htejun@gmail.com>,
Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Subject: Re: [PATCH] vmstat: Get rid of the ugly cpu_stat_off variable V2
Date: Wed, 11 May 2016 14:19:57 +0200 [thread overview]
Message-ID: <20160511121957.GK16677@dhcp22.suse.cz> (raw)
In-Reply-To: <alpine.DEB.2.20.1605061306460.17934@east.gentwo.org>
On Fri 06-05-16 13:09:49, Christoph Lameter wrote:
> The cpu_stat_off variable is unecessary since we can check if
> a workqueue request is pending otherwise. Removal of
> cpu_stat_off makes it pretty easy for the vmstat shepherd to
> ensure that the proper things happen.
OK, this looks good to me. It is racy ...
vmstat_shepherd vmstat_update
delayed_work_pending() # false
need_update() refresh_cpu_vm_stats()
queue_delayed_work_on() queue_delayed_work_on()
... but it doesn't matter because queue_delayed_work_on is a noop
if the work is already queued.
> Removing the state also removes all races related to it.
Do we have any races left? I do not see any.
> Should a workqueue not be scheduled as needed for vmstat_update
> then the shepherd will notice and schedule it as needed.
> Should a workqueue be unecessarily scheduled then the vmstat
> updater will disable it.
The code simplification is really nice!
> V1->V2:
> - Rediff to proper upstream version
>
> Signed-off-by: Christoph Lameter <cl@linux.com>
Acked-by: Michal Hocko <mhocko@suse.com>
A nit below
> @@ -1475,20 +1454,11 @@ 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) {
> + for_each_online_cpu(cpu) {
> struct delayed_work *dw = &per_cpu(vmstat_work, cpu);
>
> - if (need_update(cpu)) {
> - if (cpumask_test_and_clear_cpu(cpu, cpu_stat_off))
> + if (!delayed_work_pending(dw) && need_update(cpu))
> queue_delayed_work_on(cpu, vmstat_wq, dw, 0);
Indentation here...
> - } else {
> - /*
> - * Cancel the work if quiet_vmstat has put this
> - * cpu on cpu_stat_off because the work item might
> - * be still scheduled
> - */
> - cancel_delayed_work(dw);
> - }
> }
> put_online_cpus();
>
--
Michal Hocko
SUSE Labs
--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
prev parent reply other threads:[~2016-05-11 12:20 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-06 18:09 Christoph Lameter
2016-05-11 12:19 ` Michal Hocko [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160511121957.GK16677@dhcp22.suse.cz \
--to=mhocko@kernel.org \
--cc=akpm@linux-foundation.org \
--cc=cl@linux.com \
--cc=htejun@gmail.com \
--cc=linux-mm@kvack.org \
--cc=penguin-kernel@I-love.SAKURA.ne.jp \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox