| From sj@kernel.org Tue Jul 16 20:33:43 2024 |
| From: SeongJae Park <sj@kernel.org> |
| Date: Tue, 16 Jul 2024 11:33:28 -0700 |
| Subject: minmax: clamp more efficiently by avoiding extra comparison |
| To: stable@vger.kernel.org, gregkh@linuxfoundation.org |
| Cc: "Jason A. Donenfeld" <Jason@zx2c4.com>, linux-kernel@vger.kernel.org, Andy Shevchenko <andriy.shevchenko@linux.intel.com>, Kees Cook <keescook@chromium.org>, Andrew Morton <akpm@linux-foundation.org>, SeongJae Park <sj@kernel.org> |
| Message-ID: <20240716183333.138498-4-sj@kernel.org> |
| |
| From: "Jason A. Donenfeld" <Jason@zx2c4.com> |
| |
| commit 2122e2a4efc2cd139474079e11939b6e07adfacd upstream. |
| |
| Currently the clamp algorithm does: |
| |
| if (val > hi) |
| val = hi; |
| if (val < lo) |
| val = lo; |
| |
| But since hi > lo by definition, this can be made more efficient with: |
| |
| if (val > hi) |
| val = hi; |
| else if (val < lo) |
| val = lo; |
| |
| So fix up the clamp and clamp_t functions to do this, adding the same |
| argument checking as for min and min_t. |
| |
| For simple cases, code generation on x86_64 and aarch64 stay about the |
| same: |
| |
| before: |
| cmp edi, edx |
| mov eax, esi |
| cmova edi, edx |
| cmp edi, esi |
| cmovnb eax, edi |
| ret |
| after: |
| cmp edi, esi |
| mov eax, edx |
| cmovnb esi, edi |
| cmp edi, edx |
| cmovb eax, esi |
| ret |
| |
| before: |
| cmp w0, w2 |
| csel w8, w0, w2, lo |
| cmp w8, w1 |
| csel w0, w8, w1, hi |
| ret |
| after: |
| cmp w0, w1 |
| csel w8, w0, w1, hi |
| cmp w0, w2 |
| csel w0, w8, w2, lo |
| ret |
| |
| On MIPS64, however, code generation improves, by removing arithmetic in |
| the second branch: |
| |
| before: |
| sltu $3,$6,$4 |
| bne $3,$0,.L2 |
| move $2,$6 |
| |
| move $2,$4 |
| .L2: |
| sltu $3,$2,$5 |
| bnel $3,$0,.L7 |
| move $2,$5 |
| |
| .L7: |
| jr $31 |
| nop |
| after: |
| sltu $3,$4,$6 |
| beq $3,$0,.L13 |
| move $2,$6 |
| |
| sltu $3,$4,$5 |
| bne $3,$0,.L12 |
| move $2,$4 |
| |
| .L13: |
| jr $31 |
| nop |
| |
| .L12: |
| jr $31 |
| move $2,$5 |
| |
| For more complex cases with surrounding code, the effects are a bit |
| more complicated. For example, consider this simplified version of |
| timestamp_truncate() from fs/inode.c on x86_64: |
| |
| struct timespec64 timestamp_truncate(struct timespec64 t, struct inode *inode) |
| { |
| struct super_block *sb = inode->i_sb; |
| unsigned int gran = sb->s_time_gran; |
| |
| t.tv_sec = clamp(t.tv_sec, sb->s_time_min, sb->s_time_max); |
| if (t.tv_sec == sb->s_time_max || t.tv_sec == sb->s_time_min) |
| t.tv_nsec = 0; |
| return t; |
| } |
| |
| before: |
| mov r8, rdx |
| mov rdx, rsi |
| mov rcx, QWORD PTR [r8] |
| mov rax, QWORD PTR [rcx+8] |
| mov rcx, QWORD PTR [rcx+16] |
| cmp rax, rdi |
| mov r8, rcx |
| cmovge rdi, rax |
| cmp rdi, rcx |
| cmovle r8, rdi |
| cmp rax, r8 |
| je .L4 |
| cmp rdi, rcx |
| jge .L4 |
| mov rax, r8 |
| ret |
| .L4: |
| xor edx, edx |
| mov rax, r8 |
| ret |
| |
| after: |
| mov rax, QWORD PTR [rdx] |
| mov rdx, QWORD PTR [rax+8] |
| mov rax, QWORD PTR [rax+16] |
| cmp rax, rdi |
| jg .L6 |
| mov r8, rax |
| xor edx, edx |
| .L2: |
| mov rax, r8 |
| ret |
| .L6: |
| cmp rdx, rdi |
| mov r8, rdi |
| cmovge r8, rdx |
| cmp rax, r8 |
| je .L4 |
| xor eax, eax |
| cmp rdx, rdi |
| cmovl rax, rsi |
| mov rdx, rax |
| mov rax, r8 |
| ret |
| .L4: |
| xor edx, edx |
| jmp .L2 |
| |
| In this case, we actually gain a branch, unfortunately, because the |
| compiler's replacement axioms no longer as cleanly apply. |
| |
| So all and all, this change is a bit of a mixed bag. |
| |
| Link: https://lkml.kernel.org/r/20220926133435.1333846-2-Jason@zx2c4.com |
| Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> |
| Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> |
| Cc: Kees Cook <keescook@chromium.org> |
| Signed-off-by: Andrew Morton <akpm@linux-foundation.org> |
| (cherry picked from commit 2122e2a4efc2cd139474079e11939b6e07adfacd) |
| Signed-off-by: SeongJae Park <sj@kernel.org> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| --- |
| include/linux/minmax.h | 2 +- |
| 1 file changed, 1 insertion(+), 1 deletion(-) |
| |
| --- a/include/linux/minmax.h |
| +++ b/include/linux/minmax.h |
| @@ -38,7 +38,7 @@ |
| __cmp_once(x, y, __UNIQUE_ID(__x), __UNIQUE_ID(__y), op)) |
| |
| #define __clamp(val, lo, hi) \ |
| - __cmp(__cmp(val, lo, >), hi, <) |
| + ((val) >= (hi) ? (hi) : ((val) <= (lo) ? (lo) : (val))) |
| |
| #define __clamp_once(val, lo, hi, unique_val, unique_lo, unique_hi) ({ \ |
| typeof(val) unique_val = (val); \ |