2011-03-07 08:51:33

by Chen, Gong

[permalink] [raw]
Subject: [PATCH] page-types.c: add a new argument of debugfs path

page-types.c doesn't supply a way to specify the debugfs path and
the original debugfs path is not usual on most machines. Add a
new argument to set the debugfs path easily.

Signed-off-by: Chen Gong <[email protected]>
---
Documentation/vm/page-types.c | 33 ++++++++++++++++++++++++---------
1 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/Documentation/vm/page-types.c b/Documentation/vm/page-types.c
index cc96ee2..1ebe87d 100644
--- a/Documentation/vm/page-types.c
+++ b/Documentation/vm/page-types.c
@@ -184,7 +184,7 @@ static int kpageflags_fd;
static int opt_hwpoison;
static int opt_unpoison;

-static const char hwpoison_debug_fs[] = "/debug/hwpoison";
+static char hwpoison_debug_fs[256] = "/sys/kernel/debug";
static int hwpoison_inject_fd;
static int hwpoison_forget_fd;

@@ -464,28 +464,37 @@ static uint64_t kpageflags_flags(uint64_t flags)
return flags;
}

+static void prepare_debugfs(const char *optarg)
+{
+ /*
+ * avoid too long filename.
+ * "/hwpoison/unpoison-pfn" occupies 22 characters
+ */
+ strncpy(hwpoison_debug_fs, optarg, 255 - 22);
+}
+
/*
* page actions
*/

static void prepare_hwpoison_fd(void)
{
- char buf[100];
+ char buf[256];

if (opt_hwpoison && !hwpoison_inject_fd) {
- sprintf(buf, "%s/corrupt-pfn", hwpoison_debug_fs);
+ sprintf(buf, "%s/hwpoison/corrupt-pfn", hwpoison_debug_fs);
hwpoison_inject_fd = checked_open(buf, O_WRONLY);
}

if (opt_unpoison && !hwpoison_forget_fd) {
- sprintf(buf, "%s/unpoison-pfn", hwpoison_debug_fs);
+ sprintf(buf, "%s/hwpoison/unpoison-pfn", hwpoison_debug_fs);
hwpoison_forget_fd = checked_open(buf, O_WRONLY);
}
}

