LinuxPPC-Dev Archive on lore.kernel.org
 help / color / Atom feed
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
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


  parent reply index

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-06 15:56 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 publically 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.vnet.ibm.com \
    --cc=clabbe@baylibre.com \
    --cc=keescook@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=minkim@us.ibm.com \
    --cc=mwb@linux.vnet.ibm.com \
    --cc=nathanl@linux.vnet.ibm.com \
    --cc=npiggin@gmail.com \
    --cc=oohall@gmail.com \
    --cc=robh@kernel.org \
    --cc=tlfalcon@linux.vnet.ibm.com \
    --cc=tyreld@linux.vnet.ibm.com \
    --cc=viro@zeniv.linux.org.uk \
    /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

LinuxPPC-Dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linuxppc-dev/0 linuxppc-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linuxppc-dev linuxppc-dev/ https://lore.kernel.org/linuxppc-dev \
		linuxppc-dev@lists.ozlabs.org linuxppc-dev@ozlabs.org linuxppc-dev@archiver.kernel.org
	public-inbox-index linuxppc-dev


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.ozlabs.lists.linuxppc-dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox