Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v6 1/4] ftrace: Implement fs notification for
 tracing_max_latency
To:     Joel Fernandes <joel@joelfernandes.org>
Cc:     Steven Rostedt <rostedt@goodmis.org>,
        Ingo Molnar <mingo@redhat.com>, linux-kernel@vger.kernel.org
References: <20190905132548.5116-1-viktor.rosendahl@gmail.com>
 <20190905132548.5116-2-viktor.rosendahl@gmail.com>
 <20190906141740.GA250796@google.com>
 <c35722db-bb79-7e09-ac02-e82ab827e1e3@gmail.com>
 <20190907233801.GA117656@google.com>
From:   Viktor Rosendahl <viktor.rosendahl@gmail.com>
Message-ID: <955add6c-1465-df65-ddb1-f2b3a05df9d1@gmail.com>
Date:   Sun, 8 Sep 2019 19:05:04 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <20190907233801.GA117656@google.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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