* [bug report] mm, slab/slub: introduce kmalloc-reclaimable caches
@ 2018-11-09 17:17 Dan Carpenter
2018-11-09 17:28 ` Vlastimil Babka
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2018-11-09 17:17 UTC (permalink / raw)
To: vbabka; +Cc: linux-mm
Hello Vlastimil Babka,
The patch 1291523f2c1d: "mm, slab/slub: introduce kmalloc-reclaimable
caches" from Oct 26, 2018, leads to the following static checker
warning:
./include/linux/slab.h:585 kmalloc_node()
warn: array off by one? 'kmalloc_caches[kmalloc_type(flags)]' '0-3 == 3'
./include/linux/slab.h
298 /*
299 * Whenever changing this, take care of that kmalloc_type() and
300 * create_kmalloc_caches() still work as intended.
301 */
302 enum kmalloc_cache_type {
303 KMALLOC_NORMAL = 0,
304 KMALLOC_RECLAIM,
305 #ifdef CONFIG_ZONE_DMA
306 KMALLOC_DMA,
307 #endif
308 NR_KMALLOC_TYPES
The kmalloc_caches[] array has NR_KMALLOC_TYPES elements.
309 };
310
311 #ifndef CONFIG_SLOB
312 extern struct kmem_cache *
313 kmalloc_caches[NR_KMALLOC_TYPES][KMALLOC_SHIFT_HIGH + 1];
314
315 static __always_inline enum kmalloc_cache_type kmalloc_type(gfp_t flags)
316 {
317 int is_dma = 0;
318 int type_dma = 0;
319 int is_reclaimable;
320
321 #ifdef CONFIG_ZONE_DMA
322 is_dma = !!(flags & __GFP_DMA);
323 type_dma = is_dma * KMALLOC_DMA;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
KMALLOC_DMA is the last possible valid index.
324 #endif
325
326 is_reclaimable = !!(flags & __GFP_RECLAIMABLE);
327
328 /*
329 * If an allocation is both __GFP_DMA and __GFP_RECLAIMABLE, return
330 * KMALLOC_DMA and effectively ignore __GFP_RECLAIMABLE
331 */
332 return type_dma + (is_reclaimable & !is_dma) * KMALLOC_RECLAIM;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
We're adding one to it. This is mm/ so I assume this works, but it's
pretty confusing.
333 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] mm, slab/slub: introduce kmalloc-reclaimable caches
2018-11-09 17:17 [bug report] mm, slab/slub: introduce kmalloc-reclaimable caches Dan Carpenter
@ 2018-11-09 17:28 ` Vlastimil Babka
2018-11-13 17:02 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Vlastimil Babka @ 2018-11-09 17:28 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-mm
On 11/9/18 6:17 PM, Dan Carpenter wrote:
> Hello Vlastimil Babka,
Hi,
> The patch 1291523f2c1d: "mm, slab/slub: introduce kmalloc-reclaimable
> caches" from Oct 26, 2018, leads to the following static checker
> warning:
>
> ./include/linux/slab.h:585 kmalloc_node()
> warn: array off by one? 'kmalloc_caches[kmalloc_type(flags)]' '0-3 == 3'
I believe that's a false positive.
> ./include/linux/slab.h
> 298 /*
> 299 * Whenever changing this, take care of that kmalloc_type() and
> 300 * create_kmalloc_caches() still work as intended.
> 301 */
> 302 enum kmalloc_cache_type {
> 303 KMALLOC_NORMAL = 0,
> 304 KMALLOC_RECLAIM,
> 305 #ifdef CONFIG_ZONE_DMA
> 306 KMALLOC_DMA,
> 307 #endif
> 308 NR_KMALLOC_TYPES
>
>
> The kmalloc_caches[] array has NR_KMALLOC_TYPES elements.
Yes.
> 309 };
> 310
> 311 #ifndef CONFIG_SLOB
> 312 extern struct kmem_cache *
> 313 kmalloc_caches[NR_KMALLOC_TYPES][KMALLOC_SHIFT_HIGH + 1];
> 314
> 315 static __always_inline enum kmalloc_cache_type kmalloc_type(gfp_t flags)
> 316 {
> 317 int is_dma = 0;
> 318 int type_dma = 0;
> 319 int is_reclaimable;
> 320
> 321 #ifdef CONFIG_ZONE_DMA
> 322 is_dma = !!(flags & __GFP_DMA);
> 323 type_dma = is_dma * KMALLOC_DMA;
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> KMALLOC_DMA is the last possible valid index.
Yes, but type_dma gains the value of KMALLOC_DMA only when is_dma is 1.
> 324 #endif
> 325
> 326 is_reclaimable = !!(flags & __GFP_RECLAIMABLE);
> 327
> 328 /*
> 329 * If an allocation is both __GFP_DMA and __GFP_RECLAIMABLE, return
> 330 * KMALLOC_DMA and effectively ignore __GFP_RECLAIMABLE
> 331 */
> 332 return type_dma + (is_reclaimable & !is_dma) * KMALLOC_RECLAIM;
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> We're adding one to it.
Only when !is_dma is 1, which means then that type_dma is 0. So it's safe.
> This is mm/ so I assume this works,
I'll... take that as a compliment :D
> but it's
> pretty confusing.
Indeed. Static checkers seem to hate my too clever code, so it's already
going away [1]. Maybe your static checker can be improved to evaluate
this better? There's already a gcc bug [2] inspired by the whole thing.
Thanks!
Vlastimil
[1]
https://lore.kernel.org/lkml/cbc1fc52-dc8c-aa38-8f29-22da8bcd91c1@suse.cz/T/#u
[2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87954
> 333 }
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] mm, slab/slub: introduce kmalloc-reclaimable caches
2018-11-09 17:28 ` Vlastimil Babka
@ 2018-11-13 17:02 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2018-11-13 17:02 UTC (permalink / raw)
To: Vlastimil Babka; +Cc: linux-mm
On Fri, Nov 09, 2018 at 06:28:44PM +0100, Vlastimil Babka wrote:
> On 11/9/18 6:17 PM, Dan Carpenter wrote:
> > 315 static __always_inline enum kmalloc_cache_type kmalloc_type(gfp_t flags)
> > 316 {
> > 317 int is_dma = 0;
> > 318 int type_dma = 0;
> > 319 int is_reclaimable;
> > 320
> > 321 #ifdef CONFIG_ZONE_DMA
> > 322 is_dma = !!(flags & __GFP_DMA);
> > 323 type_dma = is_dma * KMALLOC_DMA;
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >
> > KMALLOC_DMA is the last possible valid index.
>
> Yes, but type_dma gains the value of KMALLOC_DMA only when is_dma is 1.
>
> > 324 #endif
> > 325
> > 326 is_reclaimable = !!(flags & __GFP_RECLAIMABLE);
> > 327
> > 328 /*
> > 329 * If an allocation is both __GFP_DMA and __GFP_RECLAIMABLE, return
> > 330 * KMALLOC_DMA and effectively ignore __GFP_RECLAIMABLE
> > 331 */
> > 332 return type_dma + (is_reclaimable & !is_dma) * KMALLOC_RECLAIM;
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >
> > We're adding one to it.
>
> Only when !is_dma is 1, which means then that type_dma is 0. So it's safe.
>
> > This is mm/ so I assume this works,
>
> I'll... take that as a compliment :D
>
Yes, of course.
> > but it's
> > pretty confusing.
>
> Indeed. Static checkers seem to hate my too clever code, so it's already
> going away [1]. Maybe your static checker can be improved to evaluate
> this better? There's already a gcc bug [2] inspired by the whole thing.
>
It's cool that the GCC developers think they can handle this code. I
would have to rethink how Smatch handles math entirely...
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-11-13 17:02 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-09 17:17 [bug report] mm, slab/slub: introduce kmalloc-reclaimable caches Dan Carpenter
2018-11-09 17:28 ` Vlastimil Babka
2018-11-13 17:02 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).