Re: [PATCH v2 09/10] coresight: perf: Remove set_buffer call back

[Mathieu Poirier]

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.

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