Re: [PATCH 1/1] mm, slub: Restore initial kmem_cache flags

[Eric Farman <farman@linux.ibm.com>]

On 9/30/20 7:37 AM, Vlastimil Babka wrote:
> On 9/30/20 1:26 PM, Eric Farman wrote:
>> The routine that applies debug flags to the kmem_cache slabs
>> inadvertantly prevents non-debug flags from being applied to
>> those same objects. That is, if slub_debug=<flag>,<slab> is
>> specified, non-debugged slabs will end up having flags of zero,
>> and the slabs will be unusable. Fix this by returning the input
>> flags for non-matching slabs as was done previously.
> 
> Thanks a lot for debugging this and sorry for the trouble!

You're welcome. Just glad I wasn't losing my mind!

> 
>> Fixes: e17f1dfba37b ("mm, slub: extend slub_debug syntax for multiple blocks")
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> 
> Acked-by: Vlastimil Babka <vbabka@suse.cz>
> 
> But with a small adjustment below:
> 
>> ---
>>  mm/slub.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mm/slub.c b/mm/slub.c
>> index d4177aecedf6..3d7c95fd6a08 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -1450,7 +1450,7 @@ slab_flags_t kmem_cache_flags(unsigned int object_size,
>>  		}
>>  	}
>>  
>> -	return slub_debug;
>> +	return flags;
> 
> To keep supporting the case of "debug flags set for all caches, with exceptions
> for listed caches", i.e. "slub_debug=FZ;-,zs_handle,zspage", we should return
> here this:
> 
> return flags | slub_debug;

Ah, cool...  I wondered about that, but didn't go far enough down the
combinations.  Does it then make sense to strip out the "if
(!slub_debug_string)" check at the beginning of the function?  As in:

----8<----

diff --git a/mm/slub.c b/mm/slub.c
index 3d7c95fd6a08..6d3574013b2f 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1413,10 +1413,6 @@ slab_flags_t kmem_cache_flags(unsigned int
object_size,
        char *next_block;
        slab_flags_t block_flags;

-       /* If slub_debug = 0, it folds into the if conditional. */
-       if (!slub_debug_string)
-               return flags | slub_debug;
-
        len = strlen(name);
        next_block = slub_debug_string;
        /* Go through all blocks of debug options, see if any matches
our slab's name */
@@ -1450,7 +1446,7 @@ slab_flags_t kmem_cache_flags(unsigned int
object_size,
                }
        }

-       return flags;
+       return flags | slub_debug;
 }
 #else /* !CONFIG_SLUB_DEBUG */
 static inline void setup_object_debug(struct kmem_cache *s,

> 
> Thanks again!
> 
>>  }
>>  #else /* !CONFIG_SLUB_DEBUG */
>>  static inline void setup_object_debug(struct kmem_cache *s,
>>
>