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

[Viktor Rosendahl <viktor.rosendahl@gmail.com>]

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