Re: [PATCH 1/3] sched: fix exit_mm vs membarrier (v4)

[Boqun Feng <boqun.feng@gmail.com>]

Hi,

On Tue, Oct 20, 2020 at 10:59:58AM -0400, Mathieu Desnoyers wrote:
> ----- On Oct 20, 2020, at 10:36 AM, Peter Zijlstra peterz@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