From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966075AbcIVVlS (ORCPT ); Thu, 22 Sep 2016 17:41:18 -0400 Received: from mga05.intel.com ([192.55.52.43]:50502 "EHLO mga05.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965755AbcIVVlP (ORCPT ); Thu, 22 Sep 2016 17:41:15 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.30,379,1470726000"; d="scan'208";a="12362950" Message-ID: <1474580474.3916.238.camel@linux.intel.com> Subject: Re: [PATCH v4 10/10] cpufreq: intel_pstate: Use CPPC to get max performance From: Tim Chen To: "Rafael J. Wysocki" Cc: Srinivas Pandruvada , "Rafael J. Wysocki" , Thomas Gleixner , Ingo Molnar , Borislav Petkov , the arch/x86 maintainers , Linux PM , Linux Kernel Mailing List , ACPI Devel Maling List , Peter Zijlstra , jolsa@redhat.com Date: Thu, 22 Sep 2016 14:41:14 -0700 In-Reply-To: References: <1474485552-141429-1-git-send-email-srinivas.pandruvada@linux.intel.com> <1474485552-141429-11-git-send-email-srinivas.pandruvada@linux.intel.com> <1474570233.3916.197.camel@linux.intel.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.18.5.2 (3.18.5.2-1.fc23) Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 2016-09-22 at 22:58 +0200, Rafael J. Wysocki wrote: >  > > > so what if there are two CPU packages > > > and there are highest_perf differences in both, and we first enumerate > > > the first package entirely before getting to the second one? > > > > > > In that case we'll schedule the work item after enumerating the first > > > package and it may rebuild the sched domains before all priorities are > > > set for the second package, may it not? > > That is not a problem.  For the second package, all the cpu priorities > > are initialized to the same value.  So even if we start to do > > asym_packing in the scheduler for the whole system, > > on the second package, all the cpus are treated equally by the scheduler. > > We will operate as if there is no favored core till we update the > > priorities of the cpu on the second package. > OK > > But updating those priorities after we have set the "ITMT capable" > flag is not a problem?  Nobody is going to be confused and so on? > Not a problem.  The worst thing that could happen is we schedule a job to a cpu with a lesser max turbo freq first while the priorities update are in progress. > > > > That said, we don't enable ITMT automatically for 2 package system. > > So the explicit sysctl command to enable ITMT and cause the sched domain > > rebuild for 2 package system is most likely to come after > > we have discovered and set all the cpu priorities. > Right, but if that behavior is relied on, there should be a comment > about that in the code (and relying on it would be kind of fragile for > that matter). No, we don't rely on this behavior of not enabling ITMT automatically for 2 package system.  We could enable ITMT for 2 package system by default if we want to.  Then asym_packing will just consider the second package's cpus to be equal priorities if they haven't been set.   > > > > > > > > > > > > This seems to require some more consideration. > > > > > > > > > > > > > > > +               /* > > > > +                * Since this function is in the hotcpu notifier callback > > > > +                * path, submit a task to workqueue to call > > > > +                * sched_set_itmt_support(). > > > > +                */ > > > > +               schedule_work(&sched_itmt_work); > > > It doesn't make sense to do this more than once IMO and what if we > > > attempt to schedule the work item again when it has been scheduled > > > once already?  Don't we need any protection here? > > It is not a problem for sched_set_itmt_support to be called more than > > once. > While it is not incorrect, it also is not particularly useful to > schedule a work item just to find out later that it had nothing to do > to begin with. Setting ITMT capability is done per socket during system boot.  So there is no performance impact at all so it should not be an issue. Tim