[PATCH 1/1] stackdepot: interface to check entries and size of stackdepot.

[PATCH 1/1] stackdepot: interface to check entries and size of stackdepot.

[Maninder Singh]

This patch provides interface to check all the stack enteries
saved in stackdepot so far as well as memory consumed by stackdepot.

1) Take current depot_index and offset to calculate end address for one
	iteration of (/sys/kernel/debug/depot_stack/depot_entries).

2) Fill end marker in every slab to point its end, and then use it while
	traversing all the slabs of stackdepot.

"debugfs code inspired from page_onwer's way of printing BT"

checked on ARM and x86_64.
$cat /sys/kernel/debug/depot_stack/depot_size
Memory consumed by Stackdepot:208 KB

$ cat /sys/kernel/debug/depot_stack/depot_entries
stack count 1 backtrace
 init_page_owner+0x1e/0x210
 start_kernel+0x310/0x3cd
 secondary_startup_64+0xa5/0xb0
 0xffffffffffffffff

Signed-off-by: Vaneet Narang <v.narang@samsung.com>
Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
---
 include/linux/stackdepot.h |   13 +++
 include/linux/stacktrace.h |    6 ++
 lib/stackdepot.c           |  183 ++++++++++++++++++++++++++++++++++++++++++++
 mm/page_owner.c            |    6 --
 4 files changed, 202 insertions(+), 6 deletions(-)

diff --git a/include/linux/stackdepot.h b/include/linux/stackdepot.h
index 7978b3e..dd95b11 100644
--- a/include/linux/stackdepot.h
+++ b/include/linux/stackdepot.h
@@ -23,6 +23,19 @@
 
 typedef u32 depot_stack_handle_t;
 
+/*
+ * structure to store markers which
+ * will be used while printing entries
+ * stored in stackdepot.
+ */
+struct depot_stack_data {
+	int print_offset;
+	int print_counter;
+	int print_index;
+	unsigned long end_marker;
+	void *end_address;
+};
+
 struct stack_trace;
 
 depot_stack_handle_t depot_save_stack(struct stack_trace *trace, gfp_t flags);
diff --git a/include/linux/stacktrace.h b/include/linux/stacktrace.h
index ba29a06..1cfd27d 100644
--- a/include/linux/stacktrace.h
+++ b/include/linux/stacktrace.h
@@ -4,6 +4,12 @@
 
 #include <linux/types.h>
 
+/*
+ * TODO: teach PAGE_OWNER_STACK_DEPTH (__dump_page_owner and save_stack)
+ * to use off stack temporal storage
+ */
+#define PAGE_OWNER_STACK_DEPTH (16)
+
 struct task_struct;
 struct pt_regs;
 
diff --git a/lib/stackdepot.c b/lib/stackdepot.c
index f87d138..3067fcb 100644
--- a/lib/stackdepot.c
+++ b/lib/stackdepot.c
@@ -39,6 +39,8 @@
 #include <linux/stackdepot.h>
 #include <linux/string.h>
 #include <linux/types.h>
+#include <linux/debugfs.h>
+#include <linux/uaccess.h>
 
 #define DEPOT_STACK_BITS (sizeof(depot_stack_handle_t) * 8)
 
@@ -111,6 +113,7 @@ static bool init_stack_slab(void **prealloc)
 	int required_size = offsetof(struct stack_record, entries) +
 		sizeof(unsigned long) * size;
 	struct stack_record *stack;
+	void *address;
 
 	required_size = ALIGN(required_size, 1 << STACK_ALLOC_ALIGN);
 
@@ -119,6 +122,17 @@ static bool init_stack_slab(void **prealloc)
 			WARN_ONCE(1, "Stack depot reached limit capacity");
 			return NULL;
 		}
