Messages in this thread | | | Date | Mon, 2 Feb 2015 18:53:45 +0100 | From | Frederic Weisbecker <> | Subject | Re: [PATCH 3/4] sched: Pull preemption disablement to __schedule() caller |
| |
On Wed, Jan 28, 2015 at 04:50:44PM +0100, Peter Zijlstra wrote: > On Wed, Jan 28, 2015 at 01:24:11AM +0100, Frederic Weisbecker wrote: > > +++ b/kernel/sched/core.c > > @@ -2760,7 +2760,6 @@ static void __sched __schedule(void) > > struct rq *rq; > > int cpu; > > > > - preempt_disable(); > > Implies barrier(); > > > cpu = smp_processor_id(); > > rq = cpu_rq(cpu); > > rcu_note_context_switch(); > > @@ -2822,8 +2821,6 @@ static void __sched __schedule(void) > > raw_spin_unlock_irq(&rq->lock); > > > > post_schedule(rq); > > implies barrier(); > > > - > > - sched_preempt_enable_no_resched(); > > } > > > > static inline void sched_submit_work(struct task_struct *tsk) > > > @@ -2883,9 +2882,9 @@ void __sched schedule_preempt_disabled(void) > > static void preempt_schedule_common(void) > > { > > do { > > - preempt_count_add(PREEMPT_ACTIVE); > > + preempt_count_add(PREEMPT_ACTIVE + PREEMPT_CHECK_OFFSET); > > Does _NOT_ imply barrier(); > > > __schedule(); > > - preempt_count_sub(PREEMPT_ACTIVE); > > idem.
It looks like preempt_count_add/inc() mostly imply entering a context that we want to be seen right away (thus want barrier() after) and preempt_count_sub/dec() mostly want previous work to be visible before re-enabling interrupt, preemption, etc... (thus want barrier() before).
So maybe these functions (the non-underscored ones) should imply a barrier() rather than their callers (preempt_disable() and others). Inline functions instead of macros would do the trick (if the headers hell let us do that).
Note the underscored implementations are all inline currently so this happens to work by chance for direct calls to preempt_count_add/sub() outside preempt_disable(). If the non-underscored caller is turned into inline too I don't expect performance issues.
What do you think, does it make sense?
> > > + preempt_count_sub(PREEMPT_ACTIVE + PREEMPT_CHECK_OFFSET); > > > > /* > > * Check again in case we missed a preemption opportunity
| |