Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201030070442.457739-1-amistry@google.com> <20201030180403.1.I9c36fd7a0e4d52e300c1004a0f6f2fc705e2b065@changeid>
 <20201030072118.GB1495407@kroah.com>
In-Reply-To: <20201030072118.GB1495407@kroah.com>
From:   "Anand K. Mistry" <amistry@google.com>
Date:   Fri, 30 Oct 2020 18:30:49 +1100
Message-ID: <CAATStaMAyE+mbdjjRauYNKrj85h78_=tPpWC3PRzKHYPq8mh=w@mail.gmail.com>
Subject: Re: [PATCH 1/1] debugfs: Add a helper to export atomic64_t values
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     linux-kernel@vger.kernel.org,
        "Rafael J. Wysocki" <rafael@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, 30 Oct 2020 at 18:20, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Fri, Oct 30, 2020 at 06:04:42PM +1100, Anand K Mistry wrote:
> > This mirrors support for exporting atomic_t values.
> >
> > Signed-off-by: Anand K Mistry <amistry@google.com>
> >
> > ---
> >
> >  fs/debugfs/file.c       | 37 +++++++++++++++++++++++++++++++++++++
> >  include/linux/debugfs.h |  6 ++++++
> >  2 files changed, 43 insertions(+)
> >
> > diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> > index a768a09430c3..798bd3bdedec 100644
> > --- a/fs/debugfs/file.c
> > +++ b/fs/debugfs/file.c
> > @@ -770,6 +770,43 @@ void debugfs_create_atomic_t(const char *name, umode_t mode,
> >  }
> >  EXPORT_SYMBOL_GPL(debugfs_create_atomic_t);
> >
> > +static int debugfs_atomic64_t_set(void *data, u64 val)
> > +{
> > +     atomic64_set((atomic64_t *)data, val);
> > +     return 0;
> > +}
> > +static int debugfs_atomic64_t_get(void *data, u64 *val)
> > +{
> > +     *val = atomic64_read((atomic64_t *)data);
> > +     return 0;
> > +}
> > +DEFINE_DEBUGFS_ATTRIBUTE(fops_atomic64_t, debugfs_atomic64_t_get,
> > +                     debugfs_atomic64_t_set, "%lld\n");
> > +DEFINE_DEBUGFS_ATTRIBUTE(fops_atomic64_t_ro, debugfs_atomic64_t_get, NULL,
> > +                     "%lld\n");
> > +DEFINE_DEBUGFS_ATTRIBUTE(fops_atomic64_t_wo, NULL, debugfs_atomic64_t_set,
> > +                     "%lld\n");
> > +
> > +/**
> > + * debugfs_create_atomic64_t - create a debugfs file that is used to read and
> > + * write an atomic64_t value
> > + * @name: a pointer to a string containing the name of the file to create.
> > + * @mode: the permission that the file should have
> > + * @parent: a pointer to the parent dentry for this file.  This should be a
> > + *          directory dentry if set.  If this parameter is %NULL, then the
> > + *          file will be created in the root of the debugfs filesystem.
> > + * @value: a pointer to the variable that the file should read to and write
> > + *         from.
> > + */
> > +void debugfs_create_atomic64_t(const char *name, umode_t mode,
> > +                            struct dentry *parent, atomic64_t *value)
> > +{
> > +     debugfs_create_mode_unsafe(name, mode, parent, value,
> > +                                &fops_atomic64_t, &fops_atomic64_t_ro,
> > +                                &fops_atomic64_t_wo);
> > +}
> > +EXPORT_SYMBOL_GPL(debugfs_create_atomic64_t);
> > +
> >  ssize_t debugfs_read_file_bool(struct file *file, char __user *user_buf,
> >                              size_t count, loff_t *ppos)
> >  {
> > diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
> > index 851dd1f9a8a5..0fac84c53eab 100644
> > --- a/include/linux/debugfs.h
> > +++ b/include/linux/debugfs.h
> > @@ -126,6 +126,8 @@ void debugfs_create_size_t(const char *name, umode_t mode,
> >                          struct dentry *parent, size_t *value);
> >  void debugfs_create_atomic_t(const char *name, umode_t mode,
> >                            struct dentry *parent, atomic_t *value);
> > +void debugfs_create_atomic64_t(const char *name, umode_t mode,
> > +                                  struct dentry *parent, atomic64_t *value);
> >  struct dentry *debugfs_create_bool(const char *name, umode_t mode,
> >                                 struct dentry *parent, bool *value);
> >
> > @@ -291,6 +293,10 @@ static inline void debugfs_create_atomic_t(const char *name, umode_t mode,
> >                                          atomic_t *value)
> >  { }
> >
> > +static inline void debugfs_create_atomic64_t(const char *name, umode_t mode,
> > +                                          struct dentry *parent, atomic64_t *value)
> > +{ }
> > +
> >  static inline struct dentry *debugfs_create_bool(const char *name, umode_t mode,
> >                                                struct dentry *parent,
> >                                                bool *value)
>
> Looks good, but where is the user of this code?  I can't add new apis
> without a user.

Fair enough. Right now, the user is just some local
debugging/performance measuring which will never be upstreamed.
Happy to let this drop.

>
> And are you _SURE_ you want to be using an atomic64_t in the first
> place?  We are starting to reduce the "raw" usage of atomic variables as
> almost no one needs them, they should be using something else instead,
> or just a u64 as atomics are not needed for simple statistics.

I understand, and would generally never use atomics in real code. I
used an atomic since I wanted accuracy
(for some of the benchmarks I want to run) but can't use anything that
blocks (spinlock/mutex) since
the code is somewhere inside the scheduler.

>
> thanks,
>
> greg k-h

-- 
Anand K. Mistry
Software Engineer
Google Australia