+
+		/*
+		 * write POSION_END if any space left in
+		 * current slab to represent its end.
+		 * later used while printing all the stacks.
+		 */
+		if (depot_offset < STACK_ALLOC_SIZE) {
+			address = stack_slabs[depot_index] + depot_offset;
+			memset(address, POISON_END, sizeof(unsigned long));
+		}
+
 		depot_index++;
 		depot_offset = 0;
 		/*
@@ -285,3 +299,172 @@ depot_stack_handle_t depot_save_stack(struct stack_trace *trace,
 	return retval;
 }
 EXPORT_SYMBOL_GPL(depot_save_stack);
+
+#define DEPOT_SIZE 64
+
+static ssize_t read_depot_stack_size(struct file *file, char __user *buf, size_t count, loff_t *ppos)
+{
+	char kbuf[DEPOT_SIZE];
+	ssize_t ret = 0;
+	unsigned long size = depot_index * (1 << STACK_ALLOC_ORDER) * PAGE_SIZE;
+
+	ret = snprintf(kbuf, count, "Memory consumed by Stackdepot:%lu KB\n", size >> 10);
+	if (ret >= count)
+		return -ENOMEM;
+
+	return simple_read_from_buffer(buf, count, ppos, kbuf, ret);
+}
+
+static ssize_t print_depot_stack(char __user *buf, size_t count, struct stack_trace *trace, loff_t *ppos)
+{
+	char *kbuf;
+	int ret = 0;
+
+	kbuf = kvmalloc(count, GFP_KERNEL);
+	if (!kbuf)
+		return -ENOMEM;
+
+	ret = snprintf(kbuf, count, "stack count %d backtrace\n", (int)*ppos);
+	ret += snprint_stack_trace(kbuf + ret, count - ret, trace, 0);
+	ret += snprintf(kbuf + ret, count - ret, "\n");
+
+	if (ret >= count) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	if (copy_to_user(buf, kbuf, ret))
+		ret = -EFAULT;
+
+err:
+	kvfree(kbuf);
+	return ret;
+}
+
+/*
+ * read_depot_stack()
+ *
+ * function to print all the entries present
+ * in depot_stack database currently in system.
+ */
+static ssize_t read_depot_stack(struct file *file, char __user *buf, size_t count, loff_t *ppos)
+{
+	struct stack_record *stack;
+	void *address;
+	struct depot_stack_data *debugfs_data;
+
+	debugfs_data  = (struct depot_stack_data *)file->private_data;
+
+	if (!debugfs_data)
+		return -EINVAL;
+
+	while (debugfs_data->print_counter <= debugfs_data->print_index) {
+		unsigned long entries[PAGE_OWNER_STACK_DEPTH];
+		struct stack_trace trace = {
+			.nr_entries = 0,
+			.entries = entries,
+			.max_entries = PAGE_OWNER_STACK_DEPTH,
+			.skip = 0
+		};
+
+		address = stack_slabs[debugfs_data->print_counter] + debugfs_data->print_offset;
+		if (address == debugfs_data->end_address)
+			break;
+
+		if (*((unsigned long *)address) == debugfs_data->end_marker) {
+			debugfs_data->print_counter++;
+			debugfs_data->print_offset = 0;
+			continue;
+		}
+
+		stack = address;
+		trace.nr_entries = trace.max_entries = stack->size;
+		trace.entries = stack->entries;
+
+		debugfs_data->print_offset += offsetof(struct stack_record, entries) +
+				(stack->size * sizeof(unsigned long));
+		debugfs_data->print_offset = ALIGN(debugfs_data->print_offset, 1 << STACK_ALLOC_ALIGN);
+		if (debugfs_data->print_offset >= STACK_ALLOC_SIZE) {
+			debugfs_data->print_counter++;
+			debugfs_data->print_offset = 0;
+		}
+
+		*ppos = *ppos + 1; /* one stack found, print it */
+		return print_depot_stack(buf, count, &trace, ppos);
+	}
+
+	return 0;
+}
+
+int read_depot_open(struct inode *inode, struct file *file)
+{
+	struct depot_stack_data *debugfs_data;
+	unsigned long flags;
+
+	debugfs_data  = kzalloc(sizeof(struct depot_stack_data), GFP_KERNEL);
+	if (!debugfs_data)
+		return -ENOMEM;
+	/*
+	 * First time depot_stack/depot_entries is called.
+	 * (/sys/kernel/debug/depot_stack/depot_entries)
+	 * initialise print depot_index and stopping address.
+	 */
+	memset(&(debugfs_data->end_marker), POISON_END, sizeof(unsigned long));
+
+	spin_lock_irqsave(&depot_lock, flags);
+	debugfs_data->print_index = depot_index;
+	debugfs_data->end_address = stack_slabs[depot_index] + depot_offset;
+	spin_unlock_irqrestore(&depot_lock, flags);
+
+	file->private_data = debugfs_data;
+	return 0;
+}
+
+int read_depot_release(struct inode *inode, struct file *file)
+{
+	void *debugfs_data = file->private_data;
+
+	kfree(debugfs_data);
+	return 0;
+}
+
+static const struct file_operations proc_depot_stack_operations = {
+	.open       = read_depot_open,
+	.read		= read_depot_stack,
+	.release    = read_depot_release,
+};
+
+static const struct file_operations proc_depot_stack_size_operations = {
+	.read		= read_depot_stack_size,
+};
+
+static int __init depot_stack_init(void)
+{
+	struct dentry *dentry, *dentry_root;
+
+	dentry_root = debugfs_create_dir("depot_stack", NULL);
+
+	if (!dentry_root) {
+		pr_warn("debugfs 'depot_stack' dir creation failed\n");
+		return -ENOMEM;
+	}
+
+	dentry = debugfs_create_file("depot_entries", 0400, dentry_root,
+			NULL, &proc_depot_stack_operations);
+
+	if (IS_ERR(dentry))
+		goto err;
+
+	dentry = debugfs_create_file("depot_size", 0400, dentry_root,
+			NULL, &proc_depot_stack_size_operations);
+
+	if (IS_ERR(dentry))
+		goto err;
+
+	return 0;
+
+err:
+	debugfs_remove_recursive(dentry_root);
+	return PTR_ERR(dentry);
+}
+late_initcall(depot_stack_init)
diff --git a/mm/page_owner.c b/mm/page_owner.c
index 4f44b95..341b326 100644
--- a/mm/page_owner.c
+++ b/mm/page_owner.c
@@ -13,12 +13,6 @@
 
 #include "internal.h"
 
