linux-kernel - Re: [PATCH 6/6] tick/common: optimize cpumask

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAAH8bW-o_zz_C_NFnjL3uP1BXyC4OF-BAR2Dk2Xd-DFDOZpodQ@mail.gmail.com>
Date: Tue, 14 May 2024 09:47:32 -0700
From: Yury Norov <yury.norov@...il.com>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: linux-kernel@...r.kernel.org, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, "Paul E. McKenney" <paulmck@...nel.org>, 
	"Rafael J. Wysocki" <rafael@...nel.org>, Anna-Maria Behnsen <anna-maria@...utronix.de>, 
	Ben Segall <bsegall@...gle.com>, Daniel Bristot de Oliveira <bristot@...hat.com>, 
	Dietmar Eggemann <dietmar.eggemann@....com>, Frederic Weisbecker <frederic@...nel.org>, 
	Imran Khan <imran.f.khan@...cle.com>, Ingo Molnar <mingo@...hat.com>, 
	Johannes Weiner <hannes@...xchg.org>, Juri Lelli <juri.lelli@...hat.com>, 
	Leonardo Bras <leobras@...hat.com>, Mel Gorman <mgorman@...e.de>, 
	Peter Zijlstra <peterz@...radead.org>, Rik van Riel <riel@...riel.com>, 
	Steven Rostedt <rostedt@...dmis.org>, Tejun Heo <tj@...nel.org>, 
	Valentin Schneider <vschneid@...hat.com>, Vincent Guittot <vincent.guittot@...aro.org>, 
	Waiman Long <longman@...hat.com>, Zefan Li <lizefan.x@...edance.com>, cgroups@...r.kernel.org
Subject: Re: [PATCH 6/6] tick/common: optimize cpumask_equal() usage

On Tue, May 14, 2024 at 1:42 AM Thomas Gleixner <tglx@...utronix.de> 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@...il.com>
> >> ---
> >>  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