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

[Suzuki K Poulose <suzuki.poulose@arm.com>]

On 07/24/2018 09:08 PM, Mathieu Poirier wrote:
> On Mon, 23 Jul 2018 at 16:27, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>>
>> Mathieu,
>>
>> On 07/23/2018 07:22 PM, Mathieu Poirier wrote:
>>> 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.
>>
>> 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.
> 
> We are in total agreement :o)
> 
> All I hoping for is to see the sentence "But with ETR, we need to keep
> track of the buffer details and need to be cleaned up if we fail."
> removed from the changelog.  To me that sounds like you're adding
> cleanup work in enable() which isn't the case.  On the flip side you
> are making the sink configuration atomic and that is the important
> part.

:-), I will update the change log accordingly.

> 
>>
>>>
>>> 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 ju


>>>>>> +    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 ?
> 
> I've been thinking further on this and keeping things the way you've
> implemented them is probably best at this time.  I'm currently adding
> support for PMU HW configuration and in the end we may need the
> information conveyed by event->hw.drv_config [1].  When it's all said
> and done we can clean things up if need be.

Ok.


Thanks for the review.

Cheers
Suzuki