-/*
- * TODO: teach PAGE_OWNER_STACK_DEPTH (__dump_page_owner and save_stack)
- * to use off stack temporal storage
- */
-#define PAGE_OWNER_STACK_DEPTH (16)
-
 struct page_owner {
 	unsigned int order;
 	gfp_t gfp_mask;
-- 
1.7.1

Re: [PATCH 1/1] stackdepot: interface to check entries and size of stackdepot.

[Michal Hocko]

On Wed 22-11-17 16:17:41, Maninder Singh wrote:
> This patch provides interface to check all the stack enteries
> saved in stackdepot so far as well as memory consumed by stackdepot.
> 
> 1) Take current depot_index and offset to calculate end address for one
> 	iteration of (/sys/kernel/debug/depot_stack/depot_entries).
> 
> 2) Fill end marker in every slab to point its end, and then use it while
> 	traversing all the slabs of stackdepot.
> 
> "debugfs code inspired from page_onwer's way of printing BT"
> 
> checked on ARM and x86_64.
> $cat /sys/kernel/debug/depot_stack/depot_size
> Memory consumed by Stackdepot:208 KB
> 
> $ cat /sys/kernel/debug/depot_stack/depot_entries
> stack count 1 backtrace
>  init_page_owner+0x1e/0x210
>  start_kernel+0x310/0x3cd
>  secondary_startup_64+0xa5/0xb0
>  0xffffffffffffffff

Why do we need this? Who is goging to use this information and what for?
I haven't looked at the code but just the diffstat looks like this
should better have a _very_ good justification to be considered for
merging. To be honest with you I have hard time imagine how this can be
useful other than debugging stack depot...

