Re: kfree(NULL)

[Steven Rostedt <rostedt@goodmis.org>]

On Fri, 2006-04-21 at 09:03 -0700, Daniel Walker wrote:
> 
> After reviewing some of the callers of kfree(NULL) they appear to be
> errors by the caller .. Where there's some false assumptions going on
> during looping or repeated calls to the same function. 
> 
> I agree with Andrew , I think the calls should be investigated while
> retaining the unlikley() predictor . 
> 

Below is a quick patch to do statistics on kfree.  It records up to 1000
locations of those that use kfree(NULL). Yes I know that the
adding/searching the list is O(n), but it's only for statistics, and it
only happens on the NULL case.

Then it allows for showing what it found through the sysrq-l 

Anyway, after booting with this patch I got the following output:

SysRq : Show stats on kfree
Total number of NULL frees:      16129
Total number of non NULL frees:  18119
Callers of NULL frees:
[       27]  c0154bcd - do_tune_cpucache+0x13d/0x230
[      631]  c025b9dd - class_device_add+0xcd/0x300
[       30]  c019523c - sysfs_d_iput+0x3c/0x8e
[       44]  c0193750 - sysfs_hash_and_remove+0xd0/0x110
[        1]  c01f4787 - kobject_cleanup+0x37/0x90
[        1]  c025bf73 - class_dev_release+0x23/0x90
[       14]  c021b615 - tty_write+0x105/0x220
[       20]  c025b5ff - class_device_del+0x16f/0x190
[        6]  c021cd34 - release_mem+0x174/0x2a0
[       36]  c011e804 - do_sysctl+0x94/0x250
[     6491]  c01aafc4 - start_this_handle+0x234/0x4b0
[     8404]  c01aba66 - do_get_write_access+0x2e6/0x5a0
[      263]  c01abdf0 - journal_get_undo_access+0xd0/0x120
[       28]  c01a3c9f - ext3_clear_inode+0x2f/0x40
[        3]  c0194a0c - sysfs_dir_close+0x6c/0x90
[       59]  c0304e1d - inet_sock_destruct+0xad/0x1f0
[        1]  c030a698 - ip_rt_ioctl+0xe8/0x130
[       64]  c02e2669 - ip_push_pending_frames+0x2d9/0x400
[        6]  c02d69b0 - netlink_release+0x1c0/0x300

The numbers in the []'s is the number of times they call kfree with
NULL.

This was right after a boot, but I can play on the system and see what
else is doing a heavy kfree(NULL).  Maybe Jan Engelhardt's idea of doing
the inline unless CONFIG_CC_OPTIMIZE_FOR_SIZE is set is the way to go.

Anyway, if you want to play, or make this a better patch, here it is.

-- Steve

Index: linux-2.6.17-rc2/drivers/char/sysrq.c
===================================================================
--- linux-2.6.17-rc2.orig/drivers/char/sysrq.c	2006-04-21 13:13:28.000000000 -0400
+++ linux-2.6.17-rc2/drivers/char/sysrq.c	2006-04-21 13:39:43.000000000 -0400
@@ -271,6 +271,19 @@ static struct sysrq_key_op sysrq_unrt_op
 	.enable_mask	= SYSRQ_ENABLE_RTNICE,
 };
 
+extern void kfree_show_stats(void);
+static void sysrq_handle_kfree_stat(int key, struct pt_regs *pt_regs,
+                                    struct tty_struct *tty)
+{
+        kfree_show_stats();
+}
+static struct sysrq_key_op sysrq_kfree_op = {
+	.handler	= sysrq_handle_kfree_stat,
+	.help_msg	= "KfreeStats",
+	.action_msg	= "Show stats on kfree",
+	.enable_mask	= SYSRQ_ENABLE_RTNICE,
+};
+
 /* Key Operations table and lock */
 static DEFINE_SPINLOCK(sysrq_key_table_lock);
 
@@ -301,7 +314,7 @@ static struct sysrq_key_op *sysrq_key_ta
 	&sysrq_kill_op,			/* i */
 	NULL,				/* j */
 	&sysrq_SAK_op,			/* k */
-	NULL,				/* l */
+	&sysrq_kfree_op,		/* l */
 	&sysrq_showmem_op,		/* m */
 	&sysrq_unrt_op,			/* n */
 	/* This will often be registered as 'Off' at init time */
Index: linux-2.6.17-rc2/mm/slab.c
===================================================================
--- linux-2.6.17-rc2.orig/mm/slab.c	2006-04-21 10:11:41.000000000 -0400
+++ linux-2.6.17-rc2/mm/slab.c	2006-04-21 13:47:51.000000000 -0400
@@ -3366,6 +3366,69 @@ void kmem_cache_free(struct kmem_cache *
 }
 EXPORT_SYMBOL(kmem_cache_free);
 
+#define KFREE_STAT_SZ 1000
+struct kfree_struct {
+	void *addr;
+	unsigned long counter;
+} kfree_stats[KFREE_STAT_SZ];
+int nr_kfree_stats;
+unsigned long kfree_nulls;
+atomic_t kfree_non_nulls = ATOMIC_INIT(0);
+spinlock_t kfree_stat_lock = SPIN_LOCK_UNLOCKED;
+
+void kfree_show_stats(void)
+{
+	unsigned long flags;
+	unsigned long i;
+
+	spin_lock_irqsave(&kfree_stat_lock, flags);
+	printk("Total number of NULL frees:      %ld\n",
+	       kfree_nulls);
+	printk("Total number of non NULL frees:  %d\n",
+	       atomic_read(&kfree_non_nulls));
+	printk("Callers of NULL frees:\n");
+	for (i=0; i < nr_kfree_stats; i++) {
+		void *addr = kfree_stats[i].addr;
+		unsigned long cnt = kfree_stats[i].counter;
+		printk("[%9ld]  %p - ", cnt, addr);
+		print_symbol("%s\n", (unsigned long)addr);
+	}
+	spin_unlock_irqrestore(&kfree_stat_lock, flags);
+}
+
+/*
+ * add_kfree_null - this adds the calling address to
+ *  the list of callers that called kfree with NULL.
+ *  Yes this is very inefficient, but it _should_ be the
+ *  slow path (called when NULL) and is only used for stats.
+ */
+static inline void add_kfree_null(void *addr)
+{
+	unsigned long flags;
+	unsigned long i;
+
+	spin_lock_irqsave(&kfree_stat_lock, flags);
+
+	kfree_nulls++;  /* protected by kfree_stat_lock */
+
+	if (nr_kfree_stats == KFREE_STAT_SZ)
+		goto out;
+
+	/* ARGH! linear O(n) search! */
+	for (i=0; i < nr_kfree_stats; i++)
+		if (kfree_stats[i].addr == addr) {
+			kfree_stats[i].counter++;
+			goto out;
+		}
+
+	nr_kfree_stats++;
+	kfree_stats[i].addr = addr;
+	kfree_stats[i].counter = 1;
+
+out:
+	spin_unlock_irqrestore(&kfree_stat_lock, flags);
+}
+
 /**
  * kfree - free previously allocated memory
  * @objp: pointer returned by kmalloc.
@@ -3380,9 +3443,12 @@ void kfree(const void *objp)
 	struct kmem_cache *c;
 	unsigned long flags;
 
-	if (unlikely(!objp))
+	if (unlikely(!objp)) {
+		add_kfree_null(__builtin_return_address(0));
 		return;
+	}
 	local_irq_save(flags);
+	atomic_inc(&kfree_non_nulls);
 	kfree_debugcheck(objp);
 	c = virt_to_cache(objp);
 	mutex_debug_check_no_locks_freed(objp, obj_size(c));


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/