Re: [PATCH] v4l2-dev: Add tracepoints for QBUF and DQBUF

[Hans Verkuil <hverkuil@xs4all.nl>]

On 12/10/2013 09:53 PM, Mauro Carvalho Chehab wrote:
> Em Sat, 23 Nov 2013 17:54:48 +0100
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
>> On 11/23/2013 05:30 PM, Sylwester Nawrocki wrote:
>>> Hi,
>>>
>>> On 11/23/2013 12:25 PM, Hans Verkuil wrote:
>>>> Hi Wade,
>>>>
>>>> On 11/22/2013 08:48 PM, Wade Farnsworth wrote:
>>>>> Add tracepoints to the QBUF and DQBUF ioctls to enable rudimentary
>>>>> performance measurements using standard kernel tracers.
>>>>>
>>>>> Signed-off-by: Wade Farnsworth<wade_farnsworth@mentor.com>
>>>>> ---
>>>>>
>>>>> This is the update to the RFC patch I posted a few weeks back.  I've added
>>>>> several bits of metadata to the tracepoint output per Mauro's suggestion.
>>>>
>>>> I don't like this. All v4l2 ioctls can already be traced by doing e.g.
>>>> echo 1 (or echo 2)>/sys/class/video4linux/video0/debug.
>>>>
>>>> So this code basically duplicates that functionality. It would be nice to be able
>>>> to tie in the existing tracing code (v4l2-ioctl.c) into tracepoints.
>>>
>>> I think it would be really nice to have this kind of support for standard
>>> traces at the v4l2 subsystem. Presumably it could even gradually replace
>>> the v4l2 custom debug infrastructure.
>>>
>>> If I understand things correctly, the current tracing/profiling 
>>> infrastructure
>>> is much less invasive than inserting printks all over, which may cause 
>>> changes
>>> in control flow. I doubt the system could be reliably profiled by 
>>> enabling all
>>> those debug prints.
>>>
>>> So my vote would be to add support for standard tracers, like in other
>>> subsystems in the kernel.
>>
>> The reason for the current system is to trace which ioctls are called in
>> what order by a misbehaving application. It's very useful for that,
>> especially when trying to debug user problems.
>>
>> I don't mind switching to tracepoints as long as this functionality is
>> kept one way or another.
> 
> I agree with Sylwester: we should move to tracepoints, and this is a good
> start.

As I mentioned, I don't mind switching to tracepoints, but not in the way the
current patch does it. I certainly don't agree with you merging this patch
as-is without further discussion.

To make it official:

Nacked-by: Hans Verkuil <hans.verkuil@cisco.com>

If we do tracepoints, then we do it right and for all ioctls, not in this
half-baked manner.

Please revert.

Regards,

	Hans