kcore patches (was Re: 2.6.32 -mm merge plans)

kcore patches (was Re: 2.6.32 -mm merge plans)

[Américo Wang]

On Wed, Sep 16, 2009 at 7:15 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
>#kcore-fix-proc-kcores-statst_size.patch: is it right?
>kcore-fix-proc-kcores-statst_size.patch

Hmm, I think KAMEZAWA Hiroyuki's patchset is a much better fix for this.
Hiroyuki?

Thanks!

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: kcore patches (was Re: 2.6.32 -mm merge plans)

[KAMEZAWA Hiroyuki]

Américo_Wang さんは書きました：
> On Wed, Sep 16, 2009 at 7:15 AM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
>>#kcore-fix-proc-kcores-statst_size.patch: is it right?
>>kcore-fix-proc-kcores-statst_size.patch
>
> Hmm, I think KAMEZAWA Hiroyuki's patchset is a much better fix for this.
> Hiroyuki?
>
Hmm ? My set is not agaisnt "file size" of /proc/kcore.

One problem of this patch is..this makes size of /proc/kcore as 0 bytes.
Then, objdump cannot read this. (it checks file size.)
readelf can read this. (it ignores file size.)

I wonder what you mention is.... because we know precise kclist_xxx
after my series, we can calculate kcore's size in precise by
get_kcore_size().

It seems /proc's inode->i_size is "static" and we cannot
provides return value of get_kcore_size() directly. It may need
some work and should depends on my kclist_xxx patch sets which are not
in merge candidates. If you can wait, I'll do some work for fixing this
problem. (but will not be able to merge directly against upstream.)

But for now, we have to use some fixed value....and using above
patch for 2.6.31 is not very bad.

Thanks,
-Kame

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 0/3][mmotm] showing size of kcore (Was Re: kcore patches (was Re: 2.6.32 -mm merge plans)

[KAMEZAWA Hiroyuki]

On Wed, 16 Sep 2009 20:17:52 +0900 (JST)
"KAMEZAWA Hiroyuki" <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> But for now, we have to use some fixed value....and using above
> patch for 2.6.31 is not very bad.
> 

This set is based on mmotm's kcore patch stack.
So, just for discussing. 

  [1/3] ... clean up (tiny bug fix)
  [2/3] ... show size of /proc/kcore
  [3/3] ... update kcore size at memory hotplug.

After patches, /proc/kcore's size is as following.
==
[kamezawa@bluextal mmotm-2.6.31-Sep14]$ ls -l /proc/kcore
-r-------- 1 root root 140737486266368 2009-09-17 11:53 /proc/kcore
[kamezawa@bluextal mmotm-2.6.31-Sep14]$
==
I'm not sure how this value is useful...but..hmm..better than zero ?
(The reason of very big value is because vmalloc area is too large.)


Thanks,
-Kame

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 1/3][mmotm] kcore: more fixes for init

[KAMEZAWA Hiroyuki]

From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

proc_kcore_init() doesn't check NULL case.
fix it and remove unnecessary comments.

Cc: WANG Cong <xiyou.wangcong@gmail.com>
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
 fs/proc/kcore.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Index: mmotm-2.6.31-Sep14/fs/proc/kcore.c
===================================================================
--- mmotm-2.6.31-Sep14.orig/fs/proc/kcore.c
+++ mmotm-2.6.31-Sep14/fs/proc/kcore.c
@@ -606,6 +606,10 @@ static int __init proc_kcore_init(void)
 {
 	proc_root_kcore = proc_create("kcore", S_IRUSR, NULL,
 				      &proc_kcore_operations);
+	if (!proc_root_kcore) {
+		printk(KERN_ERR "couldn't create /proc/kcore\n");
+		return 0; /* Always returns 0. */
+	}
 	/* Store text area if it's special */
 	proc_kcore_text_init();
 	/* Store vmalloc area */
@@ -615,7 +619,6 @@ static int __init proc_kcore_init(void)
 	/* Store direct-map area from physical memory map */
 	kcore_update_ram();
 	hotplug_memory_notifier(kcore_callback, 0);
-	/* Other special area, area-for-module etc is arch specific. */
 
 	return 0;
 }

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 2/3][mmotm] showing size of kcore