static int hwpoison_page(unsigned long offset)
{
- char buf[100];
+ char buf[256];
int len;

len = sprintf(buf, "0x%lx\n", offset);
@@ -499,7 +508,7 @@ static int hwpoison_page(unsigned long offset)

static int unpoison_page(unsigned long offset)
{
- char buf[100];
+ char buf[256];
int len;

len = sprintf(buf, "0x%lx\n", offset);
@@ -686,6 +695,7 @@ static void usage(void)
"page-types [options]\n"
" -r|--raw Raw mode, for kernel developers\n"
" -d|--describe flags Describe flags\n"
+" -D|--debugfs debugfs-path specify the debugfs path\n"
" -a|--addr addr-spec Walk a range of pages\n"
" -b|--bits bits-spec Walk pages with specified bits\n"
" -p|--pid pid Walk process address space\n"
@@ -917,6 +927,7 @@ static const struct option opts[] = {
{ "addr" , 1, NULL, 'a' },
{ "bits" , 1, NULL, 'b' },
{ "describe" , 1, NULL, 'd' },
+ { "debugfs" , 1, NULL, 'D' },
{ "list" , 0, NULL, 'l' },
{ "list-each" , 0, NULL, 'L' },
{ "no-summary", 0, NULL, 'N' },
@@ -933,7 +944,7 @@ int main(int argc, char *argv[])
page_size = getpagesize();

while ((c = getopt_long(argc, argv,
- "rp:f:a:b:d:lLNXxh", opts, NULL)) != -1) {
+ "rp:f:a:b:d:D:lLNXxh", opts, NULL)) != -1) {
switch (c) {
case 'r':
opt_raw = 1;
@@ -953,6 +964,9 @@ int main(int argc, char *argv[])
case 'd':
describe_flags(optarg);
exit(0);
+ case 'D':
+ prepare_debugfs(optarg);
+ break;
case 'l':
opt_list = 1;
break;
@@ -964,11 +978,9 @@ int main(int argc, char *argv[])
break;
case 'X':
opt_hwpoison = 1;
- prepare_hwpoison_fd();
break;
case 'x':
opt_unpoison = 1;
- prepare_hwpoison_fd();
break;
case 'h':
usage();
@@ -979,6 +991,9 @@ int main(int argc, char *argv[])
}
}

+ if (opt_hwpoison == 1 || opt_unpoison == 1)
+ prepare_hwpoison_fd();
+
if (opt_list && opt_pid)
printf("voffset\t");
if (opt_list == 1)
--
1.7.3.1.120.g38a18


2011-03-07 09:42:42

by KOSAKI Motohiro

[permalink] [raw]
Subject: Re: [PATCH] page-types.c: add a new argument of debugfs path

> page-types.c doesn't supply a way to specify the debugfs path and
> the original debugfs path is not usual on most machines. Add a
> new argument to set the debugfs path easily.
>
> Signed-off-by: Chen Gong <[email protected]>

Hi

Why do we need to set debugfs path manually? Instead I'd suggested to
read /proc/mount and detect it automatically.

Thanks.


2011-03-07 11:40:25

by Fengguang Wu

[permalink] [raw]
Subject: Re: [PATCH] page-types.c: add a new argument of debugfs path

On Mon, Mar 07, 2011 at 05:42:39PM +0800, KOSAKI Motohiro wrote:
> > page-types.c doesn't supply a way to specify the debugfs path and
> > the original debugfs path is not usual on most machines. Add a
> > new argument to set the debugfs path easily.
> >
> > Signed-off-by: Chen Gong <[email protected]>
>
> Hi
>
> Why do we need to set debugfs path manually? Instead I'd suggested to
> read /proc/mount and detect it automatically.

Good idea! And could reuse tools/perf/util/debugfs.c for finding out
and even audo-mounting debugfs.

Thanks,
Fengguang

2011-03-08 05:01:35

by Chen, Gong

[permalink] [raw]
Subject: Re: [PATCH V2] page-types.c: auto debugfs mount for hwpoison operation

page-types.c doesn't supply a way to specify the debugfs path and
the original debugfs path is not usual on most machines. This patch
supplies a way to auto mount debugfs if needed.

This patch is heavily inspired by tools/perf/utils/debugfs.c

Signed-off-by: Chen Gong <[email protected]>
---
Documentation/vm/page-types.c | 105
+++++++++++++++++++++++++++++++++++++++--
1 files changed, 101 insertions(+), 4 deletions(-)

diff --git a/Documentation/vm/page-types.c b/Documentation/vm/page-types.c
index cc96ee2..303b4ed 100644
--- a/Documentation/vm/page-types.c
+++ b/Documentation/vm/page-types.c
@@ -32,8 +32,20 @@
#include <sys/types.h>
#include <sys/errno.h>
#include <sys/fcntl.h>
+#include <sys/mount.h>
+#include <sys/statfs.h>
+#include "../../include/linux/magic.h"


+#ifndef MAX_PATH
+# define MAX_PATH 256
+#endif
+
+#ifndef STR
+# define _STR(x) #x
+# define STR(x) _STR(x)
+#endif
+
/*
* pagemap kernel ABI bits
*/
@@ -152,6 +164,12 @@ static const char *page_flag_names[] = {
};


+static const char *debugfs_known_mountpoints[] = {
+ "/sys/kernel/debug",
+ "/debug",
+ 0,
+};
+
/*
* data structures
*/
@@ -184,7 +202,7 @@ static int kpageflags_fd;
static int opt_hwpoison;
static int opt_unpoison;

-static const char hwpoison_debug_fs[] = "/debug/hwpoison";
+static char hwpoison_debug_fs[MAX_PATH+1];
static int hwpoison_inject_fd;
static int hwpoison_forget_fd;

@@ -464,21 +482,100 @@ static uint64_t kpageflags_flags(uint64_t flags)
return flags;
}

+/* verify that a mountpoint is actually a debugfs instance */
+int debugfs_valid_mountpoint(const char *debugfs)
+{
+ struct statfs st_fs;
+
+ if (statfs(debugfs, &st_fs) < 0)
+ return -ENOENT;
+ else if (st_fs.f_type != (long) DEBUGFS_MAGIC)
+ return -ENOENT;
+
+ return 0;
+}
+
+/* find the path to the mounted debugfs */
+const char *debugfs_find_mountpoint(void)
+{
+ const char **ptr;
+ char type[100];
+ FILE *fp;
+
+ ptr = debugfs_known_mountpoints;
+ while (*ptr) {
+ if (debugfs_valid_mountpoint(*ptr) == 0) {
+ strcpy(hwpoison_debug_fs, *ptr);
+ return hwpoison_debug_fs;
+ }
+ ptr++;
+ }
+
+ /* give up and parse /proc/mounts */
+ fp = fopen("/proc/mounts", "r");
+ if (fp == NULL)
+ perror("Can't open /proc/mounts for read");
+
+ while (fscanf(fp, "%*s %"
+ STR(MAX_PATH)
+ "s %99s %*s %*d %*d\n",
+ hwpoison_debug_fs, type) == 2) {
+ if (strcmp(type, "debugfs") == 0)
+ break;
+ }
+ fclose(fp);
+
+ if (strcmp(type, "debugfs") != 0)
+ return NULL;
+
+ return hwpoison_debug_fs;
+}
+
+/* mount the debugfs somewhere if it's not mounted */
+
+void debugfs_mount()
+{
+ const char **ptr;
+
+ /* see if it's already mounted */
+ if (debugfs_find_mountpoint())
+ return;
+
+ ptr = debugfs_known_mountpoints;
+ while (*ptr) {
+ if (mount(NULL, *ptr, "debugfs", 0, NULL) == 0) {
+ /* save the mountpoint */
+ strcpy(hwpoison_debug_fs, *ptr);
+ break;
+ }
+ ptr++;
+ }
+
+ if (*ptr == NULL) {
+ perror("mount debugfs");
+ exit(EXIT_FAILURE);
+ }
+}
+
/*
* page actions
*/

