From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753211AbbJaKHv (ORCPT ); Sat, 31 Oct 2015 06:07:51 -0400 Received: from szxga01-in.huawei.com ([58.251.152.64]:45767 "EHLO szxga01-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752784AbbJaKHu (ORCPT ); Sat, 31 Oct 2015 06:07:50 -0400 Subject: Re: [PATCH 1/2] clocksource: replace cycle_last validation with an equal way To: Thomas Gleixner References: <1445952073-7260-1-git-send-email-yangyingliang@huawei.com> <1445952073-7260-2-git-send-email-yangyingliang@huawei.com> CC: , From: Yang Yingliang Message-ID: <563492EA.90206@huawei.com> Date: Sat, 31 Oct 2015 18:07:38 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.177.19.219] X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2015/10/30 22:56, Thomas Gleixner wrote: > Yang, > > On Tue, 27 Oct 2015, Yang Yingliang wrote: > >> Mask the cycle values before subtraction. So we can use this >> validation while the clocksource mask is not 64-bits. >> >> Signed-off-by: Yang Yingliang >> Cc: Thomas Gleixner >> --- >> kernel/time/timekeeping_internal.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/kernel/time/timekeeping_internal.h b/kernel/time/timekeeping_internal.h >> index 4ea005a..984f02e 100644 >> --- a/kernel/time/timekeeping_internal.h >> +++ b/kernel/time/timekeeping_internal.h >> @@ -15,7 +15,7 @@ extern void tk_debug_account_sleep_time(struct timespec64 *t); >> #ifdef CONFIG_CLOCKSOURCE_VALIDATE_LAST_CYCLE >> static inline cycle_t clocksource_delta(cycle_t now, cycle_t last, cycle_t mask) >> { >> - cycle_t ret = (now - last) & mask; >> + cycle_t ret = (now & mask) - (last & mask); > > I agree the original code is broken for all masks which are != > CLOCKSOURCE_MASK(64). > > But your change does not work for actual wraparounds. You probably > cannot trigger it for the 56bits of the arm architected timer, but > that does not make it more correct. > > Assume a CLOCKSOURCE_MASK(32) and that the timer wrapped around since > we last read it. > > last = 0xffffffff > now = 0x01 > > So: > > ret = (0x01 & 0xffffffff) - (0xffffffff & 0xffffffff); > --> ret = 0x01 - 0xffffffff; > --> ret = ffffffff00000002; > > --> (s64) ret is < 0 !!! > > This is wrong as the clocksource legitimately wrapped around since we > accessed it last. > > The correct solution to this is: > > ret = (now - last) & mask; > > negative = ret & ~(mask >> 1); > > return negative ? 0 : ret; Thanks for your advise. I will resend this patch. Regards, Yang