Date:   Thu, 26 Apr 2018 08:20:03 -0700
From:   Joel Fernandes <joelaf@...gle.com>
To:     Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Cc:     "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
        rostedt <rostedt@...dmis.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linux-rt-users <linux-rt-users@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Tom Zanussi <tom.zanussi@...ux.intel.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Boqun Feng <boqun.feng@...il.com>,
        fweisbec <fweisbec@...il.com>,
        Randy Dunlap <rdunlap@...radead.org>,
        kbuild test robot <fengguang.wu@...el.com>,
        baohong liu <baohong.liu@...el.com>,
        vedang patel <vedang.patel@...el.com>,
        kernel-team <kernel-team@....com>,
        "Cc: Android Kernel" <kernel-team@...roid.com>
Subject: Re: [RFC v4 3/4] irqflags: Avoid unnecessary calls to trace_ if you can

On Thu, Apr 26, 2018 at 8:13 AM, Mathieu Desnoyers
<mathieu.desnoyers@...icios.com> wrote:
[...]
>>> Regarding the name, I'm OK with having something along the lines of
>>> trace_*event*_blocking or such. Please don't use "srcu" or other naming
>>> that is explicitly tied to the underlying mechanism used internally
>>> however: what we want to convey is that this specific tracepoint probe
>>
>> Problem is that _blocking isn't the right word either. In my IRQ trace
>> point case, it will look something like this then:
>>
>> local_irq_disable();
>> // IRQs are now off.
>> trace_irq_disable_blocking(..);
>>
>> This wouldn't make sense. What we really want is to use the SRCU
>> implementation so that its low overhead...
>>
>> So it would be something like:
>>
>> local_irq_disable();
>> // IRQs are now off.
>> trace_irq_disable_srcu(..);
>>
>> I also Ok if, as Paul was saying in his last email, that just for
>> _rcuidle, we use SRCU so that we don't have to do the rcu_enter_irq
>> stuff. Or we kill the _rcuidle API completely and use _srcu for those
>> users instead. We already have 1 implementation specific name anyway
>> (rcuidle), we're just replacing it with another one. If in the future,
>> if we want to change that name we can always do so (Also if you will,
>> correcting the existing already bad naming is a different problem and
>> we're not making it any worse tbh).
>
> Using SRCU rather than the sched-rcu tracepoint synchronization in your
> use-case it caused by a limitation of sched-rcu: it cannot be efficiently
> used within idle code. So you don't care about the "can_sleep" property
> of SRCU. You could event mix SRCU and sched-rcu callsites for the same
> probe name, and it would be perfectly valid.
>
> So even though both "can_sleep" and "rcuidle" caller variants would end
> up using SRCU under the hood, each can have its own caller API, e.g.:
>
> * trace_<event>() -> only non-sleeping probes can register to those.
>                      Uses sched-rcu under the hood.
>
> * trace_<event>_can_sleep() -> both sleeping and non-sleeping probes can
>                                register to those. Uses SRCU under the hood.
>
> * trace_<event>_rcuidle() -> only non-sleeping probes can register to those,
>                              uses SRCU under the hood.
>

Cool, sounds good to me!
For starters I was thinking of changing the _rcuidle underlying
implementation as you pointed. This should be simple enough and needs
no further additional APIs. I'll work on a patch along these lines and
send it out soon. Also would love to work on your sleeping callback
case in the future as well incase you wanted to spend your cycles
working on other things.

thanks,

- Joel

Powered by blists - more mailing lists