On Mon, 19 Oct 2015 09:54:16 +0800
Jiaxing Wang <[email protected]> wrote:
> Sorry for the last patch, please use this to add stub for
> debugfs_create_automount().
Can you fold this into patch number two and resubmit that patch.
Thanks,
-- Steve
>
> >From b3b877d8d9fd9795ea1055042039a272e47f4dc5 Mon Sep 17 00:00:00 2001
> From: Jiaxing Wang <[email protected]>
> Date: Mon, 19 Oct 2015 09:46:12 +0800
> Subject: [PATCH] debugfs: Add stub function for debugfs_create_automount().
>
> Add stub for debugfs_create_automount() for when debugfs is not configured
> in.
>
> Signed-off-by: Jiaxing Wang <[email protected]>
> ---
> include/linux/debugfs.h | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
> index 9beb636..b42ef88 100644
> --- a/include/linux/debugfs.h
> +++ b/include/linux/debugfs.h
> @@ -160,6 +160,14 @@ static inline struct dentry *debugfs_create_symlink(const char *name,
> return ERR_PTR(-ENODEV);
> }
>
> +static inline struct dentry *debugfs_create_automount(const char *name,
> + struct dentry *parent,
> + struct vfsmount *(*f)(void *),
> + void *data)
> +{
> + return ERR_PTR(-ENODEV);
> +}
> +
> static inline void debugfs_remove(struct dentry *dentry)
> { }
>
Currently tracing_init_dentry() returns -ENODEV when debugfs is not
initialized, which causes tracefs not populated with tracing files and
directories, so we will get an empty directory even after we manually
mount tracefs.
We can make tracing_init_dentry() return NULL as long as tracefs
is initialized and get a populated tracefs.
We also need to make global_trace.dir not NULL in order to pass the checks
in tracing_get_dentry() and add_tracer_options().
Also added stub debugfs_create_automount() for when debugfs is not
configured in.
Signed-off-by: Jiaxing Wang <[email protected]>
---
include/linux/debugfs.h | 8 ++++++++
kernel/trace/Kconfig | 1 -
kernel/trace/trace.c | 29 +++++++++++++++++------------
3 files changed, 25 insertions(+), 13 deletions(-)
diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index 9beb636..b42ef88 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -160,6 +160,14 @@ static inline struct dentry *debugfs_create_symlink(const char *name,
return ERR_PTR(-ENODEV);
}
+static inline struct dentry *debugfs_create_automount(const char *name,
+ struct dentry *parent,
+ struct vfsmount *(*f)(void *),
+ void *data)
+{
+ return ERR_PTR(-ENODEV);
+}
+
static inline void debugfs_remove(struct dentry *dentry)
{ }
diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index 1153c43..59f6377f 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -95,7 +95,6 @@ config RING_BUFFER_ALLOW_SWAP
config TRACING
bool
- select DEBUG_FS
select RING_BUFFER
select STACKTRACE if STACKTRACE_SUPPORT
select TRACEPOINTS
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 6e79408..6dd064e 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -6706,6 +6706,8 @@ static struct vfsmount *trace_automount(void *ingore)
return mnt;
}
+#define TRACE_TOP_DIR_ENTRY ((struct dentry *)1)
+
/**
* tracing_init_dentry - initialize top level trace array
*
@@ -6716,27 +6718,30 @@ static struct vfsmount *trace_automount(void *ingore)
struct dentry *tracing_init_dentry(void)
{
struct trace_array *tr = &global_trace;
+ struct dentry *traced;
/* The top level trace array uses NULL as parent */
if (tr->dir)
return NULL;
- if (WARN_ON(!debugfs_initialized()))
+ if (WARN_ON(!tracefs_initialized()))
return ERR_PTR(-ENODEV);
- /*
- * As there may still be users that expect the tracing
- * files to exist in debugfs/tracing, we must automount
- * the tracefs file system there, so older tools still
- * work with the newer kerenl.
- */
- tr->dir = debugfs_create_automount("tracing", NULL,
- trace_automount, NULL);
- if (!tr->dir) {
- pr_warn_once("Could not create debugfs directory 'tracing'\n");
- return ERR_PTR(-ENOMEM);
+ if (debugfs_initialized()) {
+ /*
+ * As there may still be users that expect the tracing
+ * files to exist in debugfs/tracing, we must automount
+ * the tracefs file system there, so older tools still
+ * work with the newer kerenl.
+ */
+ traced = debugfs_create_automount("tracing", NULL,
+ trace_automount, NULL);
+ if (!traced)
+ pr_warn_once("Could not create debugfs directory 'tracing'\n");
}
+ tr->dir = TRACE_TOP_DIR_ENTRY;
+
return NULL;
}
--
2.1.4
On Wed, 4 Nov 2015 09:11:18 +0800
Jiaxing Wang <[email protected]> wrote:
> Currently tracing_init_dentry() returns -ENODEV when debugfs is not
> initialized, which causes tracefs not populated with tracing files and
> directories, so we will get an empty directory even after we manually
> mount tracefs.
>
> We can make tracing_init_dentry() return NULL as long as tracefs
> is initialized and get a populated tracefs.
>
> We also need to make global_trace.dir not NULL in order to pass the checks
> in tracing_get_dentry() and add_tracer_options().
>
> Also added stub debugfs_create_automount() for when debugfs is not
> configured in.
>
> Signed-off-by: Jiaxing Wang <[email protected]>
> ---
> include/linux/debugfs.h | 8 ++++++++
> kernel/trace/Kconfig | 1 -
> kernel/trace/trace.c | 29 +++++++++++++++++------------
> 3 files changed, 25 insertions(+), 13 deletions(-)
>
> diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
> index 9beb636..b42ef88 100644
> --- a/include/linux/debugfs.h
> +++ b/include/linux/debugfs.h
> @@ -160,6 +160,14 @@ static inline struct dentry *debugfs_create_symlink(const char *name,
> return ERR_PTR(-ENODEV);
> }
>
> +static inline struct dentry *debugfs_create_automount(const char *name,
> + struct dentry *parent,
> + struct vfsmount *(*f)(void *),
> + void *data)
> +{
> + return ERR_PTR(-ENODEV);
> +}
This part needs an Acked-by from Greg KH.
> +
> static inline void debugfs_remove(struct dentry *dentry)
> { }
>
> diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
> index 1153c43..59f6377f 100644
> --- a/kernel/trace/Kconfig
> +++ b/kernel/trace/Kconfig
> @@ -95,7 +95,6 @@ config RING_BUFFER_ALLOW_SWAP
>
> config TRACING
> bool
> - select DEBUG_FS
> select RING_BUFFER
> select STACKTRACE if STACKTRACE_SUPPORT
> select TRACEPOINTS
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 6e79408..6dd064e 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -6706,6 +6706,8 @@ static struct vfsmount *trace_automount(void *ingore)
> return mnt;
> }
>
> +#define TRACE_TOP_DIR_ENTRY ((struct dentry *)1)
> +
> /**
> * tracing_init_dentry - initialize top level trace array
> *
> @@ -6716,27 +6718,30 @@ static struct vfsmount *trace_automount(void *ingore)
> struct dentry *tracing_init_dentry(void)
> {
> struct trace_array *tr = &global_trace;
> + struct dentry *traced;
>
> /* The top level trace array uses NULL as parent */
> if (tr->dir)
> return NULL;
>
> - if (WARN_ON(!debugfs_initialized()))
> + if (WARN_ON(!tracefs_initialized()))
> return ERR_PTR(-ENODEV);
>
> - /*
> - * As there may still be users that expect the tracing
> - * files to exist in debugfs/tracing, we must automount
> - * the tracefs file system there, so older tools still
> - * work with the newer kerenl.
> - */
> - tr->dir = debugfs_create_automount("tracing", NULL,
> - trace_automount, NULL);
> - if (!tr->dir) {
> - pr_warn_once("Could not create debugfs directory 'tracing'\n");
> - return ERR_PTR(-ENOMEM);
> + if (debugfs_initialized()) {
> + /*
> + * As there may still be users that expect the tracing
> + * files to exist in debugfs/tracing, we must automount
> + * the tracefs file system there, so older tools still
> + * work with the newer kerenl.
> + */
> + traced = debugfs_create_automount("tracing", NULL,
> + trace_automount, NULL);
> + if (!traced)
> + pr_warn_once("Could not create debugfs directory 'tracing'\n");
This should return a warning, and please keep the tr->dir instead of
this new traced variable.
> }
>
> + tr->dir = TRACE_TOP_DIR_ENTRY;
> +
Also, no need to add this, because if debugfs is not initialize, then
tr->dir would be ERR_PTR(-ENODEV), which still works as tr->dir is not
NULL.
-- Steve
> return NULL;
> }
>
On Wed, Nov 04, 2015 at 10:03:39AM -0500, Steven Rostedt wrote:
> On Wed, 4 Nov 2015 09:11:18 +0800
> Jiaxing Wang <[email protected]> wrote:
>
> > Currently tracing_init_dentry() returns -ENODEV when debugfs is not
> > initialized, which causes tracefs not populated with tracing files and
> > directories, so we will get an empty directory even after we manually
> > mount tracefs.
> >
> > We can make tracing_init_dentry() return NULL as long as tracefs
> > is initialized and get a populated tracefs.
> >
> > We also need to make global_trace.dir not NULL in order to pass the checks
> > in tracing_get_dentry() and add_tracer_options().
> >
> > Also added stub debugfs_create_automount() for when debugfs is not
> > configured in.
The debugfs change should be split out into a separate patch, which I'll
be glad to take through my tree, it isn't dependant on the tracing
code at all.
thanks,
greg k-h
On Wed, 4 Nov 2015 10:54:42 -0800
Greg Kroah-Hartman <[email protected]> wrote:
> On Wed, Nov 04, 2015 at 10:03:39AM -0500, Steven Rostedt wrote:
> > On Wed, 4 Nov 2015 09:11:18 +0800
> > Jiaxing Wang <[email protected]> wrote:
> >
> > > Currently tracing_init_dentry() returns -ENODEV when debugfs is not
> > > initialized, which causes tracefs not populated with tracing files and
> > > directories, so we will get an empty directory even after we manually
> > > mount tracefs.
> > >
> > > We can make tracing_init_dentry() return NULL as long as tracefs
> > > is initialized and get a populated tracefs.
> > >
> > > We also need to make global_trace.dir not NULL in order to pass the checks
> > > in tracing_get_dentry() and add_tracer_options().
> > >
> > > Also added stub debugfs_create_automount() for when debugfs is not
> > > configured in.
>
> The debugfs change should be split out into a separate patch, which I'll
> be glad to take through my tree, it isn't dependant on the tracing
> code at all.
>
I guess the tracing code is dependent on that change. As the tracing
code will then be compiled without the select DEBUGFS, and will fail to
compile if that change is not there.
That said. I think it's best to split it up, and have the debugfs
change go through your tree, and the other part go through mine without
the remove of the "select DEBUGFS". After both are in mainline, then we
can remove the select statement.
-- Steve
On Wed, Nov 04, 2015 at 10:03:39AM -0500, Steven Rostedt wrote:
> On Wed, 4 Nov 2015 09:11:18 +0800
> Jiaxing Wang <[email protected]> wrote:
>
> > Currently tracing_init_dentry() returns -ENODEV when debugfs is not
> > initialized, which causes tracefs not populated with tracing files and
> > directories, so we will get an empty directory even after we manually
> > mount tracefs.
> >
> > We can make tracing_init_dentry() return NULL as long as tracefs
> > is initialized and get a populated tracefs.
> >
> > We also need to make global_trace.dir not NULL in order to pass the checks
> > in tracing_get_dentry() and add_tracer_options().
> >
> > Also added stub debugfs_create_automount() for when debugfs is not
> > configured in.
> >
> > Signed-off-by: Jiaxing Wang <[email protected]>
> > ---
> > include/linux/debugfs.h | 8 ++++++++
> > kernel/trace/Kconfig | 1 -
> > kernel/trace/trace.c | 29 +++++++++++++++++------------
> > 3 files changed, 25 insertions(+), 13 deletions(-)
> >
> > diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
> > index 9beb636..b42ef88 100644
> > --- a/include/linux/debugfs.h
> > +++ b/include/linux/debugfs.h
> > @@ -160,6 +160,14 @@ static inline struct dentry *debugfs_create_symlink(const char *name,
> > return ERR_PTR(-ENODEV);
> > }
> >
> > +static inline struct dentry *debugfs_create_automount(const char *name,
> > + struct dentry *parent,
> > + struct vfsmount *(*f)(void *),
> > + void *data)
> > +{
> > + return ERR_PTR(-ENODEV);
> > +}
>
> This part needs an Acked-by from Greg KH.
>
> > +
> > static inline void debugfs_remove(struct dentry *dentry)
> > { }
> >
> > diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
> > index 1153c43..59f6377f 100644
> > --- a/kernel/trace/Kconfig
> > +++ b/kernel/trace/Kconfig
> > @@ -95,7 +95,6 @@ config RING_BUFFER_ALLOW_SWAP
> >
> > config TRACING
> > bool
> > - select DEBUG_FS
> > select RING_BUFFER
> > select STACKTRACE if STACKTRACE_SUPPORT
> > select TRACEPOINTS
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index 6e79408..6dd064e 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -6706,6 +6706,8 @@ static struct vfsmount *trace_automount(void *ingore)
> > return mnt;
> > }
> >
> > +#define TRACE_TOP_DIR_ENTRY ((struct dentry *)1)
> > +
> > /**
> > * tracing_init_dentry - initialize top level trace array
> > *
> > @@ -6716,27 +6718,30 @@ static struct vfsmount *trace_automount(void *ingore)
> > struct dentry *tracing_init_dentry(void)
> > {
> > struct trace_array *tr = &global_trace;
> > + struct dentry *traced;
> >
> > /* The top level trace array uses NULL as parent */
> > if (tr->dir)
> > return NULL;
> >
> > - if (WARN_ON(!debugfs_initialized()))
> > + if (WARN_ON(!tracefs_initialized()))
> > return ERR_PTR(-ENODEV);
> >
> > - /*
> > - * As there may still be users that expect the tracing
> > - * files to exist in debugfs/tracing, we must automount
> > - * the tracefs file system there, so older tools still
> > - * work with the newer kerenl.
> > - */
> > - tr->dir = debugfs_create_automount("tracing", NULL,
> > - trace_automount, NULL);
> > - if (!tr->dir) {
> > - pr_warn_once("Could not create debugfs directory 'tracing'\n");
> > - return ERR_PTR(-ENOMEM);
> > + if (debugfs_initialized()) {
> > + /*
> > + * As there may still be users that expect the tracing
> > + * files to exist in debugfs/tracing, we must automount
> > + * the tracefs file system there, so older tools still
> > + * work with the newer kerenl.
> > + */
> > + traced = debugfs_create_automount("tracing", NULL,
> > + trace_automount, NULL);
> > + if (!traced)
> > + pr_warn_once("Could not create debugfs directory 'tracing'\n");
>
> This should return a warning, and please keep the tr->dir instead of
> this new traced variable.
Do you mean return ERR_PTR(-ENOMEM); when debugfs_create_automount()
return NULL?
As long as tracefs is initialized, we can make tracing_init_dentry() return
NULL even if the debugfs automount point is not created(), and tracefs can
still be populated. If tracing_init_dentry() returns error in this case,
the caller of tracing_init_dentry() will not populate tracefs.
>
> > }
> >
> > + tr->dir = TRACE_TOP_DIR_ENTRY;
> > +
>
> Also, no need to add this, because if debugfs is not initialize, then
> tr->dir would be ERR_PTR(-ENODEV), which still works as tr->dir is not
> NULL.
If we accept debugfs_create_automount() return NULL, tr->dir can still
be NULL if we do tr->dir = debugfs_create_automount().
>
> -- Steve
>
> > return NULL;
> > }
> >
>
On Thu, 5 Nov 2015 13:23:01 +0800
Jiaxing Wang <[email protected]> wrote:
> > > - /*
> > > - * As there may still be users that expect the tracing
> > > - * files to exist in debugfs/tracing, we must automount
> > > - * the tracefs file system there, so older tools still
> > > - * work with the newer kerenl.
> > > - */
> > > - tr->dir = debugfs_create_automount("tracing", NULL,
> > > - trace_automount, NULL);
> > > - if (!tr->dir) {
> > > - pr_warn_once("Could not create debugfs directory 'tracing'\n");
> > > - return ERR_PTR(-ENOMEM);
> > > + if (debugfs_initialized()) {
> > > + /*
> > > + * As there may still be users that expect the tracing
> > > + * files to exist in debugfs/tracing, we must automount
> > > + * the tracefs file system there, so older tools still
> > > + * work with the newer kerenl.
> > > + */
> > > + traced = debugfs_create_automount("tracing", NULL,
> > > + trace_automount, NULL);
> > > + if (!traced)
> > > + pr_warn_once("Could not create debugfs directory 'tracing'\n");
> >
> > This should return a warning, and please keep the tr->dir instead of
> > this new traced variable.
> Do you mean return ERR_PTR(-ENOMEM); when debugfs_create_automount()
> return NULL?
Right.
> As long as tracefs is initialized, we can make tracing_init_dentry() return
> NULL even if the debugfs automount point is not created(), and tracefs can
> still be populated. If tracing_init_dentry() returns error in this case,
> the caller of tracing_init_dentry() will not populate tracefs.
But this is still a failure. tracing_init_dentry() now only mounts
tracefs on the debugfs/tracing directory. If it fails to do that when
debugfs is available, then it should fail, as it would break backward
compatibility with other tools.
If debugfs is not configured in, then it should set tr->dir to
whatever (ENOMEM is fine), and return NULL.
> >
> > > }
> > >
> > > + tr->dir = TRACE_TOP_DIR_ENTRY;
> > > +
> >
> > Also, no need to add this, because if debugfs is not initialize, then
> > tr->dir would be ERR_PTR(-ENODEV), which still works as tr->dir is not
> > NULL.
> If we accept debugfs_create_automount() return NULL, tr->dir can still
> be NULL if we do tr->dir = debugfs_create_automount().
What's wrong with that? This function is only to automount debugfs now.
Also, I think the first check should be:
if (WARN_ON(!tracefs_initialized()) ||
(IS_ENABLED(CONFIG_DEBUGFS) &&
WARN_ON(!debugfs_initialized()))
return ERR_PTR(-ENODEV);
Then we don't need the if (debugfs_initialized()) conditional.
-- Steve
> >
> > -- Steve
> >
> > > return NULL;
> > > }
> > >
> >
On Thu, Nov 05, 2015 at 10:54:32AM -0500, Steven Rostedt wrote:
> On Thu, 5 Nov 2015 13:23:01 +0800
> Jiaxing Wang <[email protected]> wrote:
>
> > > > - /*
> > > > - * As there may still be users that expect the tracing
> > > > - * files to exist in debugfs/tracing, we must automount
> > > > - * the tracefs file system there, so older tools still
> > > > - * work with the newer kerenl.
> > > > - */
> > > > - tr->dir = debugfs_create_automount("tracing", NULL,
> > > > - trace_automount, NULL);
> > > > - if (!tr->dir) {
> > > > - pr_warn_once("Could not create debugfs directory 'tracing'\n");
> > > > - return ERR_PTR(-ENOMEM);
> > > > + if (debugfs_initialized()) {
> > > > + /*
> > > > + * As there may still be users that expect the tracing
> > > > + * files to exist in debugfs/tracing, we must automount
> > > > + * the tracefs file system there, so older tools still
> > > > + * work with the newer kerenl.
> > > > + */
> > > > + traced = debugfs_create_automount("tracing", NULL,
> > > > + trace_automount, NULL);
> > > > + if (!traced)
> > > > + pr_warn_once("Could not create debugfs directory 'tracing'\n");
> > >
> > > This should return a warning, and please keep the tr->dir instead of
> > > this new traced variable.
> > Do you mean return ERR_PTR(-ENOMEM); when debugfs_create_automount()
> > return NULL?
>
> Right.
>
> > As long as tracefs is initialized, we can make tracing_init_dentry() return
> > NULL even if the debugfs automount point is not created(), and tracefs can
> > still be populated. If tracing_init_dentry() returns error in this case,
> > the caller of tracing_init_dentry() will not populate tracefs.
>
> But this is still a failure. tracing_init_dentry() now only mounts
> tracefs on the debugfs/tracing directory. If it fails to do that when
> debugfs is available, then it should fail, as it would break backward
> compatibility with other tools.
>
> If debugfs is not configured in, then it should set tr->dir to
> whatever (ENOMEM is fine), and return NULL.
>
>
> > >
> > > > }
> > > >
> > > > + tr->dir = TRACE_TOP_DIR_ENTRY;
> > > > +
> > >
> > > Also, no need to add this, because if debugfs is not initialize, then
> > > tr->dir would be ERR_PTR(-ENODEV), which still works as tr->dir is not
> > > NULL.
> > If we accept debugfs_create_automount() return NULL, tr->dir can still
> > be NULL if we do tr->dir = debugfs_create_automount().
>
> What's wrong with that? This function is only to automount debugfs now.
>
> Also, I think the first check should be:
>
> if (WARN_ON(!tracefs_initialized()) ||
> (IS_ENABLED(CONFIG_DEBUGFS) &&
> WARN_ON(!debugfs_initialized()))
> return ERR_PTR(-ENODEV);
>
> Then we don't need the if (debugfs_initialized()) conditional.
I will send you a new patch according to your suggestion, and if that
is OK, I will send a seperate patch to Greg KH to add stub for
debugfs_create_automount().
Thanks.
>
> -- Steve
>
>
> > >
> > > -- Steve
> > >
> > > > return NULL;
> > > > }
> > > >
> > >
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
Currently tracing_init_dentry() returns -ENODEV when debugfs is not
configured in, which causes tracefs not populated with tracing files and
directories, so we will get an empty directory even after we manually
mount tracefs.
We can make tracing_init_dentry() return NULL if debugfs is not
configured in and can manually mount tracefs. But return -ENODEV
if debugfs is configured in but not initialized or failed to create
automount point as that would break backward compatibility with older
tools.
Signed-off-by: Jiaxing Wang <[email protected]>
---
kernel/trace/trace.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 6e79408..c89296f 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -6721,7 +6721,9 @@ struct dentry *tracing_init_dentry(void)
if (tr->dir)
return NULL;
- if (WARN_ON(!debugfs_initialized()))
+ if (WARN_ON(!tracefs_initialized()) ||
+ (IS_ENABLED(CONFIG_DEBUG_FS) &&
+ WARN_ON(!debugfs_initialized())))
return ERR_PTR(-ENODEV);
/*
--
2.1.4