lkml.org 
[lkml]   [2018]   [Jul]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v2 09/10] coresight: perf: Remove set_buffer call back
On Fri, 20 Jul 2018 at 03:04, Suzuki K Poulose <Suzuki.Poulose@arm.com> 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.

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 *.

Thanks,
Mathieu

>
> Cheers
> Suzuki

\
 
 \ /
  Last update: 2018-07-23 20:23    [W:0.070 / U:1.472 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site