| Message ID | 20221105174225.28673-1-rui.zhang@intel.com |
|---|---|
| State | New |
| Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org>
Delivered-To: ouuuleilei@gmail.com
Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp1111768wru;
Sat, 5 Nov 2022 11:10:38 -0700 (PDT)
X-Google-Smtp-Source:
AMsMyM5lIyMS8rluv9hmyu9T9C+XQC1+ZCi7T0d+Wch8zhsABQU+egZbE1PgM1PAtGWb5+ammISo
X-Received: by 2002:a17:906:9b83:b0:730:b3ae:343 with SMTP id
dd3-20020a1709069b8300b00730b3ae0343mr41405095ejc.670.1667671838427;
Sat, 05 Nov 2022 11:10:38 -0700 (PDT)
ARC-Seal: i=1; a=rsa-sha256; t=1667671838; cv=none;
d=google.com; s=arc-20160816;
b=oP67QpaYsIsDUEknR88Vwp3AHky8kC2sJ8OvCKR9lentSQXeZRIyAYCfKxeFVF5S9k
uAYu6Y5g1FqCq8CK2BlUmNd7nwHlBUJ1XU5qZuxQHXiGfBBtXEZR8Qx7hD9j72Kyo66I
RE4OHa2Po56koLtv3Qim45DTrGPzge5Wd5mMjFVm4Y4eNOTv1jWS7rc+PE5yx9GHbWKg
JzyeuJk3UYVFXJfQuhpjq2Sz3Vx2PDOyxmkryAuK1xI+rIMrKmRANzqtUPO7nuhynrEH
U8HzQgmP9ZTsN+xfLf3e1A0m+ebIUlQaIqaTnevtYC8DSXysPu8tJSnr/9Lpir9bzlNd
HAoQ==
ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com;
s=arc-20160816;
h=list-id:precedence:message-id:date:subject:cc:to:from
:dkim-signature;
bh=Y4DvsOOtaghC4NJA7Ib5elOpiOMVFQ3oWjE176ayOAs=;
b=x6AJWK1GJwUghKYduf9Y57mODCgqC6VpDYWSoYEzNwT0Vnoqy9BxZPC3srZVWXnulD
FltB9nYJJfRxa1xjGgmLkvnTLiBCeLYVQT+FXMR2D7EXCsCzr8EZRWAvCbH/RutqeK3g
2B9ZJaMY1Yp1RNnYEyuOPNYJjC6lPGDpo687znLIwABNnx2uDEqVM4gFBNlN0mWfkyWi
WCBwD9haJeuWKuAkGy48ourabmGpAGhuK/kq4rU80vh+izxyV1g1G8IEMIdQQiQD6Aym
g/+nY2Jfewacu2ZSTFXDM1Nx0geJF4tkVbo3wO441E4tduKrjE4EXYVQVcHPDIk0y0sa
OzqA==
ARC-Authentication-Results: i=1; mx.google.com;
dkim=pass header.i=@intel.com header.s=Intel header.b=PtTCtXZ8;
spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org
designates 2620:137:e000::1:20 as permitted sender)
smtp.mailfrom=linux-kernel-owner@vger.kernel.org;
dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com
Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20])
by mx.google.com with ESMTP id
d31-20020a056402401f00b00459ff7667b4si3369768eda.203.2022.11.05.11.10.14;
Sat, 05 Nov 2022 11:10:38 -0700 (PDT)
Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org
designates 2620:137:e000::1:20 as permitted sender)
client-ip=2620:137:e000::1:20;
Authentication-Results: mx.google.com;
dkim=pass header.i=@intel.com header.s=Intel header.b=PtTCtXZ8;
spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org
designates 2620:137:e000::1:20 as permitted sender)
smtp.mailfrom=linux-kernel-owner@vger.kernel.org;
dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
id S229863AbiKERkO (ORCPT <rfc822;hjfbswb@gmail.com> + 99 others);
Sat, 5 Nov 2022 13:40:14 -0400
Received: from lindbergh.monkeyblade.net ([23.128.96.19]:34452 "EHLO
lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
with ESMTP id S229517AbiKERkM (ORCPT
<rfc822;linux-kernel@vger.kernel.org>);
Sat, 5 Nov 2022 13:40:12 -0400
Received: from mga02.intel.com (mga02.intel.com [134.134.136.20])
by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 7EFBB1C938;
Sat, 5 Nov 2022 10:40:11 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple;
d=intel.com; i=@intel.com; q=dns/txt; s=Intel;
t=1667670011; x=1699206011;
h=from:to:cc:subject:date:message-id;
bh=YNstcQSEkgMBc3Az2kOerrMpu3CRH4Immut06G88wYs=;
b=PtTCtXZ8oXDk6ikzs6KvqugGL3cGNR3SgsIQyJd9v3w4YmskIwmDYWLD
cOm+C0muQk6/mtx9ucYmS/IdXUEpRNOGqWgjjAa5FsqOytlm5r6fX8AHp
kVH+D8hqcjMZ3XXIGqpXTF9BxkeiRFk4XcvXU622m4d3lf99ooiA/7zwe
QmqRx+dINADDlhIf29EKrhBMWuVp9ghJSsR0oZb6wVaWsSe++H3xWzY+i
95OsBygpEqkf38YagIm4aE5RHPACvh+QdT03r0Rrex5cOcFGmvrVDfZJ0
KntvFb2rw3dug9rXekkQBa6Olw1pIW313DObA21blaNWR5rSyosmE8LMG
g==;
X-IronPort-AV: E=McAfee;i="6500,9779,10522"; a="297672765"
X-IronPort-AV: E=Sophos;i="5.96,140,1665471600";
d="scan'208";a="297672765"
Received: from orsmga008.jf.intel.com ([10.7.209.65])
by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384;
05 Nov 2022 10:40:10 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=McAfee;i="6500,9779,10522"; a="666729778"
X-IronPort-AV: E=Sophos;i="5.96,140,1665471600";
d="scan'208";a="666729778"
Received: from power-sh.sh.intel.com ([10.239.183.122])
by orsmga008.jf.intel.com with ESMTP; 05 Nov 2022 10:40:09 -0700
From: Zhang Rui <rui.zhang@intel.com>
To: rjw@rjwysocki.net, daniel.lezcano@linaro.org
Cc: linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: [RFC PATCH 1/3] cpuidle: ladder: Fix bogus comparison between s64 and
u64
Date: Sun, 6 Nov 2022 01:42:23 +0800
Message-Id: <20221105174225.28673-1-rui.zhang@intel.com>
X-Mailer: git-send-email 2.17.1
X-Spam-Status: No, score=-5.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE
autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?=
X-GMAIL-THRID: =?utf-8?q?1748680665729162354?=
X-GMAIL-MSGID: =?utf-8?q?1748680665729162354?=
|
| Series |
[RFC,1/3] cpuidle: ladder: Fix bogus comparison between s64 and u64
|
|
Commit Message
Zhang, Rui
Nov. 5, 2022, 5:42 p.m. UTC
ladder_device_state.threshold.promotion_time_ns/demotion_time_ns
are u64 type.
In ladder_select_state(), variable 'last_residency', as calculated by
last_residency = dev->last_residency_ns - drv->states[last_idx].exit_latency_ns
are s64 type, and it can be negative value.
When this happens, comparing between 'last_residency' and
'promotion_time_ns/demotion_time_ns' become bogus. As a result, the
ladder governor promotes or stays with current state errornously.
<idle>-0 [001] d..1. 151.893396: ladder_select_state: last_idx 7, last_residency -373033
<idle>-0 [001] d..1. 151.893399: ladder_select_state: dev->last_residency_ns 106967, drv->states[last_idx].exit_latency_ns 480000
<idle>-0 [001] d..1. 151.893402: ladder_select_state: promote, last_state->threshold.promotion_time_ns 480000
<idle>-0 [001] d..1. 151.893404: ladder_select_state: ---> new state 7
<idle>-0 [001] d..1. 151.893465: ladder_select_state: last_idx 7, last_residency -463800
<idle>-0 [001] d..1. 151.893467: ladder_select_state: dev->last_residency_ns 16200, drv->states[last_idx].exit_latency_ns 480000
<idle>-0 [001] d..1. 151.893468: ladder_select_state: promote, last_state->threshold.promotion_time_ns 480000
<idle>-0 [001] dn.1. 151.893470: ladder_select_state: ---> new state 8
Given that promotion_time_ns/demotion_time_ns are initialized with
cpuidle_state.exit_latency_ns, which is s64 type, and they are used to
compare with 'last_residency', which is also s64 type, there is no
reason to use u64 for promotion_time_ns/demotion_time_ns.
With this patch,
<idle>-0 [001] d..1. 523.578531: ladder_select_state: last_idx 8, last_residency -879453
<idle>-0 [001] d..1. 523.578531: ladder_select_state: dev->last_residency_ns 10547, drv->states[last_idx].exit_latency_ns 890000
<idle>-0 [001] d..1. 523.578532: ladder_select_state: demote , last_state->threshold.demotion_time_ns 890000
<idle>-0 [001] d..1. 523.578532: ladder_select_state: ---> new state 7
<idle>-0 [001] d..1. 523.580220: ladder_select_state: last_idx 7, last_residency -169629
<idle>-0 [001] d..1. 523.580221: ladder_select_state: dev->last_residency_ns 310371, drv->states[last_idx].exit_latency_ns 480000
<idle>-0 [001] d..1. 523.580221: ladder_select_state: demote , last_state->threshold.demotion_time_ns 480000
<idle>-0 [001] d..1. 523.580222: ladder_select_state: ---> new state 6
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
drivers/cpuidle/governors/ladder.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Comments
On Sat, Nov 5, 2022 at 6:40 PM Zhang Rui <rui.zhang@intel.com> wrote: > > ladder_device_state.threshold.promotion_time_ns/demotion_time_ns > are u64 type. > > In ladder_select_state(), variable 'last_residency', as calculated by > > last_residency = dev->last_residency_ns - drv->states[last_idx].exit_latency_ns > > are s64 type, and it can be negative value. The code changes are fine AFAICS, but the description below could be more precise. > When this happens, comparing between 'last_residency' and > 'promotion_time_ns/demotion_time_ns' become bogus. IIUC, what happens is that last_residency is converted to u64 in the comparison expression and that conversion causes it to become a large positive number if it is negative. > As a result, the ladder governor promotes or stays with current state errornously. "promotes or retains the current state erroneously". > > <idle>-0 [001] d..1. 151.893396: ladder_select_state: last_idx 7, last_residency -373033 > <idle>-0 [001] d..1. 151.893399: ladder_select_state: dev->last_residency_ns 106967, drv->states[last_idx].exit_latency_ns 480000 > <idle>-0 [001] d..1. 151.893402: ladder_select_state: promote, last_state->threshold.promotion_time_ns 480000 > <idle>-0 [001] d..1. 151.893404: ladder_select_state: ---> new state 7 > <idle>-0 [001] d..1. 151.893465: ladder_select_state: last_idx 7, last_residency -463800 > <idle>-0 [001] d..1. 151.893467: ladder_select_state: dev->last_residency_ns 16200, drv->states[last_idx].exit_latency_ns 480000 > <idle>-0 [001] d..1. 151.893468: ladder_select_state: promote, last_state->threshold.promotion_time_ns 480000 > <idle>-0 [001] dn.1. 151.893470: ladder_select_state: ---> new state 8 > > Given that promotion_time_ns/demotion_time_ns are initialized with > cpuidle_state.exit_latency_ns, which is s64 type, and they are used to > compare with 'last_residency', which is also s64 type, there is no "they are compared with" > reason to use u64 for promotion_time_ns/demotion_time_ns. "so change them both to be s64". > With this patch, > <idle>-0 [001] d..1. 523.578531: ladder_select_state: last_idx 8, last_residency -879453 > <idle>-0 [001] d..1. 523.578531: ladder_select_state: dev->last_residency_ns 10547, drv->states[last_idx].exit_latency_ns 890000 > <idle>-0 [001] d..1. 523.578532: ladder_select_state: demote , last_state->threshold.demotion_time_ns 890000 > <idle>-0 [001] d..1. 523.578532: ladder_select_state: ---> new state 7 > <idle>-0 [001] d..1. 523.580220: ladder_select_state: last_idx 7, last_residency -169629 > <idle>-0 [001] d..1. 523.580221: ladder_select_state: dev->last_residency_ns 310371, drv->states[last_idx].exit_latency_ns 480000 > <idle>-0 [001] d..1. 523.580221: ladder_select_state: demote , last_state->threshold.demotion_time_ns 480000 > <idle>-0 [001] d..1. 523.580222: ladder_select_state: ---> new state 6 > > Signed-off-by: Zhang Rui <rui.zhang@intel.com> > --- > drivers/cpuidle/governors/ladder.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/cpuidle/governors/ladder.c b/drivers/cpuidle/governors/ladder.c > index 8e9058c4ea63..fb61118aef37 100644 > --- a/drivers/cpuidle/governors/ladder.c > +++ b/drivers/cpuidle/governors/ladder.c > @@ -27,8 +27,8 @@ struct ladder_device_state { > struct { > u32 promotion_count; > u32 demotion_count; > - u64 promotion_time_ns; > - u64 demotion_time_ns; > + s64 promotion_time_ns; > + s64 demotion_time_ns; > } threshold; > struct { > int promotion_count; > --
diff --git a/drivers/cpuidle/governors/ladder.c b/drivers/cpuidle/governors/ladder.c index 8e9058c4ea63..fb61118aef37 100644 --- a/drivers/cpuidle/governors/ladder.c +++ b/drivers/cpuidle/governors/ladder.c @@ -27,8 +27,8 @@ struct ladder_device_state { struct { u32 promotion_count; u32 demotion_count; - u64 promotion_time_ns; - u64 demotion_time_ns; + s64 promotion_time_ns; + s64 demotion_time_ns; } threshold; struct { int promotion_count;