> Signed-off-by: Vaneet Narang <v.narang@samsung.com>
> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
> ---
>  include/linux/stackdepot.h |   13 +++
>  include/linux/stacktrace.h |    6 ++
>  lib/stackdepot.c           |  183 ++++++++++++++++++++++++++++++++++++++++++++
>  mm/page_owner.c            |    6 --
>  4 files changed, 202 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/stackdepot.h b/include/linux/stackdepot.h
> index 7978b3e..dd95b11 100644
> --- a/include/linux/stackdepot.h
> +++ b/include/linux/stackdepot.h
> @@ -23,6 +23,19 @@
>  
>  typedef u32 depot_stack_handle_t;
>  
> +/*
> + * structure to store markers which
> + * will be used while printing entries
> + * stored in stackdepot.
> + */
> +struct depot_stack_data {
> +	int print_offset;
> +	int print_counter;
> +	int print_index;
> +	unsigned long end_marker;
> +	void *end_address;
> +};
> +
>  struct stack_trace;
>  
>  depot_stack_handle_t depot_save_stack(struct stack_trace *trace, gfp_t flags);
> diff --git a/include/linux/stacktrace.h b/include/linux/stacktrace.h
> index ba29a06..1cfd27d 100644
> --- a/include/linux/stacktrace.h
> +++ b/include/linux/stacktrace.h
> @@ -4,6 +4,12 @@
>  
>  #include <linux/types.h>
>  
> +/*
> + * TODO: teach PAGE_OWNER_STACK_DEPTH (__dump_page_owner and save_stack)
> + * to use off stack temporal storage
> + */
> +#define PAGE_OWNER_STACK_DEPTH (16)
> +
>  struct task_struct;
>  struct pt_regs;
>  
> diff --git a/lib/stackdepot.c b/lib/stackdepot.c
> index f87d138..3067fcb 100644
> --- a/lib/stackdepot.c
> +++ b/lib/stackdepot.c
> @@ -39,6 +39,8 @@
>  #include <linux/stackdepot.h>
>  #include <linux/string.h>
>  #include <linux/types.h>
> +#include <linux/debugfs.h>
> +#include <linux/uaccess.h>
>  
>  #define DEPOT_STACK_BITS (sizeof(depot_stack_handle_t) * 8)
>  
> @@ -111,6 +113,7 @@ static bool init_stack_slab(void **prealloc)
>  	int required_size = offsetof(struct stack_record, entries) +
>  		sizeof(unsigned long) * size;
>  	struct stack_record *stack;
> +	void *address;
>  
>  	required_size = ALIGN(required_size, 1 << STACK_ALLOC_ALIGN);
>  
> @@ -119,6 +122,17 @@ static bool init_stack_slab(void **prealloc)
>  			WARN_ONCE(1, "Stack depot reached limit capacity");
>  			return NULL;
>  		}
> +
> +		/*
> +		 * write POSION_END if any space left in
> +		 * current slab to represent its end.
> +		 * later used while printing all the stacks.
> +		 */
> +		if (depot_offset < STACK_ALLOC_SIZE) {
> +			address = stack_slabs[depot_index] + depot_offset;
> +			memset(address, POISON_END, sizeof(unsigned long));
> +		}
> +
>  		depot_index++;
>  		depot_offset = 0;
>  		/*
> @@ -285,3 +299,172 @@ depot_stack_handle_t depot_save_stack(struct stack_trace *trace,
>  	return retval;
>  }
>  EXPORT_SYMBOL_GPL(depot_save_stack);
> +
> +#define DEPOT_SIZE 64
> +
> +static ssize_t read_depot_stack_size(struct file *file, char __user *buf, size_t count, loff_t *ppos)
> +{
> +	char kbuf[DEPOT_SIZE];
> +	ssize_t ret = 0;
> +	unsigned long size = depot_index * (1 << STACK_ALLOC_ORDER) * PAGE_SIZE;
> +
> +	ret = snprintf(kbuf, count, "Memory consumed by Stackdepot:%lu KB\n", size >> 10);
> +	if (ret >= count)
> +		return -ENOMEM;
> +
> +	return simple_read_from_buffer(buf, count, ppos, kbuf, ret);
> +}
> +
> +static ssize_t print_depot_stack(char __user *buf, size_t count, struct stack_trace *trace, loff_t *ppos)
> +{
> +	char *kbuf;
> +	int ret = 0;
> +
> +	kbuf = kvmalloc(count, GFP_KERNEL);
> +	if (!kbuf)
> +		return -ENOMEM;
> +
> +	ret = snprintf(kbuf, count, "stack count %d backtrace\n", (int)*ppos);
> +	ret += snprint_stack_trace(kbuf + ret, count - ret, trace, 0);
> +	ret += snprintf(kbuf + ret, count - ret, "\n");
> +
> +	if (ret >= count) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +
> +	if (copy_to_user(buf, kbuf, ret))
> +		ret = -EFAULT;
> +
> +err:
> +	kvfree(kbuf);
> +	return ret;
> +}
> +
> +/*
> + * read_depot_stack()
> + *
> + * function to print all the entries present
> + * in depot_stack database currently in system.
> + */
> +static ssize_t read_depot_stack(struct file *file, char __user *buf, size_t count, loff_t *ppos)
> +{
> +	struct stack_record *stack;
> +	void *address;
> +	struct depot_stack_data *debugfs_data;
> +
> +	debugfs_data  = (struct depot_stack_data *)file->private_data;
> +
> +	if (!debugfs_data)
> +		return -EINVAL;
> +
> +	while (debugfs_data->print_counter <= debugfs_data->print_index) {
> +		unsigned long entries[PAGE_OWNER_STACK_DEPTH];
> +		struct stack_trace trace = {
> +			.nr_entries = 0,
> +			.entries = entries,
> +			.max_entries = PAGE_OWNER_STACK_DEPTH,
> +			.skip = 0
> +		};
> +
> +		address = stack_slabs[debugfs_data->print_counter] + debugfs_data->print_offset;
> +		if (address == debugfs_data->end_address)
> +			break;
> +
> +		if (*((unsigned long *)address) == debugfs_data->end_marker) {
> +			debugfs_data->print_counter++;
> +			debugfs_data->print_offset = 0;
> +			continue;
> +		}
> +
> +		stack = address;
> +		trace.nr_entries = trace.max_entries = stack->size;
> +		trace.entries = stack->entries;
> +
> +		debugfs_data->print_offset += offsetof(struct stack_record, entries) +
> +				(stack->size * sizeof(unsigned long));
> +		debugfs_data->print_offset = ALIGN(debugfs_data->print_offset, 1 << STACK_ALLOC_ALIGN);
> +		if (debugfs_data->print_offset >= STACK_ALLOC_SIZE) {
> +			debugfs_data->print_counter++;
> +			debugfs_data->print_offset = 0;
> +		}
> +
> +		*ppos = *ppos + 1; /* one stack found, print it */
> +		return print_depot_stack(buf, count, &trace, ppos);
> +	}
> +
> +	return 0;
> +}
> +
> +int read_depot_open(struct inode *inode, struct file *file)
> +{
> +	struct depot_stack_data *debugfs_data;
> +	unsigned long flags;
> +
> +	debugfs_data  = kzalloc(sizeof(struct depot_stack_data), GFP_KERNEL);
> +	if (!debugfs_data)
> +		return -ENOMEM;
> +	/*
> +	 * First time depot_stack/depot_entries is called.
> +	 * (/sys/kernel/debug/depot_stack/depot_entries)
> +	 * initialise print depot_index and stopping address.
> +	 */
> +	memset(&(debugfs_data->end_marker), POISON_END, sizeof(unsigned long));
> +
> +	spin_lock_irqsave(&depot_lock, flags);
> +	debugfs_data->print_index = depot_index;
> +	debugfs_data->end_address = stack_slabs[depot_index] + depot_offset;
> +	spin_unlock_irqrestore(&depot_lock, flags);
> +
> +	file->private_data = debugfs_data;
> +	return 0;
> +}
> +
> +int read_depot_release(struct inode *inode, struct file *file)
> +{
> +	void *debugfs_data = file->private_data;
> +
> +	kfree(debugfs_data);
> +	return 0;
> +}
> +
> +static const struct file_operations proc_depot_stack_operations = {
> +	.open       = read_depot_open,
> +	.read		= read_depot_stack,
> +	.release    = read_depot_release,
> +};
> +
> +static const struct file_operations proc_depot_stack_size_operations = {
> +	.read		= read_depot_stack_size,
> +};
> +
> +static int __init depot_stack_init(void)
> +{
> +	struct dentry *dentry, *dentry_root;
> +
> +	dentry_root = debugfs_create_dir("depot_stack", NULL);
> +
> +	if (!dentry_root) {
> +		pr_warn("debugfs 'depot_stack' dir creation failed\n");
> +		return -ENOMEM;
> +	}
> +
> +	dentry = debugfs_create_file("depot_entries", 0400, dentry_root,
> +			NULL, &proc_depot_stack_operations);
> +
> +	if (IS_ERR(dentry))
> +		goto err;
> +
> +	dentry = debugfs_create_file("depot_size", 0400, dentry_root,
> +			NULL, &proc_depot_stack_size_operations);
> +
> +	if (IS_ERR(dentry))
> +		goto err;
> +
> +	return 0;
> +
> +err:
> +	debugfs_remove_recursive(dentry_root);
> +	return PTR_ERR(dentry);
> +}
> +late_initcall(depot_stack_init)
> diff --git a/mm/page_owner.c b/mm/page_owner.c
> index 4f44b95..341b326 100644
> --- a/mm/page_owner.c
> +++ b/mm/page_owner.c
> @@ -13,12 +13,6 @@
>  
>  #include "internal.h"
>  
> -/*
> - * TODO: teach PAGE_OWNER_STACK_DEPTH (__dump_page_owner and save_stack)
> - * to use off stack temporal storage
> - */
> -#define PAGE_OWNER_STACK_DEPTH (16)
> -
>  struct page_owner {
>  	unsigned int order;
>  	gfp_t gfp_mask;
> -- 
> 1.7.1
> 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/1] stackdepot: interface to check entries and size of stackdepot.

