Re: [PATCH v6 2/5] lazy tlb: allow lazy tlb mm refcounting to be configurable

From: "Nicholas Piggin" <npiggin@gmail.com>
To: "Nadav Amit" <nadav.amit@gmail.com>,
	"Andrew Morton" <akpm@linux-foundation.org>
Cc: "Andy Lutomirski" <luto@kernel.org>,
	"Linus Torvalds" <torvalds@linux-foundation.org>,
	"linux-arch" <linux-arch@vger.kernel.org>,
	"linux-mm" <linux-mm@kvack.org>, <linuxppc-dev@lists.ozlabs.org>
Subject: Re: [PATCH v6 2/5] lazy tlb: allow lazy tlb mm refcounting to be configurable
Date: Tue, 24 Jan 2023 12:29:38 +1000	[thread overview]
Message-ID: <CQ02EXZ5THB8.4S19LP5ZFUM9@bobo> (raw)
In-Reply-To: <4d26df97-3725-182b-6312-fa5cd8e9f85d@gmail.com>

On Mon Jan 23, 2023 at 6:02 PM AEST, Nadav Amit wrote:
>
>
> On 1/23/23 9:35 AM, Nadav Amit wrote:
> >> +    if (IS_ENABLED(CONFIG_MMU_LAZY_TLB_REFCOUNT)) {
> >> +        mmdrop(mm);
> >> +    } else {
> >> +        /*
> >> +         * mmdrop_lazy_tlb must provide a full memory barrier, see the
> >> +         * membarrier comment finish_task_switch which relies on this.
> >> +         */
> >> +        smp_mb();
> >> +    }
> >>   }
> > 
> > Considering the fact that mmdrop_lazy_tlb() replaced mmdrop() in various 
> > locations in which smp_mb() was not required, this comment might be 
> > confusing. IOW, for the cases in most cases where mmdrop_lazy_tlb() 
> > replaced mmdrop(), this comment was irrelevant, and therefore it now 
> > becomes confusing.
> > 
> > I am not sure the include the smp_mb() here instead of "open-coding" it 
> > helps.
> I think that I now understand why you do need the smp_mb() here, so 
> ignore my comment.

For the moment it's basically a convenience thing so the caller does not
have to care what option is configured. Possibly we could weaken it and
do necessary barriers in callers if we consolidated to one option, but
I'd have to be convinced it'd be worthwhile, because it would still make
it deviate from mmdrop(), and we'd probably at least need a release
barrier to drop the reference.

Thanks,
Nick