From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752569AbbGIG67 (ORCPT ); Thu, 9 Jul 2015 02:58:59 -0400 Received: from e23smtp07.au.ibm.com ([202.81.31.140]:56941 "EHLO e23smtp07.au.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752565AbbGIG6v (ORCPT ); Thu, 9 Jul 2015 02:58:51 -0400 X-Helo: d23dlp02.au.ibm.com X-MailFrom: srikar@linux.vnet.ibm.com X-RcptTo: linux-kernel@vger.kernel.org Date: Thu, 9 Jul 2015 12:27:46 +0530 From: Srikar Dronamraju To: Ingo Molnar Cc: Rik van Riel , Peter Zijlstra , linux-kernel@vger.kernel.org, Mel Gorman Subject: Re: [PATCH] sched/numa: Restore sched feature NUMA to its earlier avatar. Message-ID: <20150709065746.GE26095@linux.vnet.ibm.com> Reply-To: Srikar Dronamraju References: <1436361633-4970-1-git-send-email-srikar@linux.vnet.ibm.com> <20150708135644.GC23380@gmail.com> <559D4128.2080606@redhat.com> <20150709062926.GA31232@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20150709062926.GA31232@gmail.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 15070906-0025-0000-0000-000001C0B358 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > So I find the patch, the description and the comments in the code conflicting and > confusing. > > The patch does this: > > @@ -5676,10 +5676,10 @@ static int migrate_degrades_locality(struct task_struct *p, struct lb_env *env) > unsigned long src_faults, dst_faults; > int src_nid, dst_nid; > > - if (!p->numa_faults || !(env->sd->flags & SD_NUMA)) > + if (!sched_feat(NUMA) || !sched_feat(NUMA_FAVOUR_HIGHER)) > return -1; > > - if (!sched_feat(NUMA)) > + if (!p->numa_faults || !(env->sd->flags & SD_NUMA)) > return -1; > > src_nid = cpu_to_node(env->src_cpu); > > > while the default for 'NUMA' is 0, 'NUMA_FAVOUR_HIGHER' is 1. > > Which in itself is confusing: WTH do we have a generic switch called 'NUMA' and > then have it disabled? NUMA feature gets enabled on multi-node boxes because of start_kernel() -> numa_policy_init() -> check_numabalancing_enable() -> set_numabalancing_state() -> sched_feat_set("NUMA"); > > Secondly, and more importantly, this patch is equivalent to adding this (for the > default case): > > return -1; This is true on only UMA box. On a numa box, the NUMA feature would be enabled, so it wouldnt return -1 by default. > > i.e. it's in essence a revert of 8a9e62a! > While 8a9e62a did miss the part where we would enable NUMA on numa boxes, this commit doesnt completely revert 8a9e62a. > And it provides no explanation whatsoever. Why did we do the original change > (8a9e62a) which was well argued but apparently broken in some fashion, and why do > we want to change it back now? The original change "8a9e62a" gives preference to numa hotness compared to cache hotness. The rational being, for numa workloads tasks are better of looking at numa convergence than be spread based on cache hotness. migrate_swap/migrate_task_to already move tasks without bothering about cache hotness so that convergence is achieved. > > I.e. this patch sucks on multiple grounds, and 8a9e62a probably sucks as well. And > you added a Reviewed-by while you should have noticed at least 2-3 flaws in the > patch and its approach. Not good. > -- Thanks and Regards Srikar Dronamraju