[Maninder Singh]

Hi Michal,
  
> On Wed 22-11-17 16:17:41, Maninder Singh wrote:
> > This patch provides interface to check all the stack enteries
> > saved in stackdepot so far as well as memory consumed by stackdepot.
> > 
> > 1) Take current depot_index and offset to calculate end address for one
> >         iteration of (/sys/kernel/debug/depot_stack/depot_entries).
> > 
> > 2) Fill end marker in every slab to point its end, and then use it while
> >         traversing all the slabs of stackdepot.
> > 
> > "debugfs code inspired from page_onwer's way of printing BT"
> > 
> > checked on ARM and x86_64.
> > $cat /sys/kernel/debug/depot_stack/depot_size
> > Memory consumed by Stackdepot:208 KB
> > 
> > $ cat /sys/kernel/debug/depot_stack/depot_entries
> > stack count 1 backtrace
> >  init_page_owner+0x1e/0x210
> >  start_kernel+0x310/0x3cd
> >  secondary_startup_64+0xa5/0xb0
> >  0xffffffffffffffff
>  
> Why do we need this? Who is goging to use this information and what for?
> I haven't looked at the code but just the diffstat looks like this
> should better have a _very_ good justification to be considered for
> merging. To be honest with you I have hard time imagine how this can be
> useful other than debugging stack depot...

