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

[Yicong Yang]

On 2023/9/15 22:44, Suzuki K Poulose wrote:
> On 14/09/2023 12:22, Yicong Yang wrote:
>> From: Yicong Yang <yangyicong@xxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxx>
>> Acked-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
>> ---
>>   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;
> 
> 
> .