linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/5] Several updates for PTT driver
@ 2023-09-14 11:22 Yicong Yang
  2023-09-14 11:22 ` [PATCH v2 1/5] hwtracing: hisi_ptt: Disable interrupt after trace end Yicong Yang
                   ` (4 more replies)
  0 siblings, 5 replies; 12+ messages in thread
From: Yicong Yang @ 2023-09-14 11:22 UTC (permalink / raw)
  To: suzuki.poulose, mathieu.poirier, jonathan.cameron, linux-kernel
  Cc: alexander.shishkin, helgaas, linux-pci, prime.zeng, linuxarm,
	yangyicong, hejunhao3

From: Yicong Yang <yangyicong@hisilicon.com>

This series contains several updates for PTT driver:
- Disable interrupt when trace stops, reverse to what we do in trace start
- Always handle the interrupt in hardirq context
- Optimize the AUX buffer handling to make consumer have more time to process
  the data
- Since we're a uncore PMU so block any task attach operation
- Add a dummy pmu::read() callback since the perf core may use

Change since v1:
- Add Jonathan's tag, thanks
Link: https://lore.kernel.org/all/20230809081825.11518-1-yangyicong@huawei.com/

Junhao He (1):
  hwtracing: hisi_ptt: Add dummy callback pmu::read()

Yicong Yang (4):
  hwtracing: hisi_ptt: Disable interrupt after trace end
  hwtracing: hisi_ptt: Handle the interrupt in hardirq context
  hwtracing: hisi_ptt: Optimize the trace data committing
  hwtracing: hisi_ptt: Don't try to attach a task

 drivers/hwtracing/ptt/hisi_ptt.c | 33 +++++++++++++++++++++-----------
 drivers/hwtracing/ptt/hisi_ptt.h |  1 +
 2 files changed, 23 insertions(+), 11 deletions(-)

-- 
2.24.0


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 1/5] hwtracing: hisi_ptt: Disable interrupt after trace end
  2023-09-14 11:22 [PATCH v2 0/5] Several updates for PTT driver Yicong Yang
@ 2023-09-14 11:22 ` Yicong Yang
  2023-09-14 11:22 ` [PATCH v2 2/5] hwtracing: hisi_ptt: Handle the interrupt in hardirq context Yicong Yang
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 12+ messages in thread
From: Yicong Yang @ 2023-09-14 11:22 UTC (permalink / raw)
  To: suzuki.poulose, mathieu.poirier, jonathan.cameron, linux-kernel
  Cc: alexander.shishkin, helgaas, linux-pci, prime.zeng, linuxarm,
	yangyicong, hejunhao3

From: Yicong Yang <yangyicong@hisilicon.com>

On trace end we disable the hardware but leave the interrupt
unmasked. Mask the interrupt to make the process reverse to
the start. No actual issue since hardware should send no
interrupt after disabled.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/hwtracing/ptt/hisi_ptt.c | 4 ++++
 drivers/hwtracing/ptt/hisi_ptt.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/hwtracing/ptt/hisi_ptt.c b/drivers/hwtracing/ptt/hisi_ptt.c
index 49ea1b0f7489..428cca54217e 100644
--- a/drivers/hwtracing/ptt/hisi_ptt.c
+++ b/drivers/hwtracing/ptt/hisi_ptt.c
@@ -183,6 +183,10 @@ static void hisi_ptt_wait_dma_reset_done(struct hisi_ptt *hisi_ptt)
 static void hisi_ptt_trace_end(struct hisi_ptt *hisi_ptt)
 {
 	writel(0, hisi_ptt->iobase + HISI_PTT_TRACE_CTRL);
+
+	/* Mask the interrupt on the end */
+	writel(HISI_PTT_TRACE_INT_MASK_ALL, hisi_ptt->iobase + HISI_PTT_TRACE_INT_MASK);
+
 	hisi_ptt->trace_ctrl.started = false;
 }
 
