From: Srikar Dronamraju <srikar@linux•vnet.ibm.com>
To: Michael Bringmann <mwb@linux•vnet.ibm.com>
Cc: Rob Herring <robh@kernel•org>, Kees Cook <keescook@chromium•org>,
Nicholas Piggin <npiggin@gmail•com>,
linux-kernel@vger•kernel.org, Al Viro <viro@zeniv•linux.org.uk>,
Juliet Kim <minkim@us•ibm.com>,
Oliver O'Halloran <oohall@gmail•com>,
Tyrel Datwyler <tyreld@linux•vnet.ibm.com>,
Nathan Lynch <nathanl@linux•vnet.ibm.com>,
Thomas Falcon <tlfalcon@linux•vnet.ibm.com>,
linuxppc-dev@lists•ozlabs.org, Guenter Roeck <linux@roeck-us•net>,
Corentin Labbe <clabbe@baylibre•com>
Subject: Re: [PATCH v03] powerpc/numa: Perform full re-add of CPU for PRRN/VPHN topology update
Date: Fri, 8 Feb 2019 11:14:03 +0530 [thread overview]
Message-ID: <20190208054403.GA24971@linux.vnet.ibm.com> (raw)
In-Reply-To: <305ed693-ea85-8a70-1d3c-ae405aebc0ad@linux.vnet.ibm.com>
>
> int arch_update_cpu_topology(void)
> {
> - return numa_update_cpu_topology(true);
> + int changed = topology_changed;
> +
> + topology_changed = 0;
> + return changed;
> }
>
Do we need Powerpc override for arch_update_cpu_topology() now? That
topology_changed sometime back doesn't seem to have help. The scheduler
atleast now is neglecting whether the topology changed or not.
Also we can do away with the new topology_changed.
> static void topology_work_fn(struct work_struct *work)
> {
> - rebuild_sched_domains();
> + lock_device_hotplug();
> + if (numa_update_cpu_topology(true))
> + rebuild_sched_domains();
> + unlock_device_hotplug();
> }
Should this hunk be a separate patch by itself to say why
rebuild_sched_domains with a changelog that explains why it should be under
lock_device_hotplug? rebuild_sched_domains already takes cpuset_mutex.
So I am not sure if we need to take device_hotplug_lock.
> static DECLARE_WORK(topology_work, topology_work_fn);
>
> -static void topology_schedule_update(void)
> +void topology_schedule_update(void)
> {
> - schedule_work(&topology_work);
> + if (!topology_update_in_progress)
> + schedule_work(&topology_work);
> }
>
> static void topology_timer_fn(struct timer_list *unused)
> {
> + bool sdo = false;
Is sdo any abbrevation?
> +
> + if (topology_scans < 1)
> + bitmap_fill(cpumask_bits(&cpu_associativity_changes_mask),
> + nr_cpumask_bits);
Why do we need topology_scan? Just to make sure
cpu_associativity_changes_mask is populated only once?
cant we use a static bool inside the function for the same?
> +
> if (prrn_enabled && cpumask_weight(&cpu_associativity_changes_mask))
> - topology_schedule_update();
> - else if (vphn_enabled) {
> + sdo = true;
> + if (vphn_enabled) {
Any reason to remove the else above?
> if (update_cpu_associativity_changes_mask() > 0)
> - topology_schedule_update();
> + sdo = true;
> reset_topology_timer();
> }
> + if (sdo)
> + topology_schedule_update();
> + topology_scans++;
> }
Are the above two hunks necessary? Not getting how the current changes are
different from the previous.
--
Thanks and Regards
Srikar Dronamraju
next prev parent reply other threads:[~2019-02-08 5:46 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-06 15:56 [PATCH v03] powerpc/numa: Perform full re-add of CPU for PRRN/VPHN topology update Michael Bringmann
2019-02-07 4:48 ` kbuild test robot
2019-02-08 5:44 ` Srikar Dronamraju [this message]
2019-02-08 19:43 ` Michael Bringmann
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=20190208054403.GA24971@linux.vnet.ibm.com \
--to=srikar@linux$(echo .)vnet.ibm.com \
--cc=clabbe@baylibre$(echo .)com \
--cc=keescook@chromium$(echo .)org \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=linux@roeck-us$(echo .)net \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
--cc=minkim@us$(echo .)ibm.com \
--cc=mwb@linux$(echo .)vnet.ibm.com \
--cc=nathanl@linux$(echo .)vnet.ibm.com \
--cc=npiggin@gmail$(echo .)com \
--cc=oohall@gmail$(echo .)com \
--cc=robh@kernel$(echo .)org \
--cc=tlfalcon@linux$(echo .)vnet.ibm.com \
--cc=tyreld@linux$(echo .)vnet.ibm.com \
--cc=viro@zeniv$(echo .)linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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