From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752195AbdHHMcT (ORCPT ); Tue, 8 Aug 2017 08:32:19 -0400 Received: from mx1.redhat.com ([209.132.183.28]:17107 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752038AbdHHMcR (ORCPT ); Tue, 8 Aug 2017 08:32:17 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 8614781235 Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=prarit@redhat.com Subject: Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps To: Sergey Senozhatsky References: <1502121162-27981-1-git-send-email-prarit@redhat.com> <20170808001908.GA7765@jagdpanzerIV.localdomain> Cc: linux-kernel@vger.kernel.org, Mark Salyzyn , Jonathan Corbet , Petr Mladek , Sergey Senozhatsky , Steven Rostedt , John Stultz , Thomas Gleixner , Stephen Boyd , Andrew Morton , Greg Kroah-Hartman , "Paul E. McKenney" , Christoffer Dall , Deepa Dinamani , Ingo Molnar , Joel Fernandes , Kees Cook , Peter Zijlstra , Geert Uytterhoeven , "Luis R. Rodriguez" , Nicholas Piggin , "Jason A. Donenfeld" , Olof Johansson , Josh Poimboeuf , linux-doc@vger.kernel.org From: Prarit Bhargava Message-ID: <53f82948-b688-645e-bb2e-155e99241054@redhat.com> Date: Tue, 8 Aug 2017 08:32:12 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: <20170808001908.GA7765@jagdpanzerIV.localdomain> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Tue, 08 Aug 2017 12:32:17 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 08/07/2017 08:19 PM, Sergey Senozhatsky wrote: > On (08/07/17 11:52), Prarit Bhargava wrote: > [..] >> +/** >> + * enum printk_time_type - Timestamp types for printk() messages. >> + * @PRINTK_TIME_DISABLE: No time stamp. >> + * @PRINTK_TIME_LOCAL: Local hardware clock timestamp. >> + * @PRINTK_TIME_BOOT: Boottime clock timestamp. >> + * @PRINTK_TIME_MONO: Monotonic clock timestamp. >> + * @PRINTK_TIME_REAL: Realtime clock timestamp. On 32-bit >> + * systems selecting the real clock printk timestamp may lead to unlikely >> + * situations where a timestamp is wrong because the real time offset is read >> + * without the protection of a sequence lock in the call to ktime_get_log_ts() >> + * in printk_get_ts() below. >> + */ >> +enum printk_time_type { >> + PRINTK_TIME_DISABLE = 0, >> + PRINTK_TIME_LOCAL = 1, >> + PRINTK_TIME_BOOT = 2, >> + PRINTK_TIME_MONO = 3, >> + PRINTK_TIME_REAL = 4, >> +}; > > may be call the entire thing 'timestamp surces' or something? Done. > > [..] >> + if (strlen(param) == 1) { >> + /* Preserve legacy boolean settings */ >> + if (!strcmp("0", param) || !strcmp("n", param) || >> + !strcmp("N", param)) >> + _printk_time = PRINTK_TIME_DISABLE; >> + if (!strcmp("1", param) || !strcmp("y", param) || >> + !strcmp("Y", param)) >> + _printk_time = PRINTK_TIME_LOCAL; >> + } >> + if (_printk_time == -1) { >> + for (stamp = 0; stamp <= 4; stamp++) { >> + if (!strncmp(printk_time_str[stamp], param, >> + strlen(param))) { >> + _printk_time = stamp; >> + break; >> + } >> + } >> + } > > you can use match_string() here. > >> + if (_printk_time == -1) { >> + pr_warn("printk: invalid timestamp value %s\n", param); >> + return -EINVAL; >> + } > > `invalid timestamp value' is confusing. I can't think of anything better than 'invalid timestamp option'. If you find that confusing, please explain why. > > >> + } else if ((printk_time_setting != _printk_time) && >> + (_printk_time != 0)) { >> + pr_warn("printk: timestamp can only be set to 0(disabled) or %s\n", >> + printk_time_str[printk_time_setting]); > > ditto. Changed to 'timestamp can only e set to 0, disabled, or '. > > >> + return -EINVAL; >> + } >> + >> + printk_time = _printk_time; >> + pr_info("printk: timestamp set to %s\n", printk_time_str[printk_time]); > > ditto. I don't find this confusing _at all_. Please offer an explanation of why you think that message is confusing. > > > [..] > >> +static u64 printk_get_ts(void) >> +{ >> + u64 mono, offset_real; >> + >> + if (printk_time <= PRINTK_TIME_LOCAL) >> + return local_clock(); >> + >> + if (printk_time == PRINTK_TIME_BOOT) >> + return ktime_get_boot_log_ts(); >> + >> + mono = ktime_get_real_log_ts(&offset_real); >> + >> + if (printk_time == PRINTK_TIME_MONO) >> + return mono; >> + >> + return mono + offset_real; >> +} > > this looks hard... See previous suggestion from peterz to switch to a fn ptr. > >> +static int printk_time; >> +static int printk_time_setting; > > how about s/printk_time_setting/printk_time_source/? or something similar? Sure. P. > > -ss >