Received-SPF: pass (google.com: domain of linux-kernel+bounces-105341-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240311035502.7112-1-qiang.zhang1211@gmail.com> <49c73303-fdda-4912-b26a-75f2235b3a62@paulmck-laptop>
In-Reply-To: <49c73303-fdda-4912-b26a-75f2235b3a62@paulmck-laptop>
From: Z qiang <qiang.zhang1211@gmail.com>
Date: Sun, 17 Mar 2024 15:12:59 +0800
Message-ID: <CALm+0cWeYcp4jKV2H=g5MOHOg6WJ7se0S1g6BhZrwPACiFWYDA@mail.gmail.com>
Subject: Re: [PATCH] rcu-tasks: Avoid rtp_irq_work triggering when the
 rcu-tasks GP is ongoing
To: paulmck@kernel.org
Cc: frederic@kernel.org, joel@joelfernandes.org, neeraj.upadhyay@kernel.org, 
	rcu@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

>
> On Mon, Mar 11, 2024 at 11:55:02AM +0800, Zqiang wrote:
> > This commit generate rcu_task_gp_in_progress() to check whether
> > the rcu-tasks GP is ongoing, if is ongoing, avoid trigger
> > rtp_irq_work to wakeup rcu tasks kthreads in call_rcu_tasks_generic().
> >
> > The test results are as follows:
> >
> > echo call_rcu_tasks_iw_wakeup > /sys/kernel/debug/tracing/set_ftrace_filter
> > echo 1 > /sys/kernel/debug/tracing/function_profile_enabled
> > insmod rcutorture.ko torture_type=tasks-tracing fwd_progress=4
> > sleep 600
> > rmmod rcutorture.ko
> > echo 0 > /sys/kernel/debug/tracing/function_profile_enabled
> > echo > /sys/kernel/debug/tracing/set_ftrace_filter
> >
> > head /sys/kernel/debug/tracing/trace_stat/function*
> >
> > original: 56376  apply patch: 33521
> >
> > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com>
>
> Note that rcu_seq_current() does not provide ordering.  So are you
> sure that this change is safe on weakly ordered systems such as ARM?

The  rcu_seq_current() provide implicit address-dependency barriers, or
did I miss something?

>
> For example, consider the following sequence of events:
>
> o       The call_rcu_tasks_generic() function picks up the grace-period
>         sequence number, which shows that there is a grace period in
>         progress.

The rcu-callback has been enqueued to list before we pick up the
gp seq number.

>
> o       The grace period ends, and sees no reason to start a new grace
>         period.

the gp ends, the rcu_tasks_need_gpcb() will be invoked to check
whether to start a new gp.   find pending callback, the new gp
will start  or did I miss something?

Thanks
Zqiang


>
> o       The call_rcu_tasks_generic() function sees no reason to wake
>         up the grace-period kthread.  There are no more calls to
>         call_rcu_tasks*(), so the callback is never invoked.
>
> Or is there something that prevents this sequence of events from
> ever happening on weakly ordered systems?
>
>                                                         Thanx, Paul
>
> > ---
> >
> > original:
> > ==> /sys/kernel/debug/tracing/trace_stat/function0 <==
> >   Function                               Hit    Time            Avg             s^2
> >   --------                               ---    ----            ---             ---
> >   call_rcu_tasks_iw_wakeup             13217    19292.52 us     1.459 us        8.834 us
> >
> > ==> /sys/kernel/debug/tracing/trace_stat/function1 <==
> >   Function                               Hit    Time            Avg             s^2
> >   --------                               ---    ----            ---             ---
> >   call_rcu_tasks_iw_wakeup             15146    22377.01 us     1.477 us        22.873 us
> >
> > ==> /sys/kernel/debug/tracing/trace_stat/function2 <==
> >   Function                               Hit    Time            Avg             s^2
> >   --------                               ---    ----            ---             ---
> >   call_rcu_tasks_iw_wakeup             12561    18125.76 us     1.443 us        6.372 us
> >
> > ==> /sys/kernel/debug/tracing/trace_stat/function3 <==
> >   Function                               Hit    Time            Avg             s^2
> >   --------                               ---    ----            ---             ---
> >   call_rcu_tasks_iw_wakeup             15452    21770.57 us     1.408 us        6.710 us
> >
> > apply patch:
> > ==> /sys/kernel/debug/tracing/trace_stat/function0 <==
> >   Function                               Hit    Time            Avg             s^2
> >   --------                               ---    ----            ---             ---
> >   call_rcu_tasks_iw_wakeup              8334    15121.13 us     1.814 us        4.457 us
> >
> > ==> /sys/kernel/debug/tracing/trace_stat/function1 <==
> >   Function                               Hit    Time            Avg             s^2
> >   --------                               ---    ----            ---             ---
> >   call_rcu_tasks_iw_wakeup              8355    15760.51 us     1.886 us        14.775 us
> >
> > ==> /sys/kernel/debug/tracing/trace_stat/function2 <==
> >   Function                               Hit    Time            Avg             s^2
> >   --------                               ---    ----            ---             ---
> >   call_rcu_tasks_iw_wakeup              7219    14194.27 us     1.966 us        42.440 us
> >
> > ==> /sys/kernel/debug/tracing/trace_stat/function3 <==
> >   Function                               Hit    Time            Avg             s^2
> >   --------                               ---    ----            ---             ---
> >   call_rcu_tasks_iw_wakeup              9613    19850.04 us     2.064 us        91.023 us
> >
> >  kernel/rcu/tasks.h | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
> > index 147b5945d67a..36c7e1d441d0 100644
> > --- a/kernel/rcu/tasks.h
> > +++ b/kernel/rcu/tasks.h
> > @@ -317,6 +317,11 @@ static void call_rcu_tasks_iw_wakeup(struct irq_work *iwp)
> >       rcuwait_wake_up(&rtp->cbs_wait);
> >  }
> >
> > +static int rcu_task_gp_in_progress(struct rcu_tasks *rtp)
> > +{
> > +     return rcu_seq_state(rcu_seq_current(&rtp->tasks_gp_seq));
> > +}
> > +
> >  // Enqueue a callback for the specified flavor of Tasks RCU.
> >  static void call_rcu_tasks_generic(struct rcu_head *rhp, rcu_callback_t func,
> >                                  struct rcu_tasks *rtp)
> > @@ -375,7 +380,8 @@ static void call_rcu_tasks_generic(struct rcu_head *rhp, rcu_callback_t func,
> >       }
> >       rcu_read_unlock();
> >       /* We can't create the thread unless interrupts are enabled. */
> > -     if (needwake && READ_ONCE(rtp->kthread_ptr))
> > +     if (needwake && READ_ONCE(rtp->kthread_ptr) &&
> > +                     !rcu_task_gp_in_progress(rtp))
> >               irq_work_queue(&rtpcp->rtp_irq_work);
> >  }
> >
> > --
> > 2.17.1
> >