Re: [PATCH v2 09/10] coresight: perf: Remove set_buffer call back
From: Suzuki K Poulose
Date: Mon Jul 23 2018 - 18:27:39 EST
Mathieu,
On 07/23/2018 07:22 PM, Mathieu Poirier wrote:
On Fri, 20 Jul 2018 at 03:04, Suzuki K Poulose <Suzuki.Poulose@xxxxxxx> wrote:
Mathieu,
On 19/07/18 21:36, Mathieu Poirier wrote:
On Tue, Jul 17, 2018 at 06:11:40PM +0100, Suzuki K Poulose wrote:
In coresight perf mode, we need to prepare the sink before
starting a session, which is done via set_buffer call back.
We then proceed to enable the tracing. If we fail to start
the session successfully, we leave the sink configuration
unchanged. This was fine for the existing backends as they
don't have any state associated with the buffers. But with
ETR, we need to keep track of the buffer details and need
to be cleaned up if we fail. In order to make the operation
atomic and to avoid yet another call back, we get rid of
the "set_buffer" call back and pass the buffer details
via enable() call back to the sink.
Suzuki,
I'm not sure I understand the problem you're trying to fix there. From the
implementation of tmc_enable_etr_sink_perf() in the next patch, wouldn't the
same result been achievable using a callback?
We can definitely achieve the results using "set_buffer". But for ETR,
we track the "perf_buf" in drvdata->perf_data when we do "set_buffer".
But if we failed to enable_path(), we leave the drvdata->perf_data
and doesn't clean it up. Now when another session is about to set_buf,
we check if perf_data is empty and WARNs otherwise.
Because we can't be sure if it belongs to an abandoned session or
another active session and we completely messed somewhere in the driver.
So, we need a clear_buffer call back if the enable fails, something
not really worth. Anyways, there is no point in separating set_buffer
and enabling the sink, as the error handling becomes cumbersome as explained
above.
I'm fine with this patch and supportive of getting rid of callbacks if we can, I
just need to understand the exact problem you're after. From looking a your
code (and the current implementation), if we succeed in setting the memory for
the sink but fail in any of the subsequent steps i.e, enabling the rest of the
compoment on the path or the source, the sink is left unchanged.
Yes, thats right. And we should WARN (which I missed in this version) if
there is a perf_data already for a disabled ETR. Please see my response to the
next patch.
The changelog for this patch states the following: "But with ETR, we
need to keep track of the buffer details and need to be cleaned up if
we fail."
I did a deep dive in the code and in the current implementation if the
source fails to be enabled in etm_event_start() the path and the sink
remains unchanged. With your patchset this get fixed because a goto
was added to disable the path when such condition occur. As such each
component in the path will see its ->disable() callback invoked. In
tmc_disable_etr_sink(), drvdata->perf_data is set to NULL in
tmc_etr_disable_hw(), so the cleanup on error condition is done
properly. As such we wouldn't need a clean_buffer() callback.
All of this is right. But we still have a case. e.g, if the ETR is
enabled in sysfs mode, coresight_enable_path() will fail after we
have set the buffer. And since we don't try to disable the path
when we fail at SINK (which is the right thing to do, as we could
be potentially disabling the ETR operated in sysfs mode), we leave
the perf_data around. And the next session finds a non-empty data.
As I said I'm in favour of removing the set_buffer() callback but I
wouldn't associated it with ETR state cleanup. If the code can be
rearranged in a way that code can be removed then that alone is enough
to justify the change.
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index 3cc4a0b..12a247d 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -269,16 +269,11 @@ static void etm_event_start(struct perf_event *event, int flags)
path = etm_event_cpu_path(event_data, cpu);
/* We need a sink, no need to continue without one */
sink = coresight_get_sink(path);
- if (WARN_ON_ONCE(!sink || !sink_ops(sink)->set_buffer))
- goto fail_end_stop;
-
- /* Configure the sink */
- if (sink_ops(sink)->set_buffer(sink, handle,
- event_data->snk_config))
+ if (WARN_ON_ONCE(!sink))
goto fail_end_stop;
/* Nothing will happen without a path */
- if (coresight_enable_path(path, CS_MODE_PERF))
+ if (coresight_enable_path(path, CS_MODE_PERF, handle))
Here we already have a handle on "event_data". As such I think this is what we
should feed to coresight_enable_path() rather than the handle. That way we
don't need to call etm_perf_sink_config(), we just use the data.
The advantage of passing on the handle is, we could get all the way upto the
"perf_event" for the given session. Passing the event_data will loose that
information.
i.e, perf_event-> |perf_ouput_handle | -> |event_data | -> sink_config
| <-event | | |
The purpose of the wrapper "etm_perf_sink_config()" is to abstract the way we
handle the information under the event_data. i.e, if we decide to make some
changes in the way we store event_data, we need to spill the changes every
where. But the perf_ouput_handle has much more stable ABI than event_data,
hence the choice of passing handle.
I agree that etm_perf_sink_config() has value but it should take a
void * as parameter (i.e what gets returned from perf_get_aux())
rather than a perf_output_handle *.
Ok, did you mean :
sink_config = etm_perf_sink_config(perf_get_aux(handle)); ?
Or do you want to change the parameter passed to the enable_sink() as
well ?
Suzuki