static void prepare_hwpoison_fd(void)
{
- char buf[100];
+ char buf[MAX_PATH + 1];
+
+ debugfs_mount();

if (opt_hwpoison && !hwpoison_inject_fd) {
- sprintf(buf, "%s/corrupt-pfn", hwpoison_debug_fs);
+ snprintf(buf, MAX_PATH, "%s/hwpoison/corrupt-pfn",
+ hwpoison_debug_fs);
hwpoison_inject_fd = checked_open(buf, O_WRONLY);
}

if (opt_unpoison && !hwpoison_forget_fd) {
- sprintf(buf, "%s/unpoison-pfn", hwpoison_debug_fs);
+ snprintf(buf, MAX_PATH, "%s/hwpoison/unpoison-pfn",
+ hwpoison_debug_fs);
hwpoison_forget_fd = checked_open(buf, O_WRONLY);
}
}
--
1.7.3.1.120.g38a18

2011-03-08 06:08:13

by KOSAKI Motohiro

[permalink] [raw]
Subject: Re: [PATCH V2] page-types.c: auto debugfs mount for hwpoison operation

> page-types.c doesn't supply a way to specify the debugfs path and
> the original debugfs path is not usual on most machines. This patch
> supplies a way to auto mount debugfs if needed.
>
> This patch is heavily inspired by tools/perf/utils/debugfs.c
>
> Signed-off-by: Chen Gong <[email protected]>
> ---
> Documentation/vm/page-types.c | 105
> +++++++++++++++++++++++++++++++++++++++--
> 1 files changed, 101 insertions(+), 4 deletions(-)


Cute!
Reviewed-by: KOSAKI Motohiro <[email protected]>


2011-03-08 07:28:08

by Fengguang Wu

[permalink] [raw]
Subject: Re: [PATCH V2] page-types.c: auto debugfs mount for hwpoison operation

On Tue, Mar 08, 2011 at 01:01:09PM +0800, Chen Gong wrote:
> page-types.c doesn't supply a way to specify the debugfs path and
> the original debugfs path is not usual on most machines. This patch
> supplies a way to auto mount debugfs if needed.
>
> This patch is heavily inspired by tools/perf/utils/debugfs.c
>
> Signed-off-by: Chen Gong <[email protected]>
> ---
> Documentation/vm/page-types.c | 105
> +++++++++++++++++++++++++++++++++++++++--
> 1 files changed, 101 insertions(+), 4 deletions(-)

Thanks!

Reviewed-by: Wu Fengguang <[email protected]>

2011-03-12 00:29:47

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH V2] page-types.c: auto debugfs mount for hwpoison operation

On Tue, 08 Mar 2011 13:01:09 +0800
Chen Gong <[email protected]> wrote:

> page-types.c doesn't supply a way to specify the debugfs path and
> the original debugfs path is not usual on most machines. This patch
> supplies a way to auto mount debugfs if needed.
>
> This patch is heavily inspired by tools/perf/utils/debugfs.c
>
> Signed-off-by: Chen Gong <[email protected]>
> ---
> Documentation/vm/page-types.c | 105
> +++++++++++++++++++++++++++++++++++++++--
> 1 files changed, 101 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/vm/page-types.c b/Documentation/vm/page-types.c
> index cc96ee2..303b4ed 100644
> --- a/Documentation/vm/page-types.c
> +++ b/Documentation/vm/page-types.c
> @@ -32,8 +32,20 @@
> #include <sys/types.h>
> #include <sys/errno.h>
> #include <sys/fcntl.h>
> +#include <sys/mount.h>
> +#include <sys/statfs.h>
> +#include "../../include/linux/magic.h"