diff --git a/drivers/hwtracing/ptt/hisi_ptt.h b/drivers/hwtracing/ptt/hisi_ptt.h
index e17f045d7e72..46030aa88081 100644
--- a/drivers/hwtracing/ptt/hisi_ptt.h
+++ b/drivers/hwtracing/ptt/hisi_ptt.h
@@ -47,6 +47,7 @@
 #define HISI_PTT_TRACE_INT_STAT		0x0890
 #define   HISI_PTT_TRACE_INT_STAT_MASK	GENMASK(3, 0)
 #define HISI_PTT_TRACE_INT_MASK		0x0894
+#define   HISI_PTT_TRACE_INT_MASK_ALL	GENMASK(3, 0)
 #define HISI_PTT_TUNING_INT_STAT	0x0898
 #define   HISI_PTT_TUNING_INT_STAT_MASK	BIT(0)
 #define HISI_PTT_TRACE_WR_STS		0x08a0
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 2/5] hwtracing: hisi_ptt: Handle the interrupt in hardirq context
  2023-09-14 11:22 [PATCH v2 0/5] Several updates for PTT driver Yicong Yang
  2023-09-14 11:22 ` [PATCH v2 1/5] hwtracing: hisi_ptt: Disable interrupt after trace end Yicong Yang
@ 2023-09-14 11:22 ` Yicong Yang
  2023-09-14 11:22 ` [PATCH v2 3/5] hwtracing: hisi_ptt: Optimize the trace data committing Yicong Yang
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 12+ messages in thread
From: Yicong Yang @ 2023-09-14 11:22 UTC (permalink / raw)
  To: suzuki.poulose, mathieu.poirier, jonathan.cameron, linux-kernel
  Cc: alexander.shishkin, helgaas, linux-pci, prime.zeng, linuxarm,
	yangyicong, hejunhao3

From: Yicong Yang <yangyicong@hisilicon.com>

Handle the trace interrupt in the hardirq context, make sure the irq
core won't threaded it by declaring IRQF_NO_THREAD and userspace won't
balance it by declaring IRQF_NOBALANCING. Otherwise we may violate the
synchronization requirements of the perf core, referenced to the
change of arm-ccn PMU commit 0811ef7e2f54 ("bus: arm-ccn: fix PMU interrupt flags").

In the interrupt handler we mainly doing 2 things:
- Copy the data from the local DMA buffer to the AUX buffer
- Commit the data in the AUX buffer

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/hwtracing/ptt/hisi_ptt.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/hwtracing/ptt/hisi_ptt.c b/drivers/hwtracing/ptt/hisi_ptt.c
index 428cca54217e..3041238a6e54 100644
--- a/drivers/hwtracing/ptt/hisi_ptt.c
+++ b/drivers/hwtracing/ptt/hisi_ptt.c
@@ -346,9 +346,9 @@ static int hisi_ptt_register_irq(struct hisi_ptt *hisi_ptt)
 		return ret;
 
 	hisi_ptt->trace_irq = pci_irq_vector(pdev, HISI_PTT_TRACE_DMA_IRQ);
-	ret = devm_request_threaded_irq(&pdev->dev, hisi_ptt->trace_irq,
-					NULL, hisi_ptt_isr, 0,
-					DRV_NAME, hisi_ptt);
+	ret = devm_request_irq(&pdev->dev, hisi_ptt->trace_irq, hisi_ptt_isr,
+				IRQF_NOBALANCING | IRQF_NO_THREAD, DRV_NAME,
+				hisi_ptt);
 	if (ret) {
 		pci_err(pdev, "failed to request irq %d, ret = %d\n",
 			hisi_ptt->trace_irq, ret);
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 3/5] hwtracing: hisi_ptt: Optimize the trace data committing
  2023-09-14 11:22 [PATCH v2 0/5] Several updates for PTT driver Yicong Yang
  2023-09-14 11:22 ` [PATCH v2 1/5] hwtracing: hisi_ptt: Disable interrupt after trace end Yicong Yang
  2023-09-14 11:22 ` [PATCH v2 2/5] hwtracing: hisi_ptt: Handle the interrupt in hardirq context Yicong Yang
@ 2023-09-14 11:22 ` Yicong Yang
  2023-09-15 14:44   ` Suzuki K Poulose
  2023-09-14 11:22 ` [PATCH v2 4/5] hwtracing: hisi_ptt: Don't try to attach a task Yicong Yang
  2023-09-14 11:22 ` [PATCH v2 5/5] hwtracing: hisi_ptt: Add dummy callback pmu::read() Yicong Yang
  4 siblings, 1 reply; 12+ messages in thread
From: Yicong Yang @ 2023-09-14 11:22 UTC (permalink / raw)
  To: suzuki.poulose, mathieu.poirier, jonathan.cameron, linux-kernel
  Cc: alexander.shishkin, helgaas, linux-pci, prime.zeng, linuxarm,
	yangyicong, hejunhao3

From: Yicong Yang <yangyicong@hisilicon.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.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.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;
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 4/5] hwtracing: hisi_ptt: Don't try to attach a task
  2023-09-14 11:22 [PATCH v2 0/5] Several updates for PTT driver Yicong Yang
                   ` (2 preceding siblings ...)
  2023-09-14 11:22 ` [PATCH v2 3/5] hwtracing: hisi_ptt: Optimize the trace data committing Yicong Yang
@ 2023-09-14 11:22 ` Yicong Yang
  2023-09-14 11:22 ` [PATCH v2 5/5] hwtracing: hisi_ptt: Add dummy callback pmu::read() Yicong Yang
  4 siblings, 0 replies; 12+ messages in thread
From: Yicong Yang @ 2023-09-14 11:22 UTC (permalink / raw)
  To: suzuki.poulose, mathieu.poirier, jonathan.cameron, linux-kernel
  Cc: alexander.shishkin, helgaas, linux-pci, prime.zeng, linuxarm,
	yangyicong, hejunhao3

From: Yicong Yang <yangyicong@hisilicon.com>

PTT is an uncore PMU and shouldn't be attached to any task. Block
the usage in pmu::event_init().

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/hwtracing/ptt/hisi_ptt.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hwtracing/ptt/hisi_ptt.c b/drivers/hwtracing/ptt/hisi_ptt.c
index 4f355df8da23..62a444f5228e 100644
--- a/drivers/hwtracing/ptt/hisi_ptt.c
+++ b/drivers/hwtracing/ptt/hisi_ptt.c
@@ -1003,6 +1003,9 @@ static int hisi_ptt_pmu_event_init(struct perf_event *event)
 		return -EOPNOTSUPP;
 	}
 
+	if (event->attach_state & PERF_ATTACH_TASK)
+		return -EOPNOTSUPP;
+
 	if (event->attr.type != hisi_ptt->hisi_ptt_pmu.type)
 		return -ENOENT;
 
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 5/5] hwtracing: hisi_ptt: Add dummy callback pmu::read()
  2023-09-14 11:22 [PATCH v2 0/5] Several updates for PTT driver Yicong Yang
                   ` (3 preceding siblings ...)
  2023-09-14 11:22 ` [PATCH v2 4/5] hwtracing: hisi_ptt: Don't try to attach a task Yicong Yang
@ 2023-09-14 11:22 ` Yicong Yang
  2023-09-15 12:53   ` Suzuki K Poulose
  4 siblings, 1 reply; 12+ messages in thread
From: Yicong Yang @ 2023-09-14 11:22 UTC (permalink / raw)
  To: suzuki.poulose, mathieu.poirier, jonathan.cameron, linux-kernel
  Cc: alexander.shishkin, helgaas, linux-pci, prime.zeng, linuxarm,
	yangyicong, hejunhao3

From: Junhao He <hejunhao3@huawei.com>

When start trace with perf option "-C $cpu" and immediately stop it
with SIGTERM or others, the perf core will invoke pmu::read() while
the driver doesn't implement it. Add a dummy pmu::read() to avoid
any issues.

Signed-off-by: Junhao He <hejunhao3@huawei.com>
Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/hwtracing/ptt/hisi_ptt.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/hwtracing/ptt/hisi_ptt.c b/drivers/hwtracing/ptt/hisi_ptt.c
index 62a444f5228e..c1b5fd2b8974 100644
--- a/drivers/hwtracing/ptt/hisi_ptt.c
+++ b/drivers/hwtracing/ptt/hisi_ptt.c
@@ -1184,6 +1184,10 @@ static void hisi_ptt_pmu_del(struct perf_event *event, int flags)
 	hisi_ptt_pmu_stop(event, PERF_EF_UPDATE);
 }
 
+static void hisi_ptt_pmu_read(struct perf_event *event)
+{
+}
+
 static void hisi_ptt_remove_cpuhp_instance(void *hotplug_node)
 {
 	cpuhp_state_remove_instance_nocalls(hisi_ptt_pmu_online, hotplug_node);
@@ -1227,6 +1231,7 @@ static int hisi_ptt_register_pmu(struct hisi_ptt *hisi_ptt)
 		.stop		= hisi_ptt_pmu_stop,
 		.add		= hisi_ptt_pmu_add,
 		.del		= hisi_ptt_pmu_del,
+		.read		= hisi_ptt_pmu_read,
 	};
 
 	reg = readl(hisi_ptt->iobase + HISI_PTT_LOCATION);
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 5/5] hwtracing: hisi_ptt: Add dummy callback pmu::read()
  2023-09-14 11:22 ` [PATCH v2 5/5] hwtracing: hisi_ptt: Add dummy callback pmu::read() Yicong Yang
