Re: [PATCH 4/8] membarrier: Make the post-switch-mm barrier explicit

[Andy Lutomirski]

On Fri, Jun 18, 2021, at 7:53 PM, Nicholas Piggin wrote:
> Excerpts from Andy Lutomirski's message of June 18, 2021 9:49 am:
> > On 6/16/21 11:51 PM, Nicholas Piggin wrote:
> >> Excerpts from Andy Lutomirski's message of June 17, 2021 3:32 pm:
> >>> On Wed, Jun 16, 2021, at 7:57 PM, Andy Lutomirski wrote:
> >>>>
> >>>>
> >>>> On Wed, Jun 16, 2021, at 6:37 PM, Nicholas Piggin wrote:
> >>>>> Excerpts from Andy Lutomirski's message of June 17, 2021 4:41 am:
> >>>>>> On 6/16/21 12:35 AM, Peter Zijlstra wrote:
> >>>>>>> On Wed, Jun 16, 2021 at 02:19:49PM +1000, Nicholas Piggin wrote:
> >>>>>>>> Excerpts from Andy Lutomirski's message of June 16, 2021 1:21 pm:
> >>>>>>>>> membarrier() needs a barrier after any CPU changes mm.  There is currently
> >>>>>>>>> a comment explaining why this barrier probably exists in all cases.  This
> >>>>>>>>> is very fragile -- any change to the relevant parts of the scheduler
> >>>>>>>>> might get rid of these barriers, and it's not really clear to me that
> >>>>>>>>> the barrier actually exists in all necessary cases.
> >>>>>>>>
> >>>>>>>> The comments and barriers in the mmdrop() hunks? I don't see what is 
> >>>>>>>> fragile or maybe-buggy about this. The barrier definitely exists.
> >>>>>>>>
> >>>>>>>> And any change can change anything, that doesn't make it fragile. My
> >>>>>>>> lazy tlb refcounting change avoids the mmdrop in some cases, but it
> >>>>>>>> replaces it with smp_mb for example.
> >>>>>>>
> >>>>>>> I'm with Nick again, on this. You're adding extra barriers for no
> >>>>>>> discernible reason, that's not generally encouraged, seeing how extra
> >>>>>>> barriers is extra slow.
> >>>>>>>
> >>>>>>> Both mmdrop() itself, as well as the callsite have comments saying how
> >>>>>>> membarrier relies on the implied barrier, what's fragile about that?
> >>>>>>>
> >>>>>>
> >>>>>> My real motivation is that mmgrab() and mmdrop() don't actually need to
> >>>>>> be full barriers.  The current implementation has them being full
> >>>>>> barriers, and the current implementation is quite slow.  So let's try
> >>>>>> that commit message again:
> >>>>>>
> >>>>>> membarrier() needs a barrier after any CPU changes mm.  There is currently
> >>>>>> a comment explaining why this barrier probably exists in all cases. The
> >>>>>> logic is based on ensuring that the barrier exists on every control flow
> >>>>>> path through the scheduler.  It also relies on mmgrab() and mmdrop() being
> >>>>>> full barriers.
> >>>>>>
> >>>>>> mmgrab() and mmdrop() would be better if they were not full barriers.  As a
> >>>>>> trivial optimization, mmgrab() could use a relaxed atomic and mmdrop()
> >>>>>> could use a release on architectures that have these operations.
> >>>>>
> >>>>> I'm not against the idea, I've looked at something similar before (not
> >>>>> for mmdrop but a different primitive). Also my lazy tlb shootdown series 
> >>>>> could possibly take advantage of this, I might cherry pick it and test 
> >>>>> performance :)
> >>>>>
> >>>>> I don't think it belongs in this series though. Should go together with
> >>>>> something that takes advantage of it.
> >>>>
> >>>> I’m going to see if I can get hazard pointers into shape quickly.
> >>>
> >>> Here it is.  Not even boot tested!
> >>>
> >>> https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=sched/lazymm&id=ecc3992c36cb88087df9c537e2326efb51c95e31
> >>>
> >>> Nick, I think you can accomplish much the same thing as your patch by:
> >>>
> >>> #define for_each_possible_lazymm_cpu while (false)
> >> 
> >> I'm not sure what you mean? For powerpc, other CPUs can be using the mm 
> >> as lazy at this point. I must be missing something.
> > 
> > What I mean is: if you want to shoot down lazies instead of doing the
> > hazard pointer trick to track them, you could do:
> > 
> > #define for_each_possible_lazymm_cpu while (false)
> > 
> > which would promise to the core code that you don't have any lazies left
> > by the time exit_mmap() is done.  You might need a new hook in
> > exit_mmap() depending on exactly how you implement the lazy shootdown.
> 
> Oh for configuring it away entirely. I'll have to see how it falls out, 
> I suspect we'd want to just no-op that entire function and avoid the 2 
> atomics if we are taking care of our lazy mms with shootdowns.

Do you mean the smp_store_release()?  On x86 and similar architectures, that’s almost free.  I’m also not convinced it needs to be a real release.

> 
> The more important thing would be the context switch fast path, but even 
> there, there's really no reason why the two approaches couldn't be made 
> to both work with some careful helper functions or structuring of the 
> code.
> 
> Thanks,
> Nick
>