Your email client is space-stuffing the patches.

>
> +#ifndef MAX_PATH
> +# define MAX_PATH 256
> +#endif
> +
> +#ifndef STR
> +# define _STR(x) #x
> +# define STR(x) _STR(x)
> +#endif
> +
> /*
> * pagemap kernel ABI bits
> */
> @@ -152,6 +164,12 @@ static const char *page_flag_names[] = {
> };
>
>
> +static const char *debugfs_known_mountpoints[] = {
> + "/sys/kernel/debug",
> + "/debug",
> + 0,
> +};
> +
> /*
> * data structures
> */
> @@ -184,7 +202,7 @@ static int kpageflags_fd;
> static int opt_hwpoison;
> static int opt_unpoison;
>
> -static const char hwpoison_debug_fs[] = "/debug/hwpoison";
> +static char hwpoison_debug_fs[MAX_PATH+1];
> static int hwpoison_inject_fd;
> static int hwpoison_forget_fd;
>
> @@ -464,21 +482,100 @@ static uint64_t kpageflags_flags(uint64_t flags)
> return flags;
> }
>
> +/* verify that a mountpoint is actually a debugfs instance */
> +int debugfs_valid_mountpoint(const char *debugfs)

page-types.c carefully makes its symbols static. Let's continue to do
that.

--- a/Documentation/vm/page-types.c~documentation-vm-page-typesc-auto-debugfs-mount-for-hwpoison-operation-fix
+++ a/Documentation/vm/page-types.c
@@ -483,7 +483,7 @@ static uint64_t kpageflags_flags(uint64_
}

/* verify that a mountpoint is actually a debugfs instance */
-int debugfs_valid_mountpoint(const char *debugfs)
+static int debugfs_valid_mountpoint(const char *debugfs)
{
struct statfs st_fs;

@@ -496,7 +496,7 @@ int debugfs_valid_mountpoint(const char
}

/* find the path to the mounted debugfs */
-const char *debugfs_find_mountpoint(void)
+static const char *debugfs_find_mountpoint(void)
{
const char **ptr;
char type[100];
@@ -533,7 +533,7 @@ const char *debugfs_find_mountpoint(void

/* mount the debugfs somewhere if it's not mounted */

-void debugfs_mount()
+static void debugfs_mount()
{
const char **ptr;

_

> +{
> + struct statfs st_fs;
> +
> + if (statfs(debugfs, &st_fs) < 0)
> + return -ENOENT;
> + else if (st_fs.f_type != (long) DEBUGFS_MAGIC)
> + return -ENOENT;
> +
> + return 0;
> +}
> +
> +/* find the path to the mounted debugfs */
> +const char *debugfs_find_mountpoint(void)
> +{
> + const char **ptr;
> + char type[100];
> + FILE *fp;
> +
> + ptr = debugfs_known_mountpoints;
> + while (*ptr) {
> + if (debugfs_valid_mountpoint(*ptr) == 0) {
> + strcpy(hwpoison_debug_fs, *ptr);
> + return hwpoison_debug_fs;
> + }
> + ptr++;
> + }
> +
> + /* give up and parse /proc/mounts */
> + fp = fopen("/proc/mounts", "r");
> + if (fp == NULL)
> + perror("Can't open /proc/mounts for read");
> +
> + while (fscanf(fp, "%*s %"
> + STR(MAX_PATH)
> + "s %99s %*s %*d %*d\n",
> + hwpoison_debug_fs, type) == 2) {
> + if (strcmp(type, "debugfs") == 0)
> + break;
> + }
> + fclose(fp);
> +
> + if (strcmp(type, "debugfs") != 0)
> + return NULL;
> +
> + return hwpoison_debug_fs;
> +}
> +
> +/* mount the debugfs somewhere if it's not mounted */
> +
> +void debugfs_mount()
> +{
> + const char **ptr;
> +
> + /* see if it's already mounted */
> + if (debugfs_find_mountpoint())
> + return;
> +
> + ptr = debugfs_known_mountpoints;
> + while (*ptr) {
> + if (mount(NULL, *ptr, "debugfs", 0, NULL) == 0) {
> + /* save the mountpoint */
> + strcpy(hwpoison_debug_fs, *ptr);
> + break;
> + }
> + ptr++;
> + }
> +
> + if (*ptr == NULL) {
> + perror("mount debugfs");
> + exit(EXIT_FAILURE);
> + }
> +}

The application now silently mounts debugfs. Perhaps it should inform
the operator when it did this?