Re: [bug report] mm, slab/slub: introduce kmalloc-reclaimable caches

[Dan Carpenter <dan.carpenter@oracle.com>]

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