@ 2023-09-15 12:53   ` Suzuki K Poulose
  2023-09-19 13:03     ` Yicong Yang
  0 siblings, 1 reply; 12+ messages in thread
From: Suzuki K Poulose @ 2023-09-15 12:53 UTC (permalink / raw)
  To: Yicong Yang, mathieu.poirier, jonathan.cameron, linux-kernel
  Cc: alexander.shishkin, helgaas, linux-pci, prime.zeng, linuxarm,
	yangyicong, hejunhao3

On 14/09/2023 12:22, Yicong Yang wrote:
> From: Junhao He <hejunhao3@huawei.com>
> 
> When start trace with perf option "-C $cpu" and immediately stop it
> with SIGTERM or others, the perf core will invoke pmu::read() while
> the driver doesn't implement it. Add a dummy pmu::read() to avoid
> any issues.

What issues are we talking about here ? Shouldn't the core perf
skip the call, if pmu::read() is not available ?

Suzuki

> 
> Signed-off-by: Junhao He <hejunhao3@huawei.com>
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>   drivers/hwtracing/ptt/hisi_ptt.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/hwtracing/ptt/hisi_ptt.c b/drivers/hwtracing/ptt/hisi_ptt.c
> index 62a444f5228e..c1b5fd2b8974 100644
> --- a/drivers/hwtracing/ptt/hisi_ptt.c
> +++ b/drivers/hwtracing/ptt/hisi_ptt.c
> @@ -1184,6 +1184,10 @@ static void hisi_ptt_pmu_del(struct perf_event *event, int flags)
>   	hisi_ptt_pmu_stop(event, PERF_EF_UPDATE);
>   }
>   
> +static void hisi_ptt_pmu_read(struct perf_event *event)
> +{
> +}
> +
>   static void hisi_ptt_remove_cpuhp_instance(void *hotplug_node)
>   {
>   	cpuhp_state_remove_instance_nocalls(hisi_ptt_pmu_online, hotplug_node);
> @@ -1227,6 +1231,7 @@ static int hisi_ptt_register_pmu(struct hisi_ptt *hisi_ptt)
>   		.stop		= hisi_ptt_pmu_stop,
>   		.add		= hisi_ptt_pmu_add,
>   		.del		= hisi_ptt_pmu_del,
> +		.read		= hisi_ptt_pmu_read,
>   	};
>   
>   	reg = readl(hisi_ptt->iobase + HISI_PTT_LOCATION);


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 3/5] hwtracing: hisi_ptt: Optimize the trace data committing
  2023-09-14 11:22 ` [PATCH v2 3/5] hwtracing: hisi_ptt: Optimize the trace data committing Yicong Yang
@ 2023-09-15 14:44   ` Suzuki K Poulose
  2023-09-19 13:19     ` Yicong Yang
  0 siblings, 1 reply; 12+ messages in thread
From: Suzuki K Poulose @ 2023-09-15 14:44 UTC (permalink / raw)
  To: Yicong Yang, mathieu.poirier, jonathan.cameron, linux-kernel
  Cc: alexander.shishkin, helgaas, linux-pci, prime.zeng, linuxarm,
	yangyicong, hejunhao3

On 14/09/2023 12:22, Yicong Yang wrote:
> From: Yicong Yang <yangyicong@hisilicon.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 ?


Suzuki

> 
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.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;


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 5/5] hwtracing: hisi_ptt: Add dummy callback pmu::read()
  2023-09-15 12:53   ` Suzuki K Poulose
