Date:   Fri, 21 Oct 2022 16:03:41 -0300
From:   "Guilherme G. Piccoli" <gpiccoli@...lia.com>
To:     Dave Hansen <dave.hansen@...el.com>, x86@...nel.org,
        linux-kernel@...r.kernel.org
Cc:     tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
        dave.hansen@...ux.intel.com, hpa@...or.com, luto@...nel.org,
        corbet@....net, linux-doc@...r.kernel.org, kernel-dev@...lia.com,
        kernel@...ccoli.net, Andre Almeida <andrealmeid@...lia.com>,
        Fenghua Yu <fenghua.yu@...el.com>,
        Joshua Ashton <joshua@...ggi.es>,
        Melissa Wen <mwen@...lia.com>,
        Paul Gofman <pgofman@...eweavers.com>,
        Pavel Machek <pavel@...x.de>,
        Pierre-Loup Griffais <pgriffais@...vesoftware.com>,
        Tony Luck <tony.luck@...el.com>,
        Zebediah Figura <zfigura@...eweavers.com>
Subject: Re: [PATCH V2] x86/split_lock: Add sysctl to control the misery mode

Hi Dave, thanks for the thorough review!
Comments inline below:

On 21/10/2022 14:27, Dave Hansen wrote:
> [...]
>> +For x86 CPUs supporting the split lock detection mechanism, this parameter
>> +allows the users to turn off what is called "the misery mode", which
>> +introduces intentional delay in userspace applications that split locks.
>> +The goal of the misery mode is to prevent using such unaligned access to
>> +DoS the system dropping the performance overall, but some of these split
>> +locking programs are legacy and/or proprietary software that cannot be fixed,
>> +so using this sysctl is a way to allow them to run with a decent performance.
> 
> I think this is missing a lot of context.  End users looking here won't
> even know what a split lock *is*.  Please either refer over to the real
> documentation on this issue or write a brief description about what's
> going on.
> 
> How about this?
> 
> 	On x86, each "split lock" imposes a system-wide performance
> 	penalty.  On larger systems, large numbers of split locks from
> 	unprivileged users can result in denials of service to well-
> 	behaved and potentially more important users.
> 
> 	The kernel mitigates these bad users by detecting split locks
> 	and imposing penalties: forcing them to wait and only allowing
> 	one core to execute split locks at a time.
> 
> 	These mitigations can make those bad applications unbearably
> 	slow.  Setting split_lock_mitigate=0 may restore some
> 	application performance, but will also increase system exposure
> 	to denial of service attacks from split lock users.
> 
>> += ===================================================================
>> +0 Disables the misery mode - just warns the split lock on kernel log.
> 
> ... and exposes the system to Denial-of-Service attacks.  That's an
> awfully big side-effect to not mention.
> 
>> +1 Enables the misery mode (this is the default) - penalizes the split
>> +  lockers with intentional performance degradation.
>> += ===================================================================
> 
> As much as I love the misery terminology, let's try to use one term.
> Let's either call it "misery" *or* "mitigations", not both.
> 

OK, regarding the documentation, I'll follow your suggestion in the V3,
good stuff.


>> [...]
>> -static void __split_lock_reenable(struct work_struct *work)
>> +static void __split_lock_reenable_sem(struct work_struct *work)
>>  {
> 
> "sem" is a pretty crummy name.  Wouldn't
> 
> 	__split_lock_reenable_unlock()
> 
> be much more clear?
> 

Agreed...


>> [...]
> Better yet, do you *really* need two functions and two
> DECLARE_DELAYED_WORK()'s?
> 
> You could have a single delayed_work, and then just do:
> 
> static void split_lock_warn(unsigned long ip)
> {
> 	bool need_release_sem = false;
> 	...
> 
> 	if (down_interruptible(&buslock_sem) == -EINTR)
> 		return;
> 	need_release_sem = true;
> 
> 
> Then, farther down, you do:
> 
> 	split_lock_reenable->data = need_release_sem;
> 	schedule_delayed_work_on(cpu, &split_lock_reenable);
> 
> Then, in the work func:
> 	
> 	bool need_release_sem = work->data;
> 
> 	if (need_release_sem)
> 		up(...);
> 
> That's nice and compact.  It's also logically easy to follow because you
> can see how the need_release_sem gets set only after the
> down_interruptible().  It's also nice to have both sites share the
> 'need_release_sem' naming for grepping.
> 

...but, this is a very good suggestion, and will eliminate the need for
two delayed_works, right?


>> [...]

>> +	struct delayed_work *wk;
> 
> I think we can spare two bytes to make this "work".
> 
>> [...]
> 
> It's a little confusing to set:
> 
> 	wk = &split_lock_reenable_sem;
> 
> and then not use it.
> 
> I'd probably set it below the lock check and return.
> 
>> +	} else
>> +		wk = &split_lock_reenable;
> 
> Brackets, please:
> 
> 	} else {
> 		wk = &split_lock_reenable;
> 	}
> 
> (if you keep this hunk).
> 

But then we're back to discussing the approach of multiple delayed works.

I guess I prefer your idea of passing the state and have a single one,
will do this in V3 OK? If you or anybody else disagrees and prefer the
approach of 2 workers, let me know.

Cheers,


Guilherme

Powered by blists - more mailing lists