This interface can be used for multiple reasons as:

1) For debugging stackdepot for sure.
2) For checking all the unique allocation paths in system.
3) To check if any invalid stack is coming which is increasing 
stackdepot memory.
(https://lkml.org/lkml/2017/10/11/353)

Althoutgh this needs to be taken care in ARM as replied by maintainer, but with help
of this interface it was quite easy to check and we added workaround for saving memory.

4) At some point of time to check current memory consumed by stackdepot.
5) To check number of entries in stackdepot to decide stackdepot hash size for different systems. 
   For fewer entries hash table size can be reduced from 4MB. 

Thanks
Maninder Singh

Re: [PATCH 1/1] stackdepot: interface to check entries and size of stackdepot.

[Michal Hocko]

On Fri 24-11-17 09:41:08, Maninder Singh wrote:
> Hi Michal,
>   
> > On Wed 22-11-17 16:17:41, Maninder Singh wrote:
> > > This patch provides interface to check all the stack enteries
> > > saved in stackdepot so far as well as memory consumed by stackdepot.
> > > 
> > > 1) Take current depot_index and offset to calculate end address for one
> > >         iteration of (/sys/kernel/debug/depot_stack/depot_entries).
> > > 
> > > 2) Fill end marker in every slab to point its end, and then use it while
> > >         traversing all the slabs of stackdepot.
> > > 
> > > "debugfs code inspired from page_onwer's way of printing BT"
> > > 
> > > checked on ARM and x86_64.
> > > $cat /sys/kernel/debug/depot_stack/depot_size
> > > Memory consumed by Stackdepot:208 KB
> > > 
> > > $ cat /sys/kernel/debug/depot_stack/depot_entries
> > > stack count 1 backtrace
> > >  init_page_owner+0x1e/0x210
> > >  start_kernel+0x310/0x3cd
> > >  secondary_startup_64+0xa5/0xb0
> > >  0xffffffffffffffff
> >  
> > Why do we need this? Who is goging to use this information and what for?
> > I haven't looked at the code but just the diffstat looks like this
> > should better have a _very_ good justification to be considered for
> > merging. To be honest with you I have hard time imagine how this can be
> > useful other than debugging stack depot...
> 
> This interface can be used for multiple reasons as:
> 
> 1) For debugging stackdepot for sure.
> 2) For checking all the unique allocation paths in system.
> 3) To check if any invalid stack is coming which is increasing 
> stackdepot memory.
> (https://lkml.org/lkml/2017/10/11/353)

OK, so debugging a debugging facility... I do not think we want to
introduce a lot of code for something like that.

> Althoutgh this needs to be taken care in ARM as replied by maintainer, but with help
> of this interface it was quite easy to check and we added workaround for saving memory.
> 
> 4) At some point of time to check current memory consumed by stackdepot.
> 5) To check number of entries in stackdepot to decide stackdepot hash size for different systems. 
>    For fewer entries hash table size can be reduced from 4MB. 

What are you going to do with that information. It is not like you can
reduce the memory footprint or somehow optimize anything during the
runtime.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/1] stackdepot: interface to check entries and size of stackdepot.

[Dmitry Vyukov]

On Fri, Nov 24, 2017 at 10:54 AM, Michal Hocko <mhocko@kernel.org> wrote:
> On Fri 24-11-17 09:41:08, Maninder Singh wrote:
>> Hi Michal,
>>
>> > On Wed 22-11-17 16:17:41, Maninder Singh wrote:
>> > > This patch provides interface to check all the stack enteries
>> > > saved in stackdepot so far as well as memory consumed by stackdepot.
>> > >
>> > > 1) Take current depot_index and offset to calculate end address for one
>> > >         iteration of (/sys/kernel/debug/depot_stack/depot_entries).
>> > >
>> > > 2) Fill end marker in every slab to point its end, and then use it while
>> > >         traversing all the slabs of stackdepot.
>> > >
>> > > "debugfs code inspired from page_onwer's way of printing BT"
>> > >
>> > > checked on ARM and x86_64.
>> > > $cat /sys/kernel/debug/depot_stack/depot_size
>> > > Memory consumed by Stackdepot:208 KB
>> > >
>> > > $ cat /sys/kernel/debug/depot_stack/depot_entries
>> > > stack count 1 backtrace
>> > >  init_page_owner+0x1e/0x210
>> > >  start_kernel+0x310/0x3cd
>> > >  secondary_startup_64+0xa5/0xb0
>> > >  0xffffffffffffffff
>> >
>> > Why do we need this? Who is goging to use this information and what for?
>> > I haven't looked at the code but just the diffstat looks like this
>> > should better have a _very_ good justification to be considered for
>> > merging. To be honest with you I have hard time imagine how this can be
>> > useful other than debugging stack depot...
>>
>> This interface can be used for multiple reasons as:
>>
>> 1) For debugging stackdepot for sure.
>> 2) For checking all the unique allocation paths in system.
>> 3) To check if any invalid stack is coming which is increasing
>> stackdepot memory.
>> (https://lkml.org/lkml/2017/10/11/353)
>
> OK, so debugging a debugging facility... I do not think we want to
> introduce a lot of code for something like that.
>
>> Althoutgh this needs to be taken care in ARM as replied by maintainer, but with help
>> of this interface it was quite easy to check and we added workaround for saving memory.
>>
>> 4) At some point of time to check current memory consumed by stackdepot.
>> 5) To check number of entries in stackdepot to decide stackdepot hash size for different systems.
>>    For fewer entries hash table size can be reduced from 4MB.
>
> What are you going to do with that information. It is not like you can
> reduce the memory footprint or somehow optimize anything during the
> runtime.
>
> --
> Michal Hocko
> SUSE Labs