@ 2023-09-19 13:03     ` Yicong Yang
  2023-09-19 17:01       ` Suzuki K Poulose
  0 siblings, 1 reply; 12+ messages in thread
From: Yicong Yang @ 2023-09-19 13:03 UTC (permalink / raw)
  To: Suzuki K Poulose, mathieu.poirier, jonathan.cameron, linux-kernel
  Cc: yangyicong, alexander.shishkin, helgaas, linux-pci, prime.zeng,
	linuxarm, hejunhao3

On 2023/9/15 20:53, Suzuki K Poulose wrote:
> On 14/09/2023 12:22, Yicong Yang wrote:
>> From: Junhao He <hejunhao3@huawei.com>
>>
>> When start trace with perf option "-C $cpu" and immediately stop it
>> with SIGTERM or others, the perf core will invoke pmu::read() while
>> the driver doesn't implement it. Add a dummy pmu::read() to avoid
>> any issues.
> 
> What issues are we talking about here ? Shouldn't the core perf
> skip the call, if pmu::read() is not available ?
> 

Actually no, the core doesn't check it. So I think that's why some PMUs
like SPE implements a dummy pmu::read() callback. Otherwise we'll
dereference a NULL pointer.

Currently we only met this on emulated platforms with very slow CPUs,
follow the instructions in the commit above.

