From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753826AbdLSXZx (ORCPT ); Tue, 19 Dec 2017 18:25:53 -0500 Received: from mga04.intel.com ([192.55.52.120]:14058 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752656AbdLSXZu (ORCPT ); Tue, 19 Dec 2017 18:25:50 -0500 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,429,1508828400"; d="scan'208";a="3860870" Subject: Re: [PATCH 2/4] perf/x86/intel: fix event update for auto-reload To: Peter Zijlstra Cc: mingo@redhat.com, acme@kernel.org, linux-kernel@vger.kernel.org, tglx@linutronix.de, jolsa@redhat.com, eranian@google.com, ak@linux.intel.com References: <1513596891-12362-1-git-send-email-kan.liang@linux.intel.com> <1513596891-12362-3-git-send-email-kan.liang@linux.intel.com> <20171219185810.d4yqz6qwqmx3f5o5@hirez.programming.kicks-ass.net> <20171219220709.sq7kwvg7l2ojltvr@hirez.programming.kicks-ass.net> From: "Liang, Kan" Message-ID: <4a69ef66-acc3-c8d6-342d-270be19f201a@linux.intel.com> Date: Tue, 19 Dec 2017 18:25:47 -0500 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: <20171219220709.sq7kwvg7l2ojltvr@hirez.programming.kicks-ass.net> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 12/19/2017 5:07 PM, Peter Zijlstra wrote: > On Tue, Dec 19, 2017 at 03:08:58PM -0500, Liang, Kan wrote: >>> This all looks very wrong... In auto reload we should never call >>> intel_pmu_save_and_restore() in the first place I think. >>> >>> Things like x86_perf_event_update() and x86_perf_event_set_period() >>> simply _cannot_ do the right thing when we auto reload the counter. >>> >> >> I think it should be OK to call it in first place. >> For x86_perf_event_update(), the reload_times will tell if it's auto reload. >> Both period_left and event->count are carefully recalculated for auto >> reload. > > How does prev_count make sense when we've already reloaded a bunch of > times? Same as non-auto-reload, it's the 'left' (unfinished) period from last time. The period for the first record should always be the 'left' period no matter on which case. For auto-reload, it doesn't need to increase the prev_count with the reload. Because for later records, the period should be exactly the same as the reload value. To calculate the event->count, For auto-reload, the event->count = prev_count + (reload times - 1) * reload value + gap between PMI trigger and PMI handler. For non-auto-reload, the event->count = prev_count + gap between PMI trigger and PMI handler. The 'prev_count' is same for both auto-reload and non-auto-reload. The gap is a little bit tricky for auto-reload. Because it starts from -reload_value. But for non-auto-reload, it starts from 0. "delta += (reload_val << shift);" is used to correct it. > >> For x86_perf_event_set_period(), there is nothing special needed for auto >> reload. The period is fixed. The period_left from x86_perf_event_update() is >> already handled. > > Hurm.. I see. But rather than make an ever bigger trainwreck of things, > I'd rather you just write a special purpose intel_pmu_save_and_restart() > just for AUTO_RELOAD. OK. I will do it in V2. Thanks, Kan