linux-kernel - Re: [PATCH 11/12] sched: use for_each

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]

Date:   Mon, 9 Jul 2018 17:12:58 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Daniel Vetter <daniel.vetter@...ll.ch>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        DRI Development <dri-devel@...ts.freedesktop.org>,
        Intel Graphics Development <intel-gfx@...ts.freedesktop.org>,
        Daniel Vetter <daniel.vetter@...el.com>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH 11/12] sched: use for_each_if in topology.h

On Mon, Jul 09, 2018 at 05:00:07PM +0200, Daniel Vetter wrote:
> On Mon, Jul 9, 2018 at 12:36 PM, Peter Zijlstra <peterz@...radead.org> wrote:
> > On Mon, Jul 09, 2018 at 10:36:49AM +0200, Daniel Vetter wrote:

> >>  #define for_each_node_with_cpus(node)                        \
> >>       for_each_online_node(node)                      \
> >> -             if (nr_cpus_node(node))
> >> +             for_each_if (nr_cpus_node(node))
> >
> > Not having gotten any of the other patches, I'm not really sure what
> > this does and such, but improve readability it does not :/
> 
> Patch 1 in this series, which I dumped onto lkml as a whole:
> 
> https://lkml.org/lkml/2018/7/9/179

Right, so while I don't object to being Cc'ed to the whole series, I do
mind not being Cc'ed to at least the generic bits required to understand
the patch I do have to look at.

> Imo it does improve readability for the if (!cond) {} else pattern.
> And (assuming my grep fu isn't too badly wrong) most places in the
> kernel do use this pattern in for_each macros, so I guess its a real
> thing. We've definitely hit it plenty in drm iterators (but we seem to
> like if() checks in iterator macros maybe a bit too much).
> 
> I'm happy to drop this patch tough if you deem it offensive.

I'd just like to understand it better; what compiler complains about
this and is the warning otherwise useful? These things don't seem
mentioned in that initial patch either.

IOW I suppose I'm asking for the justification of this churn. If it's
really needed and useful so be it, but so far I'm not seeing any.

At a while guess I'd say this is something new in gcc-8 (and while I
have that installed on some machines, it doesn't seem to be the default,
and so I've not actually seen its output). But is the warning actually
useful, should we not just kill the warning like we tend to do some
really silly ones.