[KAMEZAWA Hiroyuki]

From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Now, size of /proc/kcore which can be read by 'ls -l' is 0.
But it's not correct value.

This is a patch for showing size of /proc/kcore as following.

On x86-64, ls -l shows
 ... root root 140737486266368 2009-09-17 10:29 /proc/kcore
Then, 7FFFFFFE02000. This comes from vmalloc area's size.
(*) This shows "core" size, not  memory size.

This patch shows the size by updating "size" field in struct proc_dir_entry.
Later, lookup routine will create inode and fill inode->i_size based
on this value. Then, this has a problem.

 - Once inode is cached, inode->i_size will never be updated.

Then, this patch is not memory-hotplug-aware.

To update inode->i_size, we have to know dentry or inode.
But there is no way to lookup them by inside kernel. Hmmm....
Next patch will try it.

Cc: WANG Cong <xiyou.wangcong@gmail.com>
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
 fs/proc/kcore.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Index: mmotm-2.6.31-Sep14/fs/proc/kcore.c
===================================================================
--- mmotm-2.6.31-Sep14.orig/fs/proc/kcore.c
+++ mmotm-2.6.31-Sep14/fs/proc/kcore.c
@@ -107,6 +107,8 @@ static void free_kclist_ents(struct list
  */
 static void __kcore_update_ram(struct list_head *list)
 {
+	int nphdr;
+	size_t size;
 	struct kcore_list *tmp, *pos;
 	LIST_HEAD(garbage);
 
@@ -124,6 +126,7 @@ static void __kcore_update_ram(struct li
 	write_unlock(&kclist_lock);
 
 	free_kclist_ents(&garbage);
+	proc_root_kcore->size = get_kcore_size(&nphdr, &size);
 }
 
 
@@ -429,7 +432,8 @@ read_kcore(struct file *file, char __use
 	unsigned long start;
 
 	read_lock(&kclist_lock);
-	proc_root_kcore->size = size = get_kcore_size(&nphdr, &elf_buflen);
+	size = get_kcore_size(&nphdr, &elf_buflen);
+
 	if (buflen == 0 || *fpos >= size) {
 		read_unlock(&kclist_lock);
 		return 0;

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 3/3][mmotm] updateing size of kcore

[KAMEZAWA Hiroyuki]

After memory hotplug (or other events in future), kcore size
can be modified.

To update inode->i_size, we have to know inode/dentry but we
can't get it from inside /proc directly.
But considerinyg memory hotplug, kcore image is updated only when
it's opened. Then, updating inode->i_size at open() is enough.

Cc: WANG Cong <xiyou.wangcong@gmail.com>
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
 fs/proc/kcore.c |    5 +++++
 1 file changed, 5 insertions(+)

Index: mmotm-2.6.31-Sep14/fs/proc/kcore.c
===================================================================
--- mmotm-2.6.31-Sep14.orig/fs/proc/kcore.c
+++ mmotm-2.6.31-Sep14/fs/proc/kcore.c
@@ -546,6 +546,11 @@ static int open_kcore(struct inode *inod
 		return -EPERM;
 	if (kcore_need_update)
 		kcore_update_ram();
+	if (i_size_read(inode) != proc_root_kcore->size) {
+		mutex_lock(&inode->i_mutex);
+		i_size_write(inode, proc_root_kcore->size);
+		mutex_unlock(&inode->i_mutex);
+	}
 	return 0;
 }
 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: kcore patches (was Re: 2.6.32 -mm merge plans)

[Américo Wang]

2009/9/16 KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>:
> Am�+1rico_Wang さんは書きました：
>> On Wed, Sep 16, 2009 at 7:15 AM, Andrew Morton
>> <akpm@linux-foundation.org> wrote:
>>>#kcore-fix-proc-kcores-statst_size.patch: is it right?
>>>kcore-fix-proc-kcores-statst_size.patch
>>
>> Hmm, I think KAMEZAWA Hiroyuki's patchset is a much better fix for this.
>> Hiroyuki?
>>
> Hmm ? My set is not agaisnt "file size" of /proc/kcore.
>
> One problem of this patch is..this makes size of /proc/kcore as 0 bytes.
> Then, objdump cannot read this. (it checks file size.)
> readelf can read this. (it ignores file size.)

Hmm, ok.


>
> I wonder what you mention is.... because we know precise kclist_xxx
> after my series, we can calculate kcore's size in precise by
> get_kcore_size().


Yeah, that is why I think your patchset for kcore can replace this.

>
> It seems /proc's inode->i_size is "static" and we cannot
> provides return value of get_kcore_size() directly. It may need
> some work and should depends on my kclist_xxx patch sets which are not
> in merge candidates. If you can wait, I'll do some work for fixing this
> problem. (but will not be able to merge directly against upstream.)
>
> But for now, we have to use some fixed value....and using above
> patch for 2.6.31 is not very bad.


Just saw your new patchset for this, I will review them.

Thanks.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/3][mmotm] kcore: more fixes for init

[Américo Wang]

On Thu, Sep 17, 2009 at 10:42 AM, KAMEZAWA Hiroyuki
<kamezawa.hiroyu@jp.fujitsu.com> wrote:
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
>
> proc_kcore_init() doesn't check NULL case.
> fix it and remove unnecessary comments.
>
> Cc: WANG Cong <xiyou.wangcong@gmail.com>
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Acked-by: WANG Cong <xiyou.wangcong@gmail.com>

Thanks.

> ---
>  fs/proc/kcore.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> Index: mmotm-2.6.31-Sep14/fs/proc/kcore.c
> ===================================================================
> --- mmotm-2.6.31-Sep14.orig/fs/proc/kcore.c
> +++ mmotm-2.6.31-Sep14/fs/proc/kcore.c
> @@ -606,6 +606,10 @@ static int __init proc_kcore_init(void)
>  {
>        proc_root_kcore = proc_create("kcore", S_IRUSR, NULL,
>                                      &proc_kcore_operations);
> +       if (!proc_root_kcore) {
> +               printk(KERN_ERR "couldn't create /proc/kcore\n");
> +               return 0; /* Always returns 0. */
> +       }
>        /* Store text area if it's special */
>        proc_kcore_text_init();
>        /* Store vmalloc area */
> @@ -615,7 +619,6 @@ static int __init proc_kcore_init(void)
>        /* Store direct-map area from physical memory map */
>        kcore_update_ram();
>        hotplug_memory_notifier(kcore_callback, 0);
> -       /* Other special area, area-for-module etc is arch specific. */
>
>        return 0;
>  }
>
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/3][mmotm] showing size of kcore

[Américo Wang]

On Thu, Sep 17, 2009 at 10:44 AM, KAMEZAWA Hiroyuki
<kamezawa.hiroyu@jp.fujitsu.com> wrote:
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
>
> Now, size of /proc/kcore which can be read by 'ls -l' is 0.
> But it's not correct value.
>
> This is a patch for showing size of /proc/kcore as following.
>
> On x86-64, ls -l shows
>  ... root root 140737486266368 2009-09-17 10:29 /proc/kcore
> Then, 7FFFFFFE02000. This comes from vmalloc area's size.
> (*) This shows "core" size, not  memory size.
>
> This patch shows the size by updating "size" field in struct proc_dir_entry.
> Later, lookup routine will create inode and fill inode->i_size based
> on this value. Then, this has a problem.
>
>  - Once inode is cached, inode->i_size will never be updated.
>
> Then, this patch is not memory-hotplug-aware.
>
> To update inode->i_size, we have to know dentry or inode.
> But there is no way to lookup them by inside kernel. Hmmm....
> Next patch will try it.
>
> Cc: WANG Cong <xiyou.wangcong@gmail.com>
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> ---
>  fs/proc/kcore.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> Index: mmotm-2.6.31-Sep14/fs/proc/kcore.c
> ===================================================================
> --- mmotm-2.6.31-Sep14.orig/fs/proc/kcore.c
> +++ mmotm-2.6.31-Sep14/fs/proc/kcore.c
> @@ -107,6 +107,8 @@ static void free_kclist_ents(struct list
>  */
>  static void __kcore_update_ram(struct list_head *list)
>  {
> +       int nphdr;
> +       size_t size;
>        struct kcore_list *tmp, *pos;
>        LIST_HEAD(garbage);
>
> @@ -124,6 +126,7 @@ static void __kcore_update_ram(struct li
>        write_unlock(&kclist_lock);
>
>        free_kclist_ents(&garbage);
> +       proc_root_kcore->size = get_kcore_size(&nphdr, &size);


This makes me to think if we will have some race condition here?
Two processes can open kcore at the same time...

>  }
>
>
> @@ -429,7 +432,8 @@ read_kcore(struct file *file, char __use
>        unsigned long start;
>
>        read_lock(&kclist_lock);
> -       proc_root_kcore->size = size = get_kcore_size(&nphdr, &elf_buflen);
> +       size = get_kcore_size(&nphdr, &elf_buflen);
> +
>        if (buflen == 0 || *fpos >= size) {
>                read_unlock(&kclist_lock);
>                return 0;
>
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 2/3][mmotm] showing size of kcore v2

[KAMEZAWA Hiroyuki]

On Thu, 17 Sep 2009 14:02:39 +0800
AmA(C)rico Wang <xiyou.wangcong@gmail.com> wrote:
> > @@ -124,6 +126,7 @@ static void __kcore_update_ram(struct li
> > A  A  A  A write_unlock(&kclist_lock);
> >
> > A  A  A  A free_kclist_ents(&garbage);
> > + A  A  A  proc_root_kcore->size = get_kcore_size(&nphdr, &size);
> 
> 
> This makes me to think if we will have some race condition here?
> Two processes can open kcore at the same time...
> 
Finally,
==
static void __kcore_update_ram(struct list_head *list)
{
 write_lock(&kclist_lock);
        if (kcore_need_update) {
                list_for_each_entry_safe(pos, tmp, &kclist_head, list) {
                        if (pos->type == KCORE_RAM
                                || pos->type == KCORE_VMEMMAP)
                                list_move(&pos->list, &garbage);
                }
                list_splice_tail(list, &kclist_head);
        } else
                list_splice(list, &garbage);
        kcore_need_update = 0;
        write_unlock(&kclist_lock);
}

kclist itself is double checked under write_lock.
And, once updated, get_kcore_size()'s return vaule is static.
So, I think there are no race. But..Hmm...is this clearer ?

==
From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Now, size of /proc/kcore which can be read by 'ls -l' is 0.
But it's not correct value.

This is a patch for showing size of /proc/kcore as following.

On x86-64, ls -l shows
 ... root root 140737486266368 2009-09-17 10:29 /proc/kcore
Then, 7FFFFFFE02000. This comes from vmalloc area's size.
This shows "core" size, not  memory size.

This patch shows the size by updating "size" field in struct proc_dir_entry.
Later, lookup routine will create inode and fill inode->i_size based
on this value. Then, this has a problem.

 - Once inode is cached, inode->i_size will never be updated.

Then, this patch is not memory-hotplug-aware.

To update inode->i_size, we have to know dentry or inode.
But there is no way to lookup them by inside kernel. Hmmm....
Next patch will try it.

Changelog:
 -moved upadting ->size under lock.

Cc: WANG Cong <xiyou.wangcong@gmail.com>
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
 fs/proc/kcore.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Index: mmotm-2.6.31-Sep14/fs/proc/kcore.c
===================================================================
--- mmotm-2.6.31-Sep14.orig/fs/proc/kcore.c
+++ mmotm-2.6.31-Sep14/fs/proc/kcore.c
@@ -107,6 +107,8 @@ static void free_kclist_ents(struct list
  */
 static void __kcore_update_ram(struct list_head *list)
 {
+	int nphdr;
+	size_t size;
 	struct kcore_list *tmp, *pos;
 	LIST_HEAD(garbage);
 
@@ -121,6 +123,7 @@ static void __kcore_update_ram(struct li
 	} else
 		list_splice(list, &garbage);
 	kcore_need_update = 0;
+	proc_root_kcore->size = get_kcore_size(&nphdr, &size);
 	write_unlock(&kclist_lock);
 
 	free_kclist_ents(&garbage);
@@ -429,7 +432,8 @@ read_kcore(struct file *file, char __use
 	unsigned long start;
 
 	read_lock(&kclist_lock);
-	proc_root_kcore->size = size = get_kcore_size(&nphdr, &elf_buflen);
+	size = get_kcore_size(&nphdr, &elf_buflen);
+
 	if (buflen == 0 || *fpos >= size) {
 		read_unlock(&kclist_lock);
 		return 0;







--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3/3][mmotm] updateing size of kcore

[Américo Wang]

On Thu, Sep 17, 2009 at 10:45 AM, KAMEZAWA Hiroyuki
<kamezawa.hiroyu@jp.fujitsu.com> wrote:
>
> After memory hotplug (or other events in future), kcore size
> can be modified.
>
> To update inode->i_size, we have to know inode/dentry but we
> can't get it from inside /proc directly.
> But considerinyg memory hotplug, kcore image is updated only when
> it's opened. Then, updating inode->i_size at open() is enough.
>
> Cc: WANG Cong <xiyou.wangcong@gmail.com>
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>


This patch looks fine.

However, I am thinking if kcore is the only file under /proc whose size
is changed dynamically? If no, that probably means we need to change
generic proc code.

Thanks!

> ---
>  fs/proc/kcore.c |    5 +++++
>  1 file changed, 5 insertions(+)
>
> Index: mmotm-2.6.31-Sep14/fs/proc/kcore.c
> ===================================================================
> --- mmotm-2.6.31-Sep14.orig/fs/proc/kcore.c
> +++ mmotm-2.6.31-Sep14/fs/proc/kcore.c
> @@ -546,6 +546,11 @@ static int open_kcore(struct inode *inod
>                return -EPERM;
>        if (kcore_need_update)
>                kcore_update_ram();
> +       if (i_size_read(inode) != proc_root_kcore->size) {
> +               mutex_lock(&inode->i_mutex);
> +               i_size_write(inode, proc_root_kcore->size);
> +               mutex_unlock(&inode->i_mutex);
> +       }
>        return 0;
>  }
>
>
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3/3][mmotm] updateing size of kcore

[KAMEZAWA Hiroyuki]

On Thu, 17 Sep 2009 14:59:35 +0800
AmA(C)rico Wang <xiyou.wangcong@gmail.com> wrote:

> On Thu, Sep 17, 2009 at 10:45 AM, KAMEZAWA Hiroyuki
> <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> >
> > After memory hotplug (or other events in future), kcore size
> > can be modified.
> >
> > To update inode->i_size, we have to know inode/dentry but we
> > can't get it from inside /proc directly.
> > But considerinyg memory hotplug, kcore image is updated only when
> > it's opened. Then, updating inode->i_size at open() is enough.
> >
> > Cc: WANG Cong <xiyou.wangcong@gmail.com>
> > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> 
> 
> This patch looks fine.
> 
> However, I am thinking if kcore is the only file under /proc whose size
> is changed dynamically? If no, that probably means we need to change
> generic proc code.
> 
I tried yesteray, and back to this ;)

One thing which makes me confused is that there is no way to get
inode or dentry from proc_dir_entry.

I tried to rewrite proc_getattr() for cheating "ls". But it just works for
"stat" and inode->i_size is used more widely. So, this implementation now.

But considering practically, inode->i_size itself is not meaningful in /proc
files even if it's correct. For example, /proc/vmstat or /proc/stat,
/proc/<pid>/maps... etc...

inode->i_size will be dynamically changed while reading. Now, most of users
know regular files under /proc is not a "real" file and handle them in proper
way. (programs can be used with pipe/stdin works well.)

I wonder /proc/kcore is a special one, which gdb/objdump/readelf may access.
Above 3 programs are for "usual" files and not considering pseudo files under
/proc. So, I think adding generic i->i_size support is an overkill until
there are users depends on that.

Thanks,
-Kame


> Thanks!
> 
> > ---
> > A fs/proc/kcore.c | A  A 5 +++++
> > A 1 file changed, 5 insertions(+)
> >
> > Index: mmotm-2.6.31-Sep14/fs/proc/kcore.c
> > ===================================================================
> > --- mmotm-2.6.31-Sep14.orig/fs/proc/kcore.c
> > +++ mmotm-2.6.31-Sep14/fs/proc/kcore.c
> > @@ -546,6 +546,11 @@ static int open_kcore(struct inode *inod
> > A  A  A  A  A  A  A  A return -EPERM;
> > A  A  A  A if (kcore_need_update)
> > A  A  A  A  A  A  A  A kcore_update_ram();
> > + A  A  A  if (i_size_read(inode) != proc_root_kcore->size) {
> > + A  A  A  A  A  A  A  mutex_lock(&inode->i_mutex);
> > + A  A  A  A  A  A  A  i_size_write(inode, proc_root_kcore->size);
> > + A  A  A  A  A  A  A  mutex_unlock(&inode->i_mutex);
> > + A  A  A  }
> > A  A  A  A return 0;
> > A }
> >
> >
> >
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3/3][mmotm] updateing size of kcore

[Américo Wang]

On Thu, Sep 17, 2009 at 3:14 PM, KAMEZAWA Hiroyuki
<kamezawa.hiroyu@jp.fujitsu.com> wrote:
> On Thu, 17 Sep 2009 14:59:35 +0800
> Américo Wang <xiyou.wangcong@gmail.com> wrote:
>
>> On Thu, Sep 17, 2009 at 10:45 AM, KAMEZAWA Hiroyuki
>> <kamezawa.hiroyu@jp.fujitsu.com> wrote:
>> >
>> > After memory hotplug (or other events in future), kcore size
>> > can be modified.
>> >
>> > To update inode->i_size, we have to know inode/dentry but we
>> > can't get it from inside /proc directly.
>> > But considerinyg memory hotplug, kcore image is updated only when
>> > it's opened. Then, updating inode->i_size at open() is enough.
>> >
>> > Cc: WANG Cong <xiyou.wangcong@gmail.com>
>> > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
>>
>>
>> This patch looks fine.
>>
>> However, I am thinking if kcore is the only file under /proc whose size
>> is changed dynamically? If no, that probably means we need to change
>> generic proc code.
>>
> I tried yesteray, and back to this ;)
>
> One thing which makes me confused is that there is no way to get
> inode or dentry from proc_dir_entry.
>
> I tried to rewrite proc_getattr() for cheating "ls". But it just works for
> "stat" and inode->i_size is used more widely. So, this implementation now.

Yeah, stat->size is from inode->i_size.

>
> But considering practically, inode->i_size itself is not meaningful in /proc
> files even if it's correct. For example, /proc/vmstat or /proc/stat,
> /proc/<pid>/maps... etc...

Yes.

>
> inode->i_size will be dynamically changed while reading. Now, most of users
> know regular files under /proc is not a "real" file and handle them in proper
> way. (programs can be used with pipe/stdin works well.)
>
> I wonder /proc/kcore is a special one, which gdb/objdump/readelf may access.
> Above 3 programs are for "usual" files and not considering pseudo files under
> /proc. So, I think adding generic i->i_size support is an overkill until
> there are users depends on that.

At least I can't think out another /proc file as special as kcore. :-/

Thanks for your analysis, no problem with this patch.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/3][mmotm] showing size of kcore v2

[Américo Wang]

On Thu, Sep 17, 2009 at 2:10 PM, KAMEZAWA Hiroyuki
<kamezawa.hiroyu@jp.fujitsu.com> wrote:
> On Thu, 17 Sep 2009 14:02:39 +0800
> Américo Wang <xiyou.wangcong@gmail.com> wrote:
>> > @@ -124,6 +126,7 @@ static void __kcore_update_ram(struct li
>> >        write_unlock(&kclist_lock);
>> >
>> >        free_kclist_ents(&garbage);
>> > +       proc_root_kcore->size = get_kcore_size(&nphdr, &size);
>>
>>
>> This makes me to think if we will have some race condition here?
>> Two processes can open kcore at the same time...
>>
> Finally,
> ==
> static void __kcore_update_ram(struct list_head *list)
> {
>  write_lock(&kclist_lock);
>        if (kcore_need_update) {
>                list_for_each_entry_safe(pos, tmp, &kclist_head, list) {
>                        if (pos->type == KCORE_RAM
>                                || pos->type == KCORE_VMEMMAP)
>                                list_move(&pos->list, &garbage);
>                }
>                list_splice_tail(list, &kclist_head);
>        } else
>                list_splice(list, &garbage);
>        kcore_need_update = 0;
>        write_unlock(&kclist_lock);
> }
>
> kclist itself is double checked under write_lock.
> And, once updated, get_kcore_size()'s return vaule is static.


Imagine one process does get_kcore_size(), then another process
is scheduled, who also does get_kcore_size() but at this time,
memory size is changed, so it gets a different value. If then the
second process writes to proc_root_kcore->size before the first one
does, the proc_root_kcore->size is wrong.

Am I missing something here?

> So, I think there are no race. But..Hmm...is this clearer ?
>
> ==
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>


Yes, this version should be OK.

Acked-by: WANG Cong <xiyou.wangcong@gmail.com>

>
> Now, size of /proc/kcore which can be read by 'ls -l' is 0.
> But it's not correct value.
>
> This is a patch for showing size of /proc/kcore as following.
>
> On x86-64, ls -l shows
>  ... root root 140737486266368 2009-09-17 10:29 /proc/kcore
> Then, 7FFFFFFE02000. This comes from vmalloc area's size.
> This shows "core" size, not  memory size.
>
> This patch shows the size by updating "size" field in struct proc_dir_entry.
> Later, lookup routine will create inode and fill inode->i_size based
> on this value. Then, this has a problem.
>
>  - Once inode is cached, inode->i_size will never be updated.
>
> Then, this patch is not memory-hotplug-aware.
>
> To update inode->i_size, we have to know dentry or inode.
> But there is no way to lookup them by inside kernel. Hmmm....
> Next patch will try it.
>
> Changelog:
>  -moved upadting ->size under lock.
>
> Cc: WANG Cong <xiyou.wangcong@gmail.com>
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> ---
>  fs/proc/kcore.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> Index: mmotm-2.6.31-Sep14/fs/proc/kcore.c
> ===================================================================
> --- mmotm-2.6.31-Sep14.orig/fs/proc/kcore.c
> +++ mmotm-2.6.31-Sep14/fs/proc/kcore.c
> @@ -107,6 +107,8 @@ static void free_kclist_ents(struct list
>  */
>  static void __kcore_update_ram(struct list_head *list)
>  {
> +       int nphdr;
> +       size_t size;
>        struct kcore_list *tmp, *pos;
>        LIST_HEAD(garbage);
>
> @@ -121,6 +123,7 @@ static void __kcore_update_ram(struct li
>        } else
>                list_splice(list, &garbage);
>        kcore_need_update = 0;
> +       proc_root_kcore->size = get_kcore_size(&nphdr, &size);
>        write_unlock(&kclist_lock);
>
>        free_kclist_ents(&garbage);
> @@ -429,7 +432,8 @@ read_kcore(struct file *file, char __use
>        unsigned long start;
>
>        read_lock(&kclist_lock);
> -       proc_root_kcore->size = size = get_kcore_size(&nphdr, &elf_buflen);
> +       size = get_kcore_size(&nphdr, &elf_buflen);
> +
>        if (buflen == 0 || *fpos >= size) {
>                read_unlock(&kclist_lock);
>                return 0;
>
>
>
>
>
>
>
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>