+kasan-dev mailing list

RE: Re: [PATCH 1/1] stackdepot: interface to check entries and size of stackdepot.

[Vaneet Narang]

Hi Michal,


>> 5) To check number of entries in stackdepot to decide stackdepot hash size for different systems.
>>    For fewer entries hash table size can be reduced from 4MB.
>
> What are you going to do with that information. It is not like you can
> reduce the memory footprint or somehow optimize anything during the
> runtime.

On low memory system where page owner entries are in range of 3k ~ 4k, its
a waste to keep hash table size of 4MB. It can be modified to some 128KB to
save memory footprint of stackdepot. So stackdepot entry count is important.

> OK, so debugging a debugging facility... I do not think we want to
> introduce a lot of code for something like that.

We enabled stackdepot on our system and realised, in long run stack depot consumes
more runtime memory then it actually needs. we used shared patch to debug this issue. 
stack stores following two unique entries. Page allocation done in interrupt 
context will generate a unique stack trace. Consider following two entries.

Entry 1:
 __alloc_pages_nodemask+0xec/0x200
 page_frag_alloc+0x84/0x140
 __napi_alloc_skb+0x83/0xe0
 rtl8169_poll+0x1e5/0x670
 net_rx_action+0x122/0x380          
 __do_softirq+0xce/0x298            
 irq_exit+0xa3/0xb0
 -------------------
 do_IRQ+0x72/0xc0
 ret_from_intr+0x0/0x14
 rw_copy_check_uvector+0x8a/0x100
 import_iovec+0x27/0xc0
 copy_msghdr_from_user+0xc0/0x120
 ___sys_recvmsg+0x76/0x210
 __sys_recvmsg+0x39/0x70
 entry_SYSCALL_64_fastpath+0x13/

 Entry 2:
  __alloc_pages_nodemask+0xec/0x200
 page_frag_alloc+0x84/0x140
 __napi_alloc_skb+0x83/0xe0
 rtl8169_poll+0x1e5/0x670
 net_rx_action+0x122/0x380
 __do_softirq+0xce/0x298
 irq_exit+0xa3/0xb0    
 -------------------
 smp_apic_timer_interrupt+0x5b/0x110
 apic_timer_interrupt+0x89/0x90
 cpuidle_enter_state+0x95/0x2c0
 do_idle+0x163/0x1a0
 cpu_startup_entry+0x14/0x20
 secondary_startup_64+0xa5/0xb0

 Actual Allocation Path is
 __alloc_pages_nodemask+0xec/0x200
 page_frag_alloc+0x84/0x140
 __napi_alloc_skb+0x83/0xe0
 rtl8169_poll+0x1e5/0x670
 net_rx_action+0x122/0x380          
 __do_softirq+0xce/0x298            
 irq_exit+0xa3/0xb0

We have been getting similar kind of such entries and eventually
stackdepot reaches Max Cap. So we found this interface useful in debugging
stackdepot issue so shared in community.

Regards,
Vaneet Narang

Re: Re: [PATCH 1/1] stackdepot: interface to check entries and size of stackdepot.

[Michal Hocko]

On Fri 24-11-17 11:57:07, Vaneet Narang wrote:
[...]
> > OK, so debugging a debugging facility... I do not think we want to
> > introduce a lot of code for something like that.
> 
> We enabled stackdepot on our system and realised, in long run stack depot consumes
> more runtime memory then it actually needs. we used shared patch to debug this issue. 
> stack stores following two unique entries. Page allocation done in interrupt 
> context will generate a unique stack trace. Consider following two entries.
[...]
> We have been getting similar kind of such entries and eventually
> stackdepot reaches Max Cap. So we found this interface useful in debugging
> stackdepot issue so shared in community.

