Received-SPF: pass (google.com: domain of linux-kernel+bounces-105658-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240312113524.7654-1-qiang.zhang1211@gmail.com>
 <d3762d8c-1996-4703-8fa2-f0b00febf2fb@paulmck-laptop> <CALm+0cVivqMsTi0P_4dSK5XSE1ZW_Xx=9_h61VapZTtuOoUwDw@mail.gmail.com>
 <986428d9-5d82-463c-908b-b88435d53e79@paulmck-laptop> <CALm+0cWf9ODsxodfm+3JHciuEAe4=0fA3R4n4rhjh7XZzHonNQ@mail.gmail.com>
 <ed220ce3-943f-4554-8f79-fd5da1dfb458@paulmck-laptop>
In-Reply-To: <ed220ce3-943f-4554-8f79-fd5da1dfb458@paulmck-laptop>
From: Z qiang <qiang.zhang1211@gmail.com>
Date: Mon, 18 Mar 2024 10:35:44 +0800
Message-ID: <CALm+0cVywbBNfzZBjcxJNvpeixkBrfPZu_4qSnOXQvqYTbjMyg@mail.gmail.com>
Subject: Re: [PATCH] rcu-tasks: Remove unnecessary lazy_jiffies in call_rcu_tasks_generic_timer()
To: paulmck@kernel.org
Cc: frederic@kernel.org, neeraj.upadhyay@kernel.org, joel@joelfernandes.org, 
	rcu@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

>
> On Sun, Mar 17, 2024 at 10:31:22PM +0800, Z qiang wrote:
> > > On Wed, Mar 13, 2024 at 12:49:50PM +0800, Z qiang wrote:
> > > > > On Tue, Mar 12, 2024 at 07:35:24PM +0800, Zqiang wrote:
> > > > > > The rcu_tasks_percpu structure's->lazy_timer is queued only when
> > > > > > the rcu_tasks structure's->lazy_jiffies is not equal to zero in
> > > > > > call_rcu_tasks_generic(), if the lazy_timer callback is invoked,
> > > > > > that means the lazy_jiffes is not equal to zero, this commit
> > > > > > therefore remove lazy_jiffies check in call_rcu_tasks_generic_timer().
> > > > > >
> > > > > > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com>
> > > > > > ---
> > > > > >  kernel/rcu/tasks.h | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
> > > > > > index b1254cf3c210..439e0b9a2656 100644
> > > > > > --- a/kernel/rcu/tasks.h
> > > > > > +++ b/kernel/rcu/tasks.h
> > > > > > @@ -299,7 +299,7 @@ static void call_rcu_tasks_generic_timer(struct timer_list *tlp)
> > > > > >
> > > > > >       rtp = rtpcp->rtpp;
> > > > > >       raw_spin_lock_irqsave_rcu_node(rtpcp, flags);
> > > > > > -     if (!rcu_segcblist_empty(&rtpcp->cblist) && rtp->lazy_jiffies) {
> > > > > > +     if (!rcu_segcblist_empty(&rtpcp->cblist)) {
> > > > >
> > > > > Good eyes!
> > > > >
> > > > > But did you test with something like a WARN_ON_ONCE(rtp->lazy_jiffies)?
> > > >
> > > > Hi, Paul
> > > >
> > > > +  if (!rcu_segcblist_empty(&rtpcp->cblist) &&
> > > > !WARN_ON_ONCE(!rtp->lazy_jiffies))
> > > >
> > > > I've done tests like this:
> > > >
> > > > 1. runqemu nographic kvm slirp qemuparams="-smp 4 -m 2048M -drive
> > > > file=$PWD/share.img,if=virtio"
> > > > bootparams="rcupdate.rcu_tasks_trace_lazy_ms=0" -d
> > > >
> > > > 2.  insmod torture.ko
> > > >      insmod rcutorture.ko torture_type=tasks-tracing fwd_progress=4
> > > >
> > > > 3. bpftrace -e 't:timer:timer_expire_entry /args->function ==
> > > > kaddr("call_rcu_tasks_generic_timer")/
> > > >                                             {
> > > > printf("comm:%s,cpu:%d,stack:%s,func:%s\n", comm, cpu, kstack,
> > > > ksym(args->function)); }'
> > > >
> > > > The call_rcu_tasks_generic_timer() has never been executed.
> > >
> > > Very good!
> > >
> > > Then if we get a couple of acks or reviews from the others acknowledging
> > > that if they ever make ->lazy_jiffies be changeable at runtime, they
> > > will remember to do something to adjust this logic appropriately, I will
> > > take it.  ;-)
> > >
> >
> > Hi, Paul
> >
> > Would it be better to modify it as follows? set needwake not
> > depend on lazy_jiffies,  if  ->lazy_jiffies be changed at runtime,
> > and set it to zero, will miss the chance to wake up gp kthreads.
> >
> > diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
> > index e7ac9138a4fd..aea2b71af36c 100644
> > --- a/kernel/rcu/tasks.h
> > +++ b/kernel/rcu/tasks.h
> > @@ -299,11 +299,12 @@ static void call_rcu_tasks_generic_timer(struct
> > timer_list *tlp)
> >
> >         rtp = rtpcp->rtpp;
> >         raw_spin_lock_irqsave_rcu_node(rtpcp, flags);
> > -       if (!rcu_segcblist_empty(&rtpcp->cblist) && rtp->lazy_jiffies) {
> > +       if (!rcu_segcblist_empty(&rtpcp->cblist)) {
> >                 if (!rtpcp->urgent_gp)
> >                         rtpcp->urgent_gp = 1;
> >                 needwake = true;
> > -               mod_timer(&rtpcp->lazy_timer, rcu_tasks_lazy_time(rtp));
> > +               if (rtp->lazy_jiffies)
> > +                       mod_timer(&rtpcp->lazy_timer, rcu_tasks_lazy_time(rtp));
> >         }
> >         raw_spin_unlock_irqrestore_rcu_node(rtpcp, flags);
> >         if (needwake)
>
> Interesting approach!
>
> But how does that avoid defeating laziness by doing the wakeup early?

Hello, Paul

Does this mean that if cblist is empty, we will miss the opportunity to
enqueue the timer again?  If so, we can move mod_timer() outside
the if condition.
or I didn't understand your question?

Thanks
Zqiang


>
>                                                         Thanx, Paul
>
> > Thanks
> > Zqiang
> >
> >
> > >                                                         Thanx, Paul
> > >
> > > > Thanks
> > > > Zqiang
> > > >
> > > >
> > > > >
> > > > >                                                 Thanx, Paul
> > > > >
> > > > > >               if (!rtpcp->urgent_gp)
> > > > > >                       rtpcp->urgent_gp = 1;
> > > > > >               needwake = true;
> > > > > > --
> > > > > > 2.17.1
> > > > > >