Messages in this thread | | | From | Arvind Sankar <> | Date | Sat, 25 Jan 2020 16:25:25 -0500 | Subject | Re: [PATCH v15] x86/split_lock: Enable split lock detection by kernel |
| |
On Fri, Jan 24, 2020 at 06:47:27PM -0800, Luck, Tony wrote: > I did find something with a new test. Applications that hit a > split lock warn as expected. But if they sleep before they hit > a new split lock, we get another warning. This is may be because > I messed up when fixing a PeterZ typo in the untested patch. > But I think there may have been bigger problems. > > Context switch in V14 code did: > > if (tifp & _TIF_SLD) > switch_to_sld(prev_p); > > void switch_to_sld(struct task_struct *prev) > { > __sld_msr_set(true); > clear_tsk_thread_flag(prev, TIF_SLD); > } > > Which re-enables split lock checking for the next process to run. But > mysteriously clears the TIF_SLD bit on the previous task.
Did Peter mean to disable it only for the current timeslice and re-enable it for the next time its scheduled?
> > I think we need to consider TIF_SLD state of both previous and next > process when deciding what to do with the MSR. Three cases: > > 1) If they are both the same, leave the MSR alone it is (probably) right (modulo > the other thread having messed with it). > 2) Next process has _TIF_SLD set ... disable checking > 3) Next process doesn't have _TIF_SLD set ... enable checking > > So please look closely at the new version of switch_to_sld() which is > now called unconditonally on every switch ... but commonly will do > nothing. ... > + /* > + * Disable the split lock detection for this task so it can make > + * progress and set TIF_SLD so the detection is reenabled via > + * switch_to_sld() when the task is scheduled out. > + */ > + __sld_msr_set(false); > + set_tsk_thread_flag(current, TIF_SLD); > + return true; > +} > + > +void switch_to_sld(struct task_struct *prev, struct task_struct *next) > +{ > + bool prevflag = test_tsk_thread_flag(prev, TIF_SLD); > + bool nextflag = test_tsk_thread_flag(next, TIF_SLD); > + > + /* > + * If we are switching between tasks that have the same > + * need for split lock checking, then the MSR is (probably) > + * right (modulo the other thread messing with it. > + * Otherwise look at whether the new task needs split > + * lock enabled. > + */ > + if (prevflag != nextflag) > + __sld_msr_set(nextflag); > +}
I might be missing something but shouldnt this be !nextflag given the flag being unset is when the task wants sld?
| |