Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 03/22] perf/x86/intel: Support adaptive PEBSv4
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     acme@kernel.org, mingo@redhat.com, linux-kernel@vger.kernel.org,
        tglx@linutronix.de, jolsa@kernel.org, eranian@google.com,
        alexander.shishkin@linux.intel.com, ak@linux.intel.com
References: <20190318214144.4639-1-kan.liang@linux.intel.com>
 <20190318214144.4639-4-kan.liang@linux.intel.com>
 <20190319144748.GH5996@hirez.programming.kicks-ass.net>
From:   "Liang, Kan" <kan.liang@linux.intel.com>
Message-ID: <948ed187-bfe8-c8f2-83b1-88db7af88c03@linux.intel.com>
Date:   Tue, 19 Mar 2019 17:20:04 -0400
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.5.3
MIME-Version: 1.0
In-Reply-To: <20190319144748.GH5996@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
Precedence: bulk


On 3/19/2019 10:47 AM, Peter Zijlstra wrote:
>> @@ -933,6 +998,19 @@ pebs_update_state(bool needed_cb, struct cpu_hw_events *cpuc, struct pmu *pmu)
>>   		update = true;
>>   	}
>>   
>> +	if (x86_pmu.intel_cap.pebs_baseline && add) {
>> +		u64 pebs_data_cfg;
>> +
>> +		pebs_data_cfg = pebs_update_adaptive_cfg(event);
>> +
>> +		/* Update pebs_record_size if new event requires more data. */
>> +		if (pebs_data_cfg & ~cpuc->pebs_data_cfg) {
>> +			cpuc->pebs_data_cfg |= pebs_data_cfg;
>> +			adaptive_pebs_record_size_update();
>> +			update = true;
>> +		}
>> +	}
>> +
>>   	if (update)
>>   		pebs_update_threshold(cpuc);
>>   }
> Hurmph.. this only grows the PEBS record.
>

Yes, the PEBS record doesn't shrink on the del. Because we have to go 
through all the existing pebs events for an accurate config. I think it 
doesn't worth it. There is no harmful for a bigger PEBS record, except 
little performance impacts. But that's rare case. For most cases, we 
usually apply the same pebs config for all pebs events.

> 
>> @@ -947,7 +1025,7 @@ void intel_pmu_pebs_add(struct perf_event *event)
>>   	if (hwc->flags & PERF_X86_EVENT_LARGE_PEBS)
>>   		cpuc->n_large_pebs++;
>>   
>> -	pebs_update_state(needed_cb, cpuc, event->ctx->pmu);
>> +	pebs_update_state(needed_cb, cpuc, event, true);
>>   }
>>   
>>   void intel_pmu_pebs_enable(struct perf_event *event)
>> @@ -965,6 +1043,14 @@ void intel_pmu_pebs_enable(struct perf_event *event)
>>   	else if (event->hw.flags & PERF_X86_EVENT_PEBS_ST)
>>   		cpuc->pebs_enabled |= 1ULL << 63;
>>   
>> +	if (x86_pmu.intel_cap.pebs_baseline) {
>> +		hwc->config |= ICL_EVENTSEL_ADAPTIVE;
>> +		if (cpuc->pebs_data_cfg != cpuc->active_pebs_data_cfg) {
>> +			wrmsrl(MSR_PEBS_DATA_CFG, cpuc->pebs_data_cfg);
>> +			cpuc->active_pebs_data_cfg = cpuc->pebs_data_cfg;
>> +		}
>> +	}
>> +
>>   	/*
>>   	 * Use auto-reload if possible to save a MSR write in the PMI.
>>   	 * This must be done in pmu::start(), because PERF_EVENT_IOC_PERIOD.
>> @@ -991,7 +1077,12 @@ void intel_pmu_pebs_del(struct perf_event *event)
>>   	if (hwc->flags & PERF_X86_EVENT_LARGE_PEBS)
>>   		cpuc->n_large_pebs--;
>>   
>> -	pebs_update_state(needed_cb, cpuc, event->ctx->pmu);
>> +	/* Clear both pebs_data_cfg and pebs_record_size for first PEBS. */
> Weird comment..
> 
>> +	if (x86_pmu.intel_cap.pebs_baseline && !cpuc->n_pebs) {
>> +		cpuc->pebs_data_cfg = 0;
>> +		cpuc->pebs_record_size = sizeof(struct pebs_basic);
>> +	}
>> +	pebs_update_state(needed_cb, cpuc, event, false);
> Why do we have to reset record_size? That'll be updated in
> pebs_update_state() on the next add.
> 

The record_size should be reset for the first PEBS events.
Right, I can move the reset in pebs_update_state().


Thanks,
Kan