| Message ID | 20220718192844.1805158-12-yury.norov@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce DEBUG_BITMAP config option and bitmap_check_params() | expand |
On Mon, Jul 18 2022 at 12:28, Yury Norov wrote: > tick_check_preferred() calls cpumask_equal() even if > curdev->cpumask == newdev->cpumask. Fix it. What's to fix here? It's a pointless operation in a slow path and all your "fix' is doing is to make the code larger. Thanks, tglx
On Sat, Aug 06 2022 at 10:30, Thomas Gleixner wrote: > On Mon, Jul 18 2022 at 12:28, Yury Norov wrote: > >> tick_check_preferred() calls cpumask_equal() even if >> curdev->cpumask == newdev->cpumask. Fix it. > > What's to fix here? It's a pointless operation in a slow path and all > your "fix' is doing is to make the code larger. In fact cpumask_equal() should have the ptr1 == ptr2 check, so you don't have to add it all over the place. Thanks, tglx
On Mon, Aug 08, 2022 at 01:42:54PM +0200, Thomas Gleixner wrote: > On Sat, Aug 06 2022 at 10:30, Thomas Gleixner wrote: > > On Mon, Jul 18 2022 at 12:28, Yury Norov wrote: > > > >> tick_check_preferred() calls cpumask_equal() even if > >> curdev->cpumask == newdev->cpumask. Fix it. > > > > What's to fix here? It's a pointless operation in a slow path and all > > your "fix' is doing is to make the code larger. Pointless operation in a slow path is still pointless. > In fact cpumask_equal() should have the ptr1 == ptr2 check, so you don't > have to add it all over the place. This adds to the image size: add/remove: 1/1 grow/shrink: 24/3 up/down: 507/-46 (461) The more important, cpumask shouldn't check parameters because this is an internal function. This whole series point is about adding such checks under DEBUG_BITMAP config, and not affecting general case. What about adding cpumask_equal__addr? (Any better name is welcome.) Thanks, Yury
On 08/08/2022 18.38, Yury Norov wrote: > On Mon, Aug 08, 2022 at 01:42:54PM +0200, Thomas Gleixner wrote: >> On Sat, Aug 06 2022 at 10:30, Thomas Gleixner wrote: >>> On Mon, Jul 18 2022 at 12:28, Yury Norov wrote: >>> >>>> tick_check_preferred() calls cpumask_equal() even if >>>> curdev->cpumask == newdev->cpumask. Fix it. >>> >>> What's to fix here? It's a pointless operation in a slow path and all >>> your "fix' is doing is to make the code larger. > > Pointless operation in a slow path is still pointless. > >> In fact cpumask_equal() should have the ptr1 == ptr2 check, so you don't >> have to add it all over the place. > > This adds to the image size: > add/remove: 1/1 grow/shrink: 24/3 up/down: 507/-46 (461) > > The more important, cpumask shouldn't check parameters because this is > an internal function. This whole series point is about adding such checks > under DEBUG_BITMAP config, and not affecting general case. Yury, calling bitmap_equal (and by extension cpumask_equal) with something that happens in some cases to be the same pointer for both operands is not a bug. If you want to optimize that case, add a check in __bitmap_equal(), it will add a few bytes (maybe 2-4 instructions) to the kernel image, much less than what this whole series does by open-coding that check all over, and since it's comparing two registers, it won't in any way affect the performance of the case where the pointers are distinct. Rasmus
diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c index 46789356f856..fdd5ae1a074b 100644 --- a/kernel/time/tick-common.c +++ b/kernel/time/tick-common.c @@ -316,9 +316,13 @@ static bool tick_check_preferred(struct clock_event_device *curdev, * Use the higher rated one, but prefer a CPU local device with a lower * rating than a non-CPU local device */ - return !curdev || - newdev->rating > curdev->rating || - !cpumask_equal(curdev->cpumask, newdev->cpumask); + if (!curdev || newdev->rating > curdev->rating) + return true; + + if (newdev->cpumask == curdev->cpumask) + return false; + + return !cpumask_equal(curdev->cpumask, newdev->cpumask); } /*
tick_check_preferred() calls cpumask_equal() even if curdev->cpumask == newdev->cpumask. Fix it. Caught with CONFIG_DEBUG_BITMAP: [ 0.079109] Call trace: [ 0.079124] __bitmap_check_params+0x144/0x250 [ 0.079161] tick_check_replacement+0x1a4/0x320 [ 0.079203] tick_check_new_device+0x50/0x110 [ 0.079237] clockevents_register_device+0x74/0x1c0 [ 0.079268] dummy_timer_starting_cpu+0x6c/0x80 [ 0.079310] cpuhp_invoke_callback+0x104/0x20c [ 0.079353] cpuhp_invoke_callback_range+0x70/0xf0 [ 0.079401] notify_cpu_starting+0xac/0xcc [ 0.079434] secondary_start_kernel+0xe4/0x154 [ 0.079471] __secondary_switched+0xa0/0xa4 [ 0.079516] ---[ end trace 0000000000000000 ]--- [ 0.079542] b1: ffffbfec4703b890 [ 0.079553] b2: ffffbfec4703b890 [ 0.079566] b3: 0 [ 0.079576] nbits: 256 [ 0.079588] start: 0 [ 0.079598] off: 0 [ 0.079609] Bitmap: parameters check failed [ 0.079619] include/linux/bitmap.h [419]: bitmap_equal Signed-off-by: Yury Norov <yury.norov@gmail.com> --- kernel/time/tick-common.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)