> Suzuki
> 
>>
>> Signed-off-by: Junhao He <hejunhao3@huawei.com>
>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>> Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> ---
>>   drivers/hwtracing/ptt/hisi_ptt.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/hwtracing/ptt/hisi_ptt.c b/drivers/hwtracing/ptt/hisi_ptt.c
>> index 62a444f5228e..c1b5fd2b8974 100644
>> --- a/drivers/hwtracing/ptt/hisi_ptt.c
>> +++ b/drivers/hwtracing/ptt/hisi_ptt.c
>> @@ -1184,6 +1184,10 @@ static void hisi_ptt_pmu_del(struct perf_event *event, int flags)
>>       hisi_ptt_pmu_stop(event, PERF_EF_UPDATE);
>>   }
>>   +static void hisi_ptt_pmu_read(struct perf_event *event)
>> +{
>> +}
>> +
>>   static void hisi_ptt_remove_cpuhp_instance(void *hotplug_node)
>>   {
>>       cpuhp_state_remove_instance_nocalls(hisi_ptt_pmu_online, hotplug_node);
>> @@ -1227,6 +1231,7 @@ static int hisi_ptt_register_pmu(struct hisi_ptt *hisi_ptt)
>>           .stop        = hisi_ptt_pmu_stop,
>>           .add        = hisi_ptt_pmu_add,
>>           .del        = hisi_ptt_pmu_del,
>> +        .read        = hisi_ptt_pmu_read,
>>       };
>>         reg = readl(hisi_ptt->iobase + HISI_PTT_LOCATION);
> 
> 
> .

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 3/5] hwtracing: hisi_ptt: Optimize the trace data committing
  2023-09-15 14:44   ` Suzuki K Poulose
@ 2023-09-19 13:19     ` Yicong Yang
  0 siblings, 0 replies; 12+ messages in thread
From: Yicong Yang @ 2023-09-19 13:19 UTC (permalink / raw)
  To: Suzuki K Poulose, mathieu.poirier, jonathan.cameron, linux-kernel
  Cc: yangyicong, alexander.shishkin, helgaas, linux-pci, prime.zeng,
	linuxarm, hejunhao3

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 5/5] hwtracing: hisi_ptt: Add dummy callback pmu::read()
  2023-09-19 13:03     ` Yicong Yang
@ 2023-09-19 17:01       ` Suzuki K Poulose
  2023-09-20  7:10         ` Yicong Yang
  0 siblings, 1 reply; 12+ messages in thread
From: Suzuki K Poulose @ 2023-09-19 17:01 UTC (permalink / raw)
  To: Yicong Yang, mathieu.poirier, jonathan.cameron, linux-kernel
  Cc: yangyicong, alexander.shishkin, helgaas, linux-pci, prime.zeng,
	linuxarm, hejunhao3

On 19/09/2023 14:03, Yicong Yang wrote:
> On 2023/9/15 20:53, Suzuki K Poulose wrote:
>> On 14/09/2023 12:22, Yicong Yang wrote:
>>> From: Junhao He <hejunhao3@huawei.com>
>>>
>>> When start trace with perf option "-C $cpu" and immediately stop it
>>> with SIGTERM or others, the perf core will invoke pmu::read() while
>>> the driver doesn't implement it. Add a dummy pmu::read() to avoid
>>> any issues.
>>
>> What issues are we talking about here ? Shouldn't the core perf
>> skip the call, if pmu::read() is not available ?
>>
> 
> Actually no, the core doesn't check it. So I think that's why some PMUs
> like SPE implements a dummy pmu::read() callback. Otherwise we'll
> dereference a NULL pointer.
> 
> Currently we only met this on emulated platforms with very slow CPUs,
> follow the instructions in the commit above.

Ok, then it calls for a Fixes tag. Please tag it to the commit that
introduced the PMU.

Suzuki


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 5/5] hwtracing: hisi_ptt: Add dummy callback pmu::read()
  2023-09-19 17:01       ` Suzuki K Poulose
@ 2023-09-20  7:10         ` Yicong Yang
  0 siblings, 0 replies; 12+ messages in thread
From: Yicong Yang @ 2023-09-20  7:10 UTC (permalink / raw)
  To: Suzuki K Poulose, mathieu.poirier, jonathan.cameron, linux-kernel
  Cc: yangyicong, alexander.shishkin, helgaas, linux-pci, prime.zeng,
	linuxarm, hejunhao3

On 2023/9/20 1:01, Suzuki K Poulose wrote:
> On 19/09/2023 14:03, Yicong Yang wrote:
>> On 2023/9/15 20:53, Suzuki K Poulose wrote:
>>> On 14/09/2023 12:22, Yicong Yang wrote:
>>>> From: Junhao He <hejunhao3@huawei.com>
>>>>
>>>> When start trace with perf option "-C $cpu" and immediately stop it
>>>> with SIGTERM or others, the perf core will invoke pmu::read() while
>>>> the driver doesn't implement it. Add a dummy pmu::read() to avoid
>>>> any issues.
>>>
>>> What issues are we talking about here ? Shouldn't the core perf
>>> skip the call, if pmu::read() is not available ?
>>>
>>
>> Actually no, the core doesn't check it. So I think that's why some PMUs
>> like SPE implements a dummy pmu::read() callback. Otherwise we'll
>> dereference a NULL pointer.
>>
>> Currently we only met this on emulated platforms with very slow CPUs,
>> follow the instructions in the commit above.
> 
> Ok, then it calls for a Fixes tag. Please tag it to the commit that
> introduced the PMU.
> 

Sure. I'll add the tag in v3.

Thanks.

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2023-09-20  7:10 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-14 11:22 [PATCH v2 0/5] Several updates for PTT driver Yicong Yang
2023-09-14 11:22 ` [PATCH v2 1/5] hwtracing: hisi_ptt: Disable interrupt after trace end Yicong Yang
2023-09-14 11:22 ` [PATCH v2 2/5] hwtracing: hisi_ptt: Handle the interrupt in hardirq context Yicong Yang
2023-09-14 11:22 ` [PATCH v2 3/5] hwtracing: hisi_ptt: Optimize the trace data committing Yicong Yang
2023-09-15 14:44   ` Suzuki K Poulose
2023-09-19 13:19     ` Yicong Yang
2023-09-14 11:22 ` [PATCH v2 4/5] hwtracing: hisi_ptt: Don't try to attach a task Yicong Yang
2023-09-14 11:22 ` [PATCH v2 5/5] hwtracing: hisi_ptt: Add dummy callback pmu::read() Yicong Yang
2023-09-15 12:53   ` Suzuki K Poulose
2023-09-19 13:03     ` Yicong Yang
2023-09-19 17:01       ` Suzuki K Poulose
2023-09-20  7:10         ` Yicong Yang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).