* Review of RCU-related patches in -rt
@ 2019-05-28 20:50 Paul E. McKenney
2019-06-07 16:08 ` Sebastian Andrzej Siewior
0 siblings, 1 reply; 3+ messages in thread
From: Paul E. McKenney @ 2019-05-28 20:50 UTC (permalink / raw)
To: bigeasy; +Cc: linux-kernel
Hello, Sebastian,
Finally getting around to taking another look:
c7e07056a108 EXP rcu: skip the workqueue path on RT
This one makes sense given the later commit setting the
rcu_normal_after_boot kernel parameter. Otherwise, it is
slowing down expedited grace periods for no reason. But
should the check also include rcu_normal_after_boot and
rcu_normal? For example:
if ((IS_ENABLED(CONFIG_PREEMPT_RT_FULL) &&
(rcu_normal || rcu_normal_after_boot) ||
!READ_ONCE(rcu_par_gp_wq) ||
rcu_scheduler_active != RCU_SCHEDULER_RUNNING ||
rcu_is_last_leaf_node(rnp)) {
Alternatively, one approach would be to take the kernel
parameters out in -rt:
static int rcu_normal_after_boot = IS_ENABLED(CONFIG_PREEMPT_RT_FULL);
#ifndef CONFIG_PREEMPT_RT_FULL
module_param(rcu_normal_after_boot, int, 0);
#endif
And similar for rcu_normal and rcu_expedited.
Or is there some reason to allow run-time expedited grace
periods in CONFIG_PREEMPT_RT_FULL=y kernels?
d1f52391bd8a rcu: Disable RCU_FAST_NO_HZ on RT
Looks good. More complexity could be added if too many people
get themselves in trouble via "select RCU_FAST_NO_HZ".
42b346870326 rcu: make RCU_BOOST default on RT
To avoid complaints about this showing up when people don't
expected, could you please instead "select RCU_BOOST" in
the Kconfig definition of PREEMPT_RT_FULL?
Or do people really want to be able to disable boosting?
457c1b0d9c0e sched: Do not account rcu_preempt_depth on RT in might_sleep()
The idea behind this one is to avoid false-positive complaints
about -rt's sleeping spinlocks, correct?
7ee13e640b01 rbtree: don't include the rcu header
c9b0c9b87081 rtmutex: annotate sleeping lock context
No specific comments.
7912d002ebf9 rcu: Eliminate softirq processing from rcutree
This hasn't caused any problems in -rcu from what I can see.
I am therefore planning to submit the -rcu variant of this to
mainline during the next merge window.
f06d34ebdbbb srcu: Remove srcu_queue_delayed_work_on()
Looks plausible. I will check more carefully for mainline.
aeb04e894cc9 srcu: replace local_irqsave() with a locallock
e48989b033ad irqwork: push most work into softirq context
These look to still be -rt only.
Thanx, Paul
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Review of RCU-related patches in -rt
2019-05-28 20:50 Review of RCU-related patches in -rt Paul E. McKenney
@ 2019-06-07 16:08 ` Sebastian Andrzej Siewior
2019-06-20 22:32 ` Paul E. McKenney
0 siblings, 1 reply; 3+ messages in thread
From: Sebastian Andrzej Siewior @ 2019-06-07 16:08 UTC (permalink / raw)
To: Paul E. McKenney; +Cc: linux-kernel, tglx
On 2019-05-28 13:50:30 [-0700], Paul E. McKenney wrote:
> Hello, Sebastian,
Hi Paul,
> Finally getting around to taking another look:
>
> c7e07056a108 EXP rcu: skip the workqueue path on RT
>
> This one makes sense given the later commit setting the
> rcu_normal_after_boot kernel parameter. Otherwise, it is
> slowing down expedited grace periods for no reason. But
> should the check also include rcu_normal_after_boot and
> rcu_normal? For example:
>
> if ((IS_ENABLED(CONFIG_PREEMPT_RT_FULL) &&
> (rcu_normal || rcu_normal_after_boot) ||
> !READ_ONCE(rcu_par_gp_wq) ||
> rcu_scheduler_active != RCU_SCHEDULER_RUNNING ||
> rcu_is_last_leaf_node(rnp)) {
I recently dropped that patch from the queue because the workqueue
problem vanished.
> Alternatively, one approach would be to take the kernel
> parameters out in -rt:
>
> static int rcu_normal_after_boot = IS_ENABLED(CONFIG_PREEMPT_RT_FULL);
> #ifndef CONFIG_PREEMPT_RT_FULL
> module_param(rcu_normal_after_boot, int, 0);
> #endif
>
> And similar for rcu_normal and rcu_expedited.
This makes sense.
> Or is there some reason to allow run-time expedited grace
> periods in CONFIG_PREEMPT_RT_FULL=y kernels?
No, I doubt there is any need to use the `expedited' version. The
problem is that it increases latencies.
> d1f52391bd8a rcu: Disable RCU_FAST_NO_HZ on RT
>
> Looks good. More complexity could be added if too many people
> get themselves in trouble via "select RCU_FAST_NO_HZ".
That patch disables RCU_FAST_NO_HZ and claims that it has something to
do with a timer_list timer and IRQ-off section. We couldn't schedule
timers from IRQ-off regions but not anymore. Only del_timer_sync() can't
be invoked from IRQ-off regions.
I just booted a box with this enabled together with NO_HZ/ NO_HZ_FULL
and I not complains yet. So I might drop that…
> 42b346870326 rcu: make RCU_BOOST default on RT
>
> To avoid complaints about this showing up when people don't
> expected, could you please instead "select RCU_BOOST" in
> the Kconfig definition of PREEMPT_RT_FULL?
>
> Or do people really want to be able to disable boosting?
I have no idea. I guess most people don't know what it does and stay
with the default. It become default on RT once a few people complained
that they run OOM during boot on some "memory contrained systems". That
option avoided it.
So yes, will make it depend on RT.
> 457c1b0d9c0e sched: Do not account rcu_preempt_depth on RT in might_sleep()
>
> The idea behind this one is to avoid false-positive complaints
> about -rt's sleeping spinlocks, correct?
Correct. Maybe we could invoke a different schedule() primitiv so RCU is
aware that this is a sleeping spinlock and not a regular sleeping lock.
> 7ee13e640b01 rbtree: don't include the rcu header
> c9b0c9b87081 rtmutex: annotate sleeping lock context
>
> No specific comments.
>
> 7912d002ebf9 rcu: Eliminate softirq processing from rcutree
>
> This hasn't caused any problems in -rcu from what I can see.
> I am therefore planning to submit the -rcu variant of this to
> mainline during the next merge window.
wonderful.
> f06d34ebdbbb srcu: Remove srcu_queue_delayed_work_on()
>
> Looks plausible. I will check more carefully for mainline.
Hmmm. I though this was already upstream.
That said, we can now schedule work from a preempt_disable() section but
I still like the negative diffstat here :)
> aeb04e894cc9 srcu: replace local_irqsave() with a locallock
> e48989b033ad irqwork: push most work into softirq context
>
> These look to still be -rt only.
I might get rid of the local_lock in srcu. Will have to check.
Thank you Paul.
> Thanx, Paul
Sebastian
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Review of RCU-related patches in -rt
2019-06-07 16:08 ` Sebastian Andrzej Siewior
@ 2019-06-20 22:32 ` Paul E. McKenney
0 siblings, 0 replies; 3+ messages in thread
From: Paul E. McKenney @ 2019-06-20 22:32 UTC (permalink / raw)
To: Sebastian Andrzej Siewior; +Cc: linux-kernel, tglx
On Fri, Jun 07, 2019 at 06:08:57PM +0200, Sebastian Andrzej Siewior wrote:
> On 2019-05-28 13:50:30 [-0700], Paul E. McKenney wrote:
> > Hello, Sebastian,
> Hi Paul,
>
> > Finally getting around to taking another look:
> >
> > c7e07056a108 EXP rcu: skip the workqueue path on RT
> >
> > This one makes sense given the later commit setting the
> > rcu_normal_after_boot kernel parameter. Otherwise, it is
> > slowing down expedited grace periods for no reason. But
> > should the check also include rcu_normal_after_boot and
> > rcu_normal? For example:
> >
> > if ((IS_ENABLED(CONFIG_PREEMPT_RT_FULL) &&
> > (rcu_normal || rcu_normal_after_boot) ||
> > !READ_ONCE(rcu_par_gp_wq) ||
> > rcu_scheduler_active != RCU_SCHEDULER_RUNNING ||
> > rcu_is_last_leaf_node(rnp)) {
>
> I recently dropped that patch from the queue because the workqueue
> problem vanished.
>
> > Alternatively, one approach would be to take the kernel
> > parameters out in -rt:
> >
> > static int rcu_normal_after_boot = IS_ENABLED(CONFIG_PREEMPT_RT_FULL);
> > #ifndef CONFIG_PREEMPT_RT_FULL
> > module_param(rcu_normal_after_boot, int, 0);
> > #endif
> >
> > And similar for rcu_normal and rcu_expedited.
>
> This makes sense.
>
> > Or is there some reason to allow run-time expedited grace
> > periods in CONFIG_PREEMPT_RT_FULL=y kernels?
>
> No, I doubt there is any need to use the `expedited' version. The
> problem is that it increases latencies.
>
> > d1f52391bd8a rcu: Disable RCU_FAST_NO_HZ on RT
> >
> > Looks good. More complexity could be added if too many people
> > get themselves in trouble via "select RCU_FAST_NO_HZ".
>
> That patch disables RCU_FAST_NO_HZ and claims that it has something to
> do with a timer_list timer and IRQ-off section. We couldn't schedule
> timers from IRQ-off regions but not anymore. Only del_timer_sync() can't
> be invoked from IRQ-off regions.
> I just booted a box with this enabled together with NO_HZ/ NO_HZ_FULL
> and I not complains yet. So I might drop that…
>
> > 42b346870326 rcu: make RCU_BOOST default on RT
> >
> > To avoid complaints about this showing up when people don't
> > expected, could you please instead "select RCU_BOOST" in
> > the Kconfig definition of PREEMPT_RT_FULL?
> >
> > Or do people really want to be able to disable boosting?
>
> I have no idea. I guess most people don't know what it does and stay
> with the default. It become default on RT once a few people complained
> that they run OOM during boot on some "memory contrained systems". That
> option avoided it.
> So yes, will make it depend on RT.
>
> > 457c1b0d9c0e sched: Do not account rcu_preempt_depth on RT in might_sleep()
> >
> > The idea behind this one is to avoid false-positive complaints
> > about -rt's sleeping spinlocks, correct?
>
> Correct. Maybe we could invoke a different schedule() primitiv so RCU is
> aware that this is a sleeping spinlock and not a regular sleeping lock.
>
> > 7ee13e640b01 rbtree: don't include the rcu header
> > c9b0c9b87081 rtmutex: annotate sleeping lock context
> >
> > No specific comments.
> >
> > 7912d002ebf9 rcu: Eliminate softirq processing from rcutree
> >
> > This hasn't caused any problems in -rcu from what I can see.
> > I am therefore planning to submit the -rcu variant of this to
> > mainline during the next merge window.
>
> wonderful.
>
> > f06d34ebdbbb srcu: Remove srcu_queue_delayed_work_on()
> >
> > Looks plausible. I will check more carefully for mainline.
>
> Hmmm. I though this was already upstream.
> That said, we can now schedule work from a preempt_disable() section but
> I still like the negative diffstat here :)
Right you are! e81baf4cb19a ("srcu: Remove srcu_queue_delayed_work_on()")
is in v5.1.
> > aeb04e894cc9 srcu: replace local_irqsave() with a locallock
> > e48989b033ad irqwork: push most work into softirq context
> >
> > These look to still be -rt only.
>
> I might get rid of the local_lock in srcu. Will have to check.
>
> Thank you Paul.
And you! I will check again in a few months, for some definition of "a few".
Thanx, Paul
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-06-20 22:32 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-05-28 20:50 Review of RCU-related patches in -rt Paul E. McKenney
2019-06-07 16:08 ` Sebastian Andrzej Siewior
2019-06-20 22:32 ` Paul E. McKenney
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).