Re: [PATCH v15] x86/split_lock: Enable split lock detection by kernel

[Luck, Tony]

On Sat, Jan 25, 2020 at 04:25:25PM -0500, Arvind Sankar wrote:
> 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?

He's seen and commented on this thread since I made this comment. So
I'll assume not.  Things get really noisy on the console (even with
the rate limit) if split lock detection is re-enabled after a context
switch (my new test highlighted this!)

> > +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?

That logic is convoluted ... but Thomas showed me a much better
way that is also much simpler ... so this code has gone now. The
new version is far easier to read (argument is flags for the new task
that we are switching to)

void switch_to_sld(unsigned long tifn)
{
        __sld_msr_set(tifn & _TIF_SLD);
}

-Tony