Hi,
On Tue, Oct 20, 2020 at 10:59:58AM -0400, Mathieu Desnoyers wrote:
> ----- On Oct 20, 2020, at 10:36 AM, Peter Zijlstra pet...@infradead.org wrote:
>
> > On Tue, Oct 20, 2020 at 09:47:13AM -0400, Mathieu Desnoyers wrote:
> >> +void membarrier_update_current_mm(struct mm_struct *next_mm)
> >> +{
> >> + struct rq *rq = this_rq();
> >> + int membarrier_state = 0;
> >> +
> >> + if (next_mm)
> >> + membarrier_state = atomic_read(&next_mm->membarrier_state);
> >> + if (READ_ONCE(rq->membarrier_state) == membarrier_state)
> >> + return;
> >> + WRITE_ONCE(rq->membarrier_state, membarrier_state);
> >> +}
> >
> > This is suspisioucly similar to membarrier_switch_mm().
> >
> > Would something like so make sense?
>
> Very much yes. Do you want me to re-send the series, or you
> want to fold this in as you merge it ?
>
> Thanks,
>
> Mathieu
>
> >
> > ---
> > --- a/kernel/sched/membarrier.c
> > +++ b/kernel/sched/membarrier.c
> > @@ -206,14 +206,7 @@ void membarrier_exec_mmap(struct mm_stru
> >
> > void membarrier_update_current_mm(struct mm_struct *next_mm)
> > {
> > - struct rq *rq = this_rq();
> > - int membarrier_state = 0;
> > -
> > - if (next_mm)
> > - membarrier_state = atomic_read(&next_mm->membarrier_state);
> > - if (READ_ONCE(rq->membarrier_state) == membarrier_state)
> > - return;
> > - WRITE_ONCE(rq->membarrier_state, membarrier_state);
> > + membarrier_switch_mm(this_rq(), NULL, next_mm);
> > }
> >
> > static int membarrier_global_expedited(void)
> > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> > index d2621155393c..3d589c2ffd28 100644
> > --- a/kernel/sched/sched.h
> > +++ b/kernel/sched/sched.h
> > @@ -2645,12 +2645,14 @@ static inline void membarrier_switch_mm(struct rq
> > *rq,
> > struct mm_struct *prev_mm,
> > struct mm_struct *next_mm)
> > {
> > - int membarrier_state;
> > + int membarrier_state = 0;
> >
> > if (prev_mm == next_mm)
Unless I'm missing something subtle, in exit_mm(),
membarrier_update_current_mm() is called with @next_mm == NULL, and
inside membarrier_update_current_mm(), membarrier_switch_mm() is called
wiht @prev_mm == NULL. As a result, the branch above is taken, so
membarrier_update_current_mm() becomes a nop. I think we should use the
previous value of current->mm as the @prev_mm, something like below
maybe?
void update_current_mm(struct mm_struct *next_mm)
{
struct mm_struct *prev_mm;
unsigned long flags;
local_irq_save(flags);
prev_mm = current->mm;
current->mm = next_mm;
membarrier_switch_mm(this_rq(), prev_mm, next_mm);
local_irq_restore(flags);
}
, and replace all settings for "current->mm" in kernel with
update_current_mm().
Thoughts?
Regards,
Boqun
> > return;
> >
> > - membarrier_state = atomic_read(&next_mm->membarrier_state);
> > + if (next_mm)
> > + membarrier_state = atomic_read(&next_mm->membarrier_state);
> > +
> > if (READ_ONCE(rq->membarrier_state) == membarrier_state)
> > return;
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com
