linux-kernel - Re: [PATCH v03] powerpc/numa: Perform full re-add of CPU for PRRN/VPHN topology update

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <20190208054403.GA24971@linux.vnet.ibm.com>
Date:   Fri, 8 Feb 2019 11:14:03 +0530
From:   Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
To:     Michael Bringmann <mwb@...ux.vnet.ibm.com>
Cc:     linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
        Michael Ellerman <mpe@...erman.id.au>,
        Nathan Lynch <nathanl@...ux.vnet.ibm.com>,
        Corentin Labbe <clabbe@...libre.com>,
        Tyrel Datwyler <tyreld@...ux.vnet.ibm.com>,
        Guenter Roeck <linux@...ck-us.net>,
        "Oliver O'Halloran" <oohall@...il.com>,
        Russell Currey <ruscur@...sell.cc>,
        Haren Myneni <haren@...ibm.com>,
        Al Viro <viro@...iv.linux.org.uk>,
        Kees Cook <keescook@...omium.org>,
        Nicholas Piggin <npiggin@...il.com>,
        Rob Herring <robh@...nel.org>, Juliet Kim <minkim@...ibm.com>,
        Thomas Falcon <tlfalcon@...ux.vnet.ibm.com>
Subject: Re: [PATCH v03] powerpc/numa: Perform full re-add of CPU for
 PRRN/VPHN topology update

> 
>  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