Then use it for internal debugging and provide a code which would scale
better on smaller systems. We do not need this in the kernel IMHO. We do
not merge all the debugging patches we use for internal development.

-- 
Michal Hocko
SUSE Labs

RE: Re: Re: [PATCH 1/1] stackdepot: interface to check entries and size of stackdepot.

[Vaneet Narang]

Hi Michal,

>> We have been getting similar kind of such entries and eventually
>> stackdepot reaches Max Cap. So we found this interface useful in debugging
>> stackdepot issue so shared in community.
 
>Then use it for internal debugging and provide a code which would scale
>better on smaller systems. We do not need this in the kernel IMHO. We do
>not merge all the debugging patches we use for internal development.
` 
Not just debugging but this information can also be used to profile and tune stack depot. 
Getting count of stack entries would help in deciding hash table size and 
page order used by stackdepot. 

For less entries, bigger hash table and higher page order slabs might not be required as 
maintained by stackdepot. As i already mentioned smaller size hashtable can be choosen and 
similarly lower order  pages can be used for slabs.

If you think its useful, we can share scalable patch to configure below two values based on 
number of stack entries dynamically.

#define STACK_ALLOC_ORDER 2 
#define STACK_HASH_SIZE (1L << STACK_HASH_ORDER)


Regards,
Vaneet Narang

Re: Re: Re: [PATCH 1/1] stackdepot: interface to check entries and size of stackdepot.

[Michal Hocko]

On Fri 24-11-17 13:30:25, Vaneet Narang wrote:
> Hi Michal,
> 
> >> We have been getting similar kind of such entries and eventually
> >> stackdepot reaches Max Cap. So we found this interface useful in debugging
> >> stackdepot issue so shared in community.
>  
> >Then use it for internal debugging and provide a code which would scale
> >better on smaller systems. We do not need this in the kernel IMHO. We do
> >not merge all the debugging patches we use for internal development.
> ` 
> Not just debugging but this information can also be used to profile and tune stack depot. 
> Getting count of stack entries would help in deciding hash table size and 
> page order used by stackdepot. 

How can you control that in runtime?

> For less entries, bigger hash table and higher page order slabs might not be required as 
> maintained by stackdepot. As i already mentioned smaller size hashtable can be choosen and 
> similarly lower order  pages can be used for slabs.
> 
> If you think its useful, we can share scalable patch to configure below two values based on 
> number of stack entries dynamically.

Exporting this data without having a way to control it is just not very
useful for upstream kernel. If you can come up with some dynamic tuning
then that might be interesting. But your patch doesn't seem useful
outside of the development enviroment AFAICS.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/1] stackdepot: interface to check entries and size of stackdepot.

[Vinayak Menon]

On 11/24/2017 7:00 PM, Vaneet Narang wrote:
> Hi Michal,
>
>>>  We have been getting similar kind of such entries and eventually
>>>  stackdepot reaches Max Cap. So we found this interface useful in debugging
>>>  stackdepot issue so shared in community.
>  
>> Then use it for internal debugging and provide a code which would scale
>> better on smaller systems. We do not need this in the kernel IMHO. We do
>> not merge all the debugging patches we use for internal development.
> ` 
> Not just debugging but this information can also be used to profile and tune stack depot. 
> Getting count of stack entries would help in deciding hash table size and 
> page order used by stackdepot. 
>
> For less entries, bigger hash table and higher page order slabs might not be required as 
> maintained by stackdepot. As i already mentioned smaller size hashtable can be choosen and 
> similarly lower order  pages can be used for slabs.
>
> If you think its useful, we can share scalable patch to configure below two values based on 
> number of stack entries dynamically.
>
> #define STACK_ALLOC_ORDER 2 
> #define STACK_HASH_SIZE (1L << STACK_HASH_ORDER)
It will be good if this hash table size can be tuned somehow. When CONFIG_PAGE_OWNER is enabled, we expect it to
consume significant amount of memory only when "page_owner" kernel param is set. But since PAGE_OWNER selects
STACKDEPOT, it consumes around 8MB (stack_table) on 64 bit without even a single stack being stored. This is a problem
on low RAM targets where we want to keep CONFIG_PAGE_OWNER enabled by default and for debugging enable the
feature via the kernel param.
I am not sure how feasible it is to configure it dynamically, but I think a hash_size early param and then a memblock alloc
of stack table at boot would work and help low ram devices.

Thanks,
Vinayak