Re: [PATCH v6 1/4] ftrace: Implement fs notification for tracing_max_latency

[Viktor Rosendahl]

On 9/8/19 1:38 AM, Joel Fernandes wrote:> On Sat, Sep 07, 2019 at 
11:12:59PM +0200, Viktor Rosendahl wrote:
>> On 9/6/19 4:17 PM, Joel Fernandes wrote:
>>> On Thu, Sep 05, 2019 at 03:25:45PM +0200, Viktor Rosendahl wrote:
>> <clip>
>>>> +
>>>> +__init static int latency_fsnotify_init(void)
>>>> +{
>>>> +	fsnotify_wq = alloc_workqueue("tr_max_lat_wq",
>>>> +				      WQ_UNBOUND | WQ_HIGHPRI, 0);
>>>> +	if (!fsnotify_wq) {
>>>> +		pr_err("Unable to allocate tr_max_lat_wq\n");
>>>> +		return -ENOMEM;
>>>> +	}
>>>
>>> Why not just use the system workqueue instead of adding another 
workqueue?
>>>
>>
>> For the the latency-collector to work properly in the worst case, when a
>> new latency occurs immediately, the fsnotify must be received in less
>> time than what the threshold is set to. If we always are slower we will
>> always lose certain latencies.
>>
>> My intention was to minimize latency in some important cases, so that
>> user space receives the notification sooner rather than later.
>>
>> There doesn't seem to be any system workqueue with WQ_UNBOUND and
>> WQ_HIGHPRI. My thinking was that WQ_UNBOUND might help with the latency
>> in some important cases.
>>
>> If we use:
>>
>> queue_work(system_highpri_wq, &tr->fsnotify_work);
>>
>> then the work will (almost) always execute on the same CPU but if we are
>> unlucky that CPU could be too busy while there could be another CPU in
>> the system that would be able to process the work soon enough.
>>
>> queue_work_on() could be used to queue the work on another CPU but it
>> seems difficult to select the right CPU.
>
> Ok, a separate WQ is fine with me as such since the preempt/irq 
events are on
> a debug kernel anyway.

Keep in mind that this feature is also enabled by the wakeup tracers and by
hwlat. These are often enabled by production kernels.

I guess it would be possible to add some ifdefs so that we create a new
workqueue only if preempt/irqsoff tracing is enabled in the kernel 
config and
use system_highpri_wq if we only have the wakeup and hwlat tracers in the
config.

However, I don't really like adding yet some more ifdefs to the code.

Since a new workqueue will not necessariy create a new worker thread 
nowadays,
I thought that it would be OK with a new unbound workqueue, which should not
add much to the tendency to create more worker threads.

>
> I'll keep reviewing your patches next few days, I am at the LPC 
conference so
> might be a bit slow. Overall I think the series look like its 
maturing and
> getting close.
>

Ok, thanks. Could you let me know when you have looked through it all so 
that
I know when it makes sense to make another reroll of the series?

best regards,

Viktor