linux-kernel - Re: [PATCH v2 3/5] hwtracing: hisi_ptt: Optimize the trace data committing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fd62aa39-78d7-a6c5-7782-8214256ee51b@huawei.com>
Date:   Tue, 19 Sep 2023 21:19:41 +0800
From:   Yicong Yang <yangyicong@...wei.com>
To:     Suzuki K Poulose <suzuki.poulose@....com>,
        <mathieu.poirier@...aro.org>, <jonathan.cameron@...wei.com>,
        <linux-kernel@...r.kernel.org>
CC:     <yangyicong@...ilicon.com>, <alexander.shishkin@...ux.intel.com>,
        <helgaas@...nel.org>, <linux-pci@...r.kernel.org>,
        <prime.zeng@...ilicon.com>, <linuxarm@...wei.com>,
        <hejunhao3@...wei.com>
Subject: Re: [PATCH v2 3/5] hwtracing: hisi_ptt: Optimize the trace data
 committing

On 2023/9/15 22:44, Suzuki K Poulose wrote:
> On 14/09/2023 12:22, Yicong Yang wrote:
>> From: Yicong Yang <yangyicong@...ilicon.com>
>>
>> Currently during the PTT trace, we'll only commit the data
>> to the perf core when its full, which means after 4 interrupts
>> and totally 16MiB data while the AUX buffer is 16MiB length.
>> Then the userspace gets notified and handle the data. The driver
>> cannot apply a new AUX buffer immediately until the committed data
>> are handled and there's enough room in the buffer again.
>>
>> This patch tries to optimize this by commit the data in every
>> interrupts in a 4MiB granularity. Then the userspace can have
>> enough time to consume the data and there's always enough room
>> in the AUX buffer.
> 
> Instead of always committing at 4M, could we not use the existing
> markers used by the handle->wakeup to decide if we should commit it ?
> 

Is it intended to avoid too much wakeups? I think the core will handle
the wakeup even if we commit the data in perf_aux_output_end().
For example, if the userspace buffer is 16MiB and the watermark is 8MiB,
we'll not wake up userspace after the first 4MiB committing.

4MiB mentioned in the commit is our current configuration: hardware
maintains 4 buffers and driver configure each one as 4MiB. Driver will
get interrupt if one buffer filled and then copy the data to the AUX
buffer. We restrict the AUX buffer to be at least 16MiB. Previous we'll
only commit the data if the resident space in the AUX buffer cannot
contain next 4MiB data, typically after copy 4 buffers to AUX buffer.
This is suboptimal because after committing there's no space in the
AUX buffer and driver needs to wait until userspace consumes the data.
So we optimize it in this patch to always commit the data to AUX
buffer in time to avoid waiting.

> 
> Suzuki
> 
>>
>> Signed-off-by: Yicong Yang <yangyicong@...ilicon.com>
>> Acked-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
>> ---
>>   drivers/hwtracing/ptt/hisi_ptt.c | 15 +++++++--------
>>   1 file changed, 7 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/hwtracing/ptt/hisi_ptt.c b/drivers/hwtracing/ptt/hisi_ptt.c
>> index 3041238a6e54..4f355df8da23 100644
>> --- a/drivers/hwtracing/ptt/hisi_ptt.c
>> +++ b/drivers/hwtracing/ptt/hisi_ptt.c
>> @@ -274,15 +274,14 @@ static int hisi_ptt_update_aux(struct hisi_ptt *hisi_ptt, int index, bool stop)
>>       buf->pos += size;
>>         /*
>> -     * Just commit the traced data if we're going to stop. Otherwise if the
>> -     * resident AUX buffer cannot contain the data of next trace buffer,
>> -     * apply a new one.
>> +     * Always commit the data to the AUX buffer in time to make sure
>> +     * userspace got enough time to consume the data.
>> +     *
>> +     * If we're not going to stop, apply a new one and check whether
>> +     * there's enough room for the next trace.
>>        */
>> -    if (stop) {
>> -        perf_aux_output_end(handle, buf->pos);
>> -    } else if (buf->length - buf->pos < HISI_PTT_TRACE_BUF_SIZE) {
>> -        perf_aux_output_end(handle, buf->pos);
>> -
>> +    perf_aux_output_end(handle, size);
>> +    if (!stop) {
>>           buf = perf_aux_output_begin(handle, event);
>>           if (!buf)
>>               return -EINVAL;
> 
> 
> .