linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [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).