From owner-svn-src-head@freebsd.org Thu Nov 30 19:04:58 2017 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id D57F0E68FF0; Thu, 30 Nov 2017 19:04:58 +0000 (UTC) (envelope-from cse.cem@gmail.com) Received: from mail-it0-f49.google.com (mail-it0-f49.google.com [209.85.214.49]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id A34A4708B9; Thu, 30 Nov 2017 19:04:58 +0000 (UTC) (envelope-from cse.cem@gmail.com) Received: by mail-it0-f49.google.com with SMTP id p139so9600404itb.1; Thu, 30 Nov 2017 11:04:58 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:reply-to:in-reply-to:references :from:date:message-id:subject:to:cc; bh=Sli61XDq4CeCpWKo6GsOWN8+HOY6e5wVhFg86fvfcyA=; b=QWmVW3xi3vJDw6fI71ex7wBSPlGMHC33SMelAXJedpDRC0wjLb+f018nZpAfM2g81L 3XLKCLV1wwpK7xr09NRRj+DrMOnMbVwYBRABjpxJmZ5p1qT+DxKTEpUSOk4ffZQyMKyg oI1XikQyYMd/nZAEvU8vCHMl1ek2qZXj/h4i6udttj96Sb6X8R+babDqGgJksxrvDR5k gSg2hE/bSI2jihlB7yHAQxTpZGneIaJtrNZOBOTPgyRbyx6bVFtfgnLN8MZZGgUA52Wj rgpHGbAedlxsX448lI5j0aLcP6Ai9w4pd+UjPLNSd2HU3agDN6tWmjeZK2NMXBUP/rIQ u8lg== X-Gm-Message-State: AJaThX49SWd42pBB+cLCM/hphR0QFg1LL7z5hGels3JFFjEK0XTIv/k1 Uviu+K2Bx3vHpdD5B5P+4T6Y6kPi X-Google-Smtp-Source: AGs4zMZqgAcyQmLTfqc0QUHFq1aQcDbrRnyG7GpD/rlpGu4yjtiB4jbwp5cUivpdv2pfBySRcekc0g== X-Received: by 10.36.125.75 with SMTP id b72mr4727555itc.16.1512068230314; Thu, 30 Nov 2017 10:57:10 -0800 (PST) Received: from mail-it0-f51.google.com (mail-it0-f51.google.com. [209.85.214.51]) by smtp.gmail.com with ESMTPSA id w133sm2577850itc.44.2017.11.30.10.57.09 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 30 Nov 2017 10:57:09 -0800 (PST) Received: by mail-it0-f51.google.com with SMTP id z6so9514460iti.4; Thu, 30 Nov 2017 10:57:09 -0800 (PST) X-Received: by 10.36.228.68 with SMTP id o65mr4736862ith.128.1512068229765; Thu, 30 Nov 2017 10:57:09 -0800 (PST) MIME-Version: 1.0 Reply-To: cem@freebsd.org Received: by 10.2.165.150 with HTTP; Thu, 30 Nov 2017 10:57:09 -0800 (PST) In-Reply-To: <201711300140.vAU1e7dC001292@repo.freebsd.org> References: <201711300140.vAU1e7dC001292@repo.freebsd.org> From: Conrad Meyer Date: Thu, 30 Nov 2017 10:57:09 -0800 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: svn commit: r326383 - head/sys/x86/cpufreq To: Jung-uk Kim Cc: src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset="UTF-8" X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 30 Nov 2017 19:04:58 -0000 Hi Jung-uk, I have some questions about this change. On Wed, Nov 29, 2017 at 5:40 PM, Jung-uk Kim wrote: > Author: jkim > Date: Thu Nov 30 01:40:07 2017 > New Revision: 326383 > URL: https://svnweb.freebsd.org/changeset/base/326383 > > Log: > Add a tunable "debug.hwpstate_verify" to check P-state after changing it and > turn it off by default. It is very inefficient to verify current P-state of > each core, especially for CPUs with many cores. When multiple commands are > requested to the same power domain before completion of pending transitions, > the last command is executed according to the manual. Because requests are > serialized by the caller, all cores will receive the same command for each > call. Do not call sched_bind() and sched_unbind(). It is redundant because > the caller does it anyway. > ... > @@ -176,47 +178,57 @@ hwpstate_goto_pstate(device_t dev, int pstate) > if (limit > id) > id = limit; > Should we bind the thread and record PCPU_GET() here? > + HWPSTATE_DEBUG(dev, "setting P%d-state on cpu%d\n", id, > + PCPU_GET(cpuid)); > + /* Go To Px-state */ > + wrmsr(MSR_AMD_10H_11H_CONTROL, id); > + > /* > * We are going to the same Px-state on all cpus. > * Probably should take _PSD into account. > */ > - error = 0; > CPU_FOREACH(i) { > + if (i == PCPU_GET(cpuid)) It seems like this check could evaluate to a different CPU every time? When really we are trying to avoid setting on the CPU we set on initially above? > + continue; > + > /* Bind to each cpu. */ > thread_lock(curthread); > sched_bind(curthread, i); > thread_unlock(curthread); > - HWPSTATE_DEBUG(dev, "setting P%d-state on cpu%d\n", > - id, PCPU_GET(cpuid)); > + HWPSTATE_DEBUG(dev, "setting P%d-state on cpu%d\n", id, i); > /* Go To Px-state */ > wrmsr(MSR_AMD_10H_11H_CONTROL, id); > } > - CPU_FOREACH(i) { > - /* Bind to each cpu. */ > - thread_lock(curthread); > - sched_bind(curthread, i); > - thread_unlock(curthread); > - /* wait loop (100*100 usec is enough ?) */ > - for (j = 0; j < 100; j++) { > - /* get the result. not assure msr=id */ > - msr = rdmsr(MSR_AMD_10H_11H_STATUS); > - if (msr == id) > - break; > - sbt = SBT_1MS / 10; > - tsleep_sbt(dev, PZERO, "pstate_goto", sbt, > - sbt >> tc_precexp, 0); > + > + /* > + * Verify whether each core is in the requested P-state. > + */ > + if (hwpstate_verify) { > + CPU_FOREACH(i) { > + thread_lock(curthread); > + sched_bind(curthread, i); > + thread_unlock(curthread); > + /* wait loop (100*100 usec is enough ?) */ > + for (j = 0; j < 100; j++) { > + /* get the result. not assure msr=id */ > + msr = rdmsr(MSR_AMD_10H_11H_STATUS); > + if (msr == id) > + break; > + sbt = SBT_1MS / 10; > + tsleep_sbt(dev, PZERO, "pstate_goto", sbt, > + sbt >> tc_precexp, 0); > + } > + HWPSTATE_DEBUG(dev, "result: P%d-state on cpu%d\n", > + (int)msr, i); > + if (msr != id) { > + HWPSTATE_DEBUG(dev, > + "error: loop is not enough.\n"); In this error return, should the current thread be unbinded? The old code did this by setting error and falling through to the ordinary exit path. We could use 'goto out;' to avoid looping through the rest of the CPUs. > + return (ENXIO); > + } > } > - HWPSTATE_DEBUG(dev, "result: P%d-state on cpu%d\n", > - (int)msr, PCPU_GET(cpuid)); > - if (msr != id) { > - HWPSTATE_DEBUG(dev, "error: loop is not enough.\n"); > - error = ENXIO; > - } > } > - thread_lock(curthread); > - sched_unbind(curthread); > - thread_unlock(curthread); > - return (error); > + > + return (0); > } > > static int > Best, Conrad