On Tue, May 14, 2024 at 1:42 AM Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote: > > On Tue, May 14 2024 at 10:31, Thomas Gleixner wrote: > > On Mon, May 13 2024 at 15:01, Yury Norov wrote: > >> Some functions in the file call cpumask_equal() with src1p == src2p. > >> We can obviously just skip comparison entirely in this case. > >> > >> This patch fixes cpumask_equal invocations when boot-test or LTP detect > >> such condition. > > > > Please write your changelogs in imperative mood. > > > > git grep 'This patch' Documentation/process/ > > > > Also please see Documentation/process/maintainer-tip.rst > > > >> Signed-off-by: Yury Norov <yury.norov@xxxxxxxxx> > >> --- > >> kernel/time/tick-common.c | 15 +++++++++++---- > >> 1 file changed, 11 insertions(+), 4 deletions(-) > >> > >> diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c > >> index d88b13076b79..b31fef292833 100644 > >> --- a/kernel/time/tick-common.c > >> +++ b/kernel/time/tick-common.c > >> @@ -253,7 +253,8 @@ static void tick_setup_device(struct tick_device *td, > >> * When the device is not per cpu, pin the interrupt to the > >> * current cpu: > >> */ > >> - if (!cpumask_equal(newdev->cpumask, cpumask)) > >> + if (newdev->cpumask != cpumask && > >> + !cpumask_equal(newdev->cpumask, cpumask)) > >> irq_set_affinity(newdev->irq, cpumask); > > > > I'm not seeing the benefit. This is slow path setup code so the extra > > comparison does not really buy anything aside of malformatted line > > breaks. > > Instead of sprinkling these conditional all over the place, can't you > just do the obvious and check for ptr1 == ptr2 in bitmap_copy() and > bitmap_equal()? I proposed this a while (few years) ago, and it has been rejected. On bitmaps level we decided not to do that for the reasons memcpy() and memcmp() doesn't, and on cpumasks and nodemasks level it hasn't been discussed at all. Now that most of bitmap ops have inline and outline implementation, we technically can move this checks in outline code, as inline bitmap ops are very lightweight already. So I see the following options: - Implement these sanity checks in outline bitmap API (lib/bitmap.c); - Implement them on cpumask and nodemask level; or - add a new family of helpers that do this check, like bitmap_copy_if_needed() (better name appreciated). The argument against #1 and #2 these days was that memcpy() and similarly bitmap_copy() with dst == src may be a sign of error, and we don't want to add a code that optimizes for it. Now, I ran the kernel through the LTP test and in practice all the cases that I spot look pretty normal. So I can continue sprinkling the checks once a few years, or do something like described above. Can you / everyone please share your opinion? Thanks, Yury
