Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Wed, 8 Jun 2022 16:29:27 +0200
From:   Frederic Weisbecker <frederic@kernel.org>
To:     nicolas saenz julienne <nsaenz@kernel.org>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Phil Auld <pauld@redhat.com>,
        Alex Belits <abelits@marvell.com>,
        Xiongfeng Wang <wangxiongfeng2@huawei.com>,
        Neeraj Upadhyay <quic_neeraju@quicinc.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Yu Liao <liaoyu15@huawei.com>,
        Boqun Feng <boqun.feng@gmail.com>,
        "Paul E . McKenney" <paulmck@kernel.org>,
        Marcelo Tosatti <mtosatti@redhat.com>,
        Paul Gortmaker <paul.gortmaker@windriver.com>,
        Uladzislau Rezki <uladzislau.rezki@sony.com>,
        Joel Fernandes <joel@joelfernandes.org>,
        Mark Rutland <mark.rutland@arm.com>
Subject: Re: [PATCH 20/21] rcu/context_tracking: Merge dynticks counter and
 context tracking states
Message-ID: <20220608142927.GA1735086@lothringen>
References: <20220519145823.618983-1-frederic@kernel.org>
 <20220519145823.618983-21-frederic@kernel.org>
 <315b0b80f7f541b99a49a9fecb743874f31d95ba.camel@kernel.org>
 <20220531142335.GA1272449@lothringen>
 <bcc4c9fa41ace6f9d98d88d24d1bd67a469cbfeb.camel@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <bcc4c9fa41ace6f9d98d88d24d1bd67a469cbfeb.camel@kernel.org>
Precedence: bulk

On Tue, May 31, 2022 at 06:15:36PM +0200, nicolas saenz julienne wrote:
> On Tue, 2022-05-31 at 16:23 +0200, Frederic Weisbecker wrote:
> > But idle at least is an exception and CONTEXT_IDLE will remain during the
> > interrupt handling. It's not that trivial to handle the idle case because
> > ct_irq_exit() needs to know that it is called between ct_idle_enter() and
> > ct_idle_exit().
> 
> Just for the record, this behaviour was already here regardless of this series,
> so it's not something it needs to fix.

Right.

> 
> Something like this should work, right?
> 
>      ct_idle_enter()
>          //IRQ or NMI
> 	 if (__ct_state() == CONTEXT_IDLE)
> 	     ct_idle_exit()

Right but that's one more costly operation (atomic_add_return())

>          ct_irq_enter()

Ideally this should increment by RCU_DYNTICKS_IDX - CONTEXT_IDLE

> 	    ...
>          ct_irq_exit()

And this should increment by RCU_DYNTICKS_IDX + CONTEXT_IDLE

I guess the CONTEXT_IDLE state should be remembered on some per cpu
variable somewhere.

BTW one interesting optimization to do when an idle interrupt leads to
setting need_resched() would be to have:

    idle_loop() {
        while (!need_resched) {
            rcu_idle_enter();
	    mwait();
	    //IRQ {
	        rcu_irq_enter();
		do_irq()... //set need_resched()
		rcu_irq_exit() // but no need to do the atomic_add_return() here
		               // since we want to keep RCU watching as we'll
                               // escape from idle
            }
	    rcu_idle_exit() // and no need to do the atomic_add_return() here either


That's two expensive operations spared for a pretty common event.
	
	
> 	 if (needs_update_state()) //using irqentry_state_t for ex.
> 	     ct_idle_entry()
>      ct_idle_exit()
> 
> Note that it's not a big issue as we can work around this behaviour by checking
> through dynticks whether a CPU is really idle.
> 
> Do you think it's worth fixing nonetheless?

Nothing urgent for sure.

> 
> Regards,
> Nicolas