linux-kernel - Re: [PATCH] slab.h: Avoid using & for logical and of booleans

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: <CAKgT0Ue59US_f-cZtoA=yVbFJ03ca5OMce2opUdQcsvgd8LWMw@mail.gmail.com>
Date:   Mon, 5 Nov 2018 16:11:04 -0800
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     bvanassche@....org
Cc:     linux@...musvillemoes.dk,
        Andrew Morton <akpm@...ux-foundation.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Vlastimil Babka <vbabka@...e.cz>,
        Mel Gorman <mgorman@...hsingularity.net>,
        Christoph Lameter <cl@...ux.com>, guro@...com,
        Pekka Enberg <penberg@...nel.org>,
        David Rientjes <rientjes@...gle.com>,
        Joonsoo Kim <iamjoonsoo.kim@....com>,
        linux-mm <linux-mm@...ck.org>
Subject: Re: [PATCH] slab.h: Avoid using & for logical and of booleans

On Mon, Nov 5, 2018 at 4:01 PM Bart Van Assche <bvanassche@....org> wrote:
>
> On Mon, 2018-11-05 at 14:48 -0800, Alexander Duyck wrote:
> > On Mon, Nov 5, 2018 at 2:41 PM Bart Van Assche <bvanassche@....org> wrote:
> > > How about this version, still untested? My compiler is able to evaluate
> > > the switch expression if the argument is constant.
> > >
> > >  static __always_inline enum kmalloc_cache_type kmalloc_type(gfp_t flags)
> > >  {
> > > -       int is_dma = 0;
> > > -       int type_dma = 0;
> > > -       int is_reclaimable;
> > > +       unsigned int dr = !!(flags & __GFP_RECLAIMABLE);
> > >
> > >  #ifdef CONFIG_ZONE_DMA
> > > -       is_dma = !!(flags & __GFP_DMA);
> > > -       type_dma = is_dma * KMALLOC_DMA;
> > > +       dr |= !!(flags & __GFP_DMA) << 1;
> > >  #endif
> > >
> > > -       is_reclaimable = !!(flags & __GFP_RECLAIMABLE);
> > > -
> > >         /*
> > >          * If an allocation is both __GFP_DMA and __GFP_RECLAIMABLE, return
> > >          * KMALLOC_DMA and effectively ignore __GFP_RECLAIMABLE
> > >          */
> > > -       return type_dma + (is_reclaimable & !is_dma) * KMALLOC_RECLAIM;
> > > +       switch (dr) {
> > > +       default:
> > > +       case 0:
> > > +               return 0;
> > > +       case 1:
> > > +               return KMALLOC_RECLAIM;
> > > +       case 2:
> > > +       case 3:
> > > +               return KMALLOC_DMA;
> > > +       }
> > >  }
> >
> > Doesn't this defeat the whole point of the code which I thought was to
> > avoid conditional jumps and branches? Also why would you bother with
> > the "dr" value when you could just mask the flags value and switch on
> > that directly?
>
> Storing the relevant bits of 'flags' in the 'dr' variable avoids that the
> bit selection expressions have to be repeated and allows to use a switch
> statement instead of multiple if / else statements.

Really they shouldn't have to be repeated. You essentially have just 3
cases. 0, __GFP_RECLAIMABLE, and the default case.

> Most kmalloc() calls pass a constant to the gfp argument. That allows the
> compiler to evaluate kmalloc_type() at compile time. So the conditional jumps
> and branches only appear when the gfp argument is not a constant. What makes
> you think it is important to optimize for that case?
>
> Bart.

I didn't really think it was all that important to optimize, but I
thought that was what you were trying to maintain with the earlier
patch since it was converting things to a table lookup.

If we really don't care then why even bother with the switch statement
anyway? It seems like you could just do one ternary operator and be
done with it. Basically all you need is:
return (defined(CONFIG_ZONE_DMA) && (flags & __GFP_DMA)) ? KMALLOC_DMA :
        (flags & __GFP_RECLAIMABLE) ? KMALLOC_RECLAIM : 0;

Why bother with all the extra complexity of the switch statement?

Thanks.

- Alex