Re: [PATCH] failslab: add ability to filter slab caches

[Dmitry Monakhov <dmonakhov@openvz.org>]

David Rientjes <rientjes@google.com> writes:

> On Wed, 24 Feb 2010, Dmitry Monakhov wrote:
>
>> >> Example:
>> >> echo 1 > /sys/kernel/slab/skbuff_head_cache/failslab
>> >> echo 1 > /sys/kernel/debug/failslab/cache-filter
>> >> 
>> >
>> > Where's the changelog?
>> I've hoped than subject is enough. But if you want more verbose
>> changelog, take following version.
>> 
Ohh... i'm sorry i've missed other comments in your previous review.
It will be fixed in second version. Also see major note about
slab flags later in the text.
>
> Please do not attach patches to emails when sending to LKML, they are 
> preferred to be inline.  See Documentation/SubmittingPatches.
Yeah, In fact i've chose inline option but my mailer by unknown reason
change it.
>
>> This patch allow to inject faults only for specific slabs.
>> In order to preserve default behavior cache filter is off by
>> default (all caches are faulty).
>> 
>> One may define specific set of slabs like this:
>> # mark skbuff_head_cache as faulty
>> echo 1 > /sys/kernel/slab/skbuff_head_cache/failslab
>> # Turn on cache filter (off by default)
>> echo 1 > /sys/kernel/debug/failslab/cache-filter
>> # Turn on fault injection
>> echo 1 > /sys/kernel/debug/failslab/times
>> echo 1 > /sys/kernel/debug/failslab/probability
>> 
>
> Please describe your new slub_debug=a option here as well as in an update 
> to Documentation/vm/slub.txt.
Done
>
>> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
>> ---
>>  include/linux/fault-inject.h |    4 ++--
>>  include/linux/slab.h         |    4 +++-
>>  mm/failslab.c                |   18 +++++++++++++++---
>>  mm/slab.c                    |    2 +-
>>  mm/slub.c                    |   31 +++++++++++++++++++++++++++++--
>>  5 files changed, 50 insertions(+), 9 deletions(-)
>> 
>> diff --git a/include/linux/fault-inject.h b/include/linux/fault-inject.h
>> index 06ca9b2..d935647 100644
>> --- a/include/linux/fault-inject.h
>> +++ b/include/linux/fault-inject.h
>> @@ -82,9 +82,9 @@ static inline void cleanup_fault_attr_dentries(struct fault_attr *attr)
>>  #endif /* CONFIG_FAULT_INJECTION */
>>  
>>  #ifdef CONFIG_FAILSLAB
>> -extern bool should_failslab(size_t size, gfp_t gfpflags);
>> +extern bool should_failslab(size_t size, gfp_t gfpflags, unsigned long c_flags);
>>  #else
>> -static inline bool should_failslab(size_t size, gfp_t gfpflags)
>> +static inline bool should_failslab(size_t size, gfp_t gfpflags, unsigned long f)
>>  {
>>  	return false;
>>  }
>
> As I said in the first review, these need to be more descriptive: 'flags' 
> would be better.
done
>
>> diff --git a/include/linux/slab.h b/include/linux/slab.h
>> index 2da8372..9e03a81 100644
>> --- a/include/linux/slab.h
>> +++ b/include/linux/slab.h
>> @@ -70,7 +70,9 @@
>>  #else
>>  # define SLAB_NOTRACK		0x00000000UL
>>  #endif
>> -
>> +#ifdef CONFIG_FAILSLAB
>> +# define SLAB_FAILSLAB	0x02000000UL	/* Fault injection filter mark */
>> +#endif 
>
> Trailing whitespace on your #endif.
Sorry. fixed
>
>>  /* The following flags affect the page allocator grouping pages by mobility */
>>  #define SLAB_RECLAIM_ACCOUNT	0x00020000UL		/* Objects are reclaimable */
>>  #define SLAB_TEMPORARY		SLAB_RECLAIM_ACCOUNT	/* Objects are short-lived */
>> diff --git a/mm/failslab.c b/mm/failslab.c
>> index 9339de5..bb41f98 100644
>> --- a/mm/failslab.c
>> +++ b/mm/failslab.c
>> @@ -1,18 +1,22 @@
>>  #include <linux/fault-inject.h>
>>  #include <linux/gfp.h>
>> +#include <linux/slab.h>
>>  
>>  static struct {
>>  	struct fault_attr attr;
>>  	u32 ignore_gfp_wait;
>> +	int cache_filter;
>>  #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS
>>  	struct dentry *ignore_gfp_wait_file;
>> +	struct dentry *cache_filter_file;
>>  #endif
>>  } failslab = {
>>  	.attr = FAULT_ATTR_INITIALIZER,
>>  	.ignore_gfp_wait = 1,
>> +	.cache_filter = 0,
>>  };
>>  
>> -bool should_failslab(size_t size, gfp_t gfpflags)
>> +bool should_failslab(size_t size, gfp_t gfpflags, unsigned long cache_flags)
>>  {
>>  	if (gfpflags & __GFP_NOFAIL)
>>  		return false;
>> @@ -20,6 +24,9 @@ bool should_failslab(size_t size, gfp_t gfpflags)
>>          if (failslab.ignore_gfp_wait && (gfpflags & __GFP_WAIT))
>>  		return false;
>>  
>> +	if (failslab.cache_filter && !(cache_flags & SLAB_FAILSLAB))
>> +		return false;
>> +
>>  	return should_fail(&failslab.attr, size);
>>  }
>>  
>> @@ -30,7 +37,6 @@ static int __init setup_failslab(char *str)
>>  __setup("failslab=", setup_failslab);
>>  
>>  #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS
>> -
>>  static int __init failslab_debugfs_init(void)
>>  {
>>  	mode_t mode = S_IFREG | S_IRUSR | S_IWUSR;
>> @@ -46,8 +52,14 @@ static int __init failslab_debugfs_init(void)
>>  		debugfs_create_bool("ignore-gfp-wait", mode, dir,
>>  				      &failslab.ignore_gfp_wait);
>>  
>> -	if (!failslab.ignore_gfp_wait_file) {
>> +	failslab.cache_filter_file =
>> +		debugfs_create_bool("cache-filter", mode, dir,
>> +				      &failslab.cache_filter);
>> +
>> +	if (!failslab.ignore_gfp_wait_file ||
>> +	    !failslab.cache_filter_file) {
>>  		err = -ENOMEM;
>> +		debugfs_remove(failslab.cache_filter_file);
>>  		debugfs_remove(failslab.ignore_gfp_wait_file);
>>  		cleanup_fault_attr_dentries(&failslab.attr);
>>  	}
>> diff --git a/mm/slab.c b/mm/slab.c
>> index 7451bda..33496b7 100644
>> --- a/mm/slab.c
>> +++ b/mm/slab.c
>> @@ -3101,7 +3101,7 @@ static bool slab_should_failslab(struct kmem_cache *cachep, gfp_t flags)
>>  	if (cachep == &cache_cache)
>>  		return false;
>>  
>> -	return should_failslab(obj_size(cachep), flags);
>> +	return should_failslab(obj_size(cachep), flags, cachep->flags);
>>  }
>>  
>
> cachep->flags for CONFIG_SLAB is of type unsigned int and 
> should_failslab() takes an unsigned long.
Strange story, seems that flag definition in slab_def.h is differ
from slub_def.h and slob_def.h (unsigned long long)
Need to be fixed. Will send as separate patch.
>
>>  static inline void *____cache_alloc(struct kmem_cache *cachep, gfp_t flags)
>> diff --git a/mm/slub.c b/mm/slub.c
>> index 8d71aaf..64114fa 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -151,7 +151,8 @@
>>   * Set of flags that will prevent slab merging
>>   */
>>  #define SLUB_NEVER_MERGE (SLAB_RED_ZONE | SLAB_POISON | SLAB_STORE_USER | \
>> -		SLAB_TRACE | SLAB_DESTROY_BY_RCU | SLAB_NOLEAKTRACE)
>> +		SLAB_TRACE | SLAB_DESTROY_BY_RCU | SLAB_NOLEAKTRACE | \
>> +		SLAB_FAILSLAB)
>>  
>>  #define SLUB_MERGE_SAME (SLAB_DEBUG_FREE | SLAB_RECLAIM_ACCOUNT | \
>>  		SLAB_CACHE_DMA | SLAB_NOTRACK)
>
> As I mentioned in my first review of your patch, this breaks for 
> CONFIG_FAILSLAB=n.  Please make sure your patch compiles for the 
> defconfig.
fixed.