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
