On 11/13/18 7:22 PM, Vlastimil Babka wrote: > On 11/12/18 10:55 AM, David Laight wrote: >> From: Vlastimil Babka [mailto:vbabka@xxxxxxx] >>> Sent: 09 November 2018 19:16 >> ... >>> This? Not terribly elegant, but I don't see a nicer way right now... >> >> Maybe just have two copies of the function body? >> >> static __always_inline enum kmalloc_cache_type kmalloc_type(gfp_t flags) >> { >> #ifndef CONFIG_ZONE_DMA >> return flags & __GFP_RECLAIMABLE ? KMALLOC_RECLAIM : KMALLOC_NORMAL; >> #else >> if (likely((flags & (__GFP_DMA | __GFP_RECLAIMABLE)) == 0)) >> return KMALLOC_NORMAL; >> return flags & __GFP_DMA ? KMALLOC_DMA : KMALLOC_RECLAIM; >> #endif >> } > > OK that's probably the most straightforward to follow, thanks. > Note that for CONFIG_ZONE_DMA=n the result is identical to original code and > all other attempts. flags & __GFP_DMA is converted to 1/0 index without branches > or cmovs or whatnot. Ping? Seems like people will report duplicates until the sparse warning is gone in mainline... Also CC linux-mm which was somehow lost. ----8<---- >From 40735b637b28c3e5798bc7e90f72f349050c2045 Mon Sep 17 00:00:00 2001 From: Vlastimil Babka <vbabka@xxxxxxx> Date: Fri, 9 Nov 2018 08:47:12 +0100 Subject: [PATCH] mm, slab: fix sparse warning in kmalloc_type() Multiple people have reported the following sparse warning: ./include/linux/slab.h:332:43: warning: dubious: x & !y The minimal fix would be to change the logical & to boolean &&, which emits the same code, but Andrew has suggested that the branch-avoiding tricks are maybe not worthwile. David Laight provided a nice comparison of disassembly of multiple variants, which shows that the current version produces a 4 deep dependency chain, and fixing the sparse warning by changing logical and to multiplication emits an IMUL, making it even more expensive. The code as rewritten by this patch yielded the best disassembly, with a single predictable branch for the most common case, and a ternary operator for the rest, which gcc seems to compile without a branch or cmov by itself. The result should be more readable, without a sparse warning and probably also faster for the common case. Reported-by: Bart Van Assche <bvanassche@xxxxxxx> Reported-by: Darryl T. Agostinelli <dagostinelli@xxxxxxxxx> Reported-by: Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx> Suggested-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> Suggested-by: David Laight <David.Laight@xxxxxxxxxx> Fixes: 1291523f2c1d ("mm, slab/slub: introduce kmalloc-reclaimable caches") Signed-off-by: Vlastimil Babka <vbabka@xxxxxxx> --- include/linux/slab.h | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/include/linux/slab.h b/include/linux/slab.h index 918f374e7156..6d5009f29ce5 100644 --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -314,22 +314,22 @@ kmalloc_caches[NR_KMALLOC_TYPES][KMALLOC_SHIFT_HIGH + 1]; static __always_inline enum kmalloc_cache_type kmalloc_type(gfp_t flags) { - int is_dma = 0; - int type_dma = 0; - int is_reclaimable; - #ifdef CONFIG_ZONE_DMA - is_dma = !!(flags & __GFP_DMA); - type_dma = is_dma * KMALLOC_DMA; -#endif - - is_reclaimable = !!(flags & __GFP_RECLAIMABLE); + /* + * The most common case is KMALLOC_NORMAL, so test for it + * with a single branch for both flags. + */ + if (likely((flags & (__GFP_DMA | __GFP_RECLAIMABLE)) == 0)) + return KMALLOC_NORMAL; /* - * If an allocation is both __GFP_DMA and __GFP_RECLAIMABLE, return - * KMALLOC_DMA and effectively ignore __GFP_RECLAIMABLE + * At least one of the flags has to be set. If both are, __GFP_DMA + * is more important. */ - return type_dma + (is_reclaimable & !is_dma) * KMALLOC_RECLAIM; + return flags & __GFP_DMA ? KMALLOC_DMA : KMALLOC_RECLAIM; +#else + return flags & __GFP_RECLAIMABLE ? KMALLOC_RECLAIM : KMALLOC_NORMAL; +#endif } /* -- 2.19.1
