Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Date:   Mon, 30 Oct 2023 19:26:31 +0900
From:   Byungchul Park <byungchul@sk.com>
To:     Nadav Amit <namit@vmware.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-mm <linux-mm@kvack.org>,
        "kernel_team@skhynix.com" <kernel_team@skhynix.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        "ying.huang@intel.com" <ying.huang@intel.com>,
        "xhao@linux.alibaba.com" <xhao@linux.alibaba.com>,
        "mgorman@techsingularity.net" <mgorman@techsingularity.net>,
        "hughd@google.com" <hughd@google.com>,
        "willy@infradead.org" <willy@infradead.org>,
        "david@redhat.com" <david@redhat.com>,
        "peterz@infradead.org" <peterz@infradead.org>,
        Andy Lutomirski <luto@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        "mingo@redhat.com" <mingo@redhat.com>,
        "bp@alien8.de" <bp@alien8.de>,
        "dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>
Subject: Re: [v3 1/3] mm/rmap: Recognize non-writable TLB entries during TLB
 batch flush
Message-ID: <20231030102631.GB81877@system.software.com>
References: <20231030072540.38631-1-byungchul@sk.com>
 <20231030072540.38631-2-byungchul@sk.com>
 <1DB097E6-6585-4D10-95C9-7BAA5A622B7E@vmware.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1DB097E6-6585-4D10-95C9-7BAA5A622B7E@vmware.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Mon, Oct 30, 2023 at 07:52:05AM +0000, Nadav Amit wrote:
> 
> Below are some points you might find useful:

Thank you!

> > +
> > /*
> >  * Blindly accessing user memory from NMI context can be dangerous
> >  * if we're in the middle of switching the current user task or
> > diff --git a/include/linux/mm_types_task.h b/include/linux/mm_types_task.h
> > index aa44fff8bb9d..35ba9425d48d 100644
> > --- a/include/linux/mm_types_task.h
> > +++ b/include/linux/mm_types_task.h
> > @@ -59,8 +59,8 @@ struct tlbflush_unmap_batch {
> > 	 */
> > 	struct arch_tlbflush_unmap_batch arch;
> > 
> > -	/* True if a flush is needed. */
> > -	bool flush_required;
> > +	/* The number of flush requested. */
> 
> Number of what? Base pages I presume.

How many times set_tlb_ubc_flush_pending() has been called.

> > +	int nr_flush_required;
> 
> Perhaps unsigned would be better suited?

Will change it to unsigned.

> > 	/*
> > 	 * If true then the PTE was dirty when unmapped. The entry must be
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index 77f01ac385f7..63189c023357 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -1324,6 +1324,7 @@ struct task_struct {
> > #endif
> > 
> > 	struct tlbflush_unmap_batch	tlb_ubc;
> > +	struct tlbflush_unmap_batch	tlb_ubc_nowr;
> 
> tlb_ubc_nowr is - I think - less informative the tlb_ubc_ro (and a comment
> would be useful).

At the beginning, I named it tlb_ubc_ro but.. I forgot why I changed it
to tlb_ubc_nowr but.. I will change it back and add a comment on it.

> > +
> > +int nr_flush_required(void)
> > +{
> > +	return current->tlb_ubc.nr_flush_required;
> > +}
> > +
> > +int nr_flush_required_nowr(void)
> > +{
> > +	return current->tlb_ubc_nowr.nr_flush_required;
> > +}
> 
> I haven’t gone through the users of these functions yet, as they are not included
> in this patch (which is usually not great).

Right. I will place these two on another patch that uses the functions.
Or need to add an explanation in this commit message.

> Anyhow, it might be a bit wasteful to have a function call for such a function. See
> if it is possible to avoid that call.

I will move them to mm/internal.h with inline added if possible.

> > +
> > /*
> >  * Flush TLB entries for recently unmapped pages from remote CPUs. It is
> >  * important if a PTE was dirty when it was unmapped that it's flushed
> > @@ -615,11 +641,12 @@ void try_to_unmap_flush(void)
> > {
> > 	struct tlbflush_unmap_batch *tlb_ubc = &current->tlb_ubc;
> > 
> > -	if (!tlb_ubc->flush_required)
> > +	fold_ubc_nowr();
> > +	if (!tlb_ubc->nr_flush_required)
> > 		return;
> > 
> > 	arch_tlbbatch_flush(&tlb_ubc->arch);
> > -	tlb_ubc->flush_required = false;
> > +	tlb_ubc->nr_flush_required = 0;
> > 	tlb_ubc->writable = false;
> > }
> > 
> > @@ -627,8 +654,9 @@ void try_to_unmap_flush(void)
> > void try_to_unmap_flush_dirty(void)
> > {
> > 	struct tlbflush_unmap_batch *tlb_ubc = &current->tlb_ubc;
> > +	struct tlbflush_unmap_batch *tlb_ubc_nowr = &current->tlb_ubc_nowr;
> > 
> > -	if (tlb_ubc->writable)
> > +	if (tlb_ubc->writable || tlb_ubc_nowr->writable)
> > 		try_to_unmap_flush();
> > }
> > 
> > @@ -645,15 +673,16 @@ void try_to_unmap_flush_dirty(void)
> > static void set_tlb_ubc_flush_pending(struct mm_struct *mm, pte_t pteval,
> > 				      unsigned long uaddr)
> > {
> > -	struct tlbflush_unmap_batch *tlb_ubc = &current->tlb_ubc;
> > +	struct tlbflush_unmap_batch *tlb_ubc;
> > 	int batch;
> > 	bool writable = pte_dirty(pteval);
> > 
> > 	if (!pte_accessible(mm, pteval))
> > 		return;
> > 
> > +	tlb_ubc = pte_write(pteval) || writable ? &current->tlb_ubc : &current->tlb_ubc_nowr;
> 
> Using the ternary operator here is a bit confusing. You can use an “if”
> instead or if you mind is set doing it this way at least make it easier to
> read:
> 
> 	tlb_ubc = (pte_write(pteval) || writable) ? &current->tlb_ubc :
> 						    &current->tlb_ubc_nowr;

You are right. I should change it that way. Thanks.

> And of course, add a comment.

Okay. Also will add a comment.

> > 	arch_tlbbatch_add_pending(&tlb_ubc->arch, mm, uaddr);
> > -	tlb_ubc->flush_required = true;
> > +	tlb_ubc->nr_flush_required += 1;
> 
> Presumably overflow is impossible for other reasons, but something like that
> worries me.

Agree with you. Lemme think it more and fix it.

Thank you.

	Byungchul