Messages in this thread | | | From | Mathieu Poirier <> | Date | Mon, 23 Jul 2018 12:22:21 -0600 | Subject | Re: [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
| |