Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210106075236.4184-1-amir73il@gmail.com> <20210106075236.4184-3-amir73il@gmail.com>
 <20210106205017.GG13116@fieldses.org>
In-Reply-To: <20210106205017.GG13116@fieldses.org>
From:   Amir Goldstein <amir73il@gmail.com>
Date:   Thu, 7 Jan 2021 09:17:42 +0200
Message-ID: <CAOQ4uxgh=f7fQ6Zf6ry8tC-WXMEoZzTo50y+K56paLTyadV7yw@mail.gmail.com>
Subject: Re: [PATCH v2 2/3] nfsd: protect concurrent access to nfsd stats counters
To:     "J . Bruce Fields" <bfields@fieldses.org>
Cc:     Chuck Lever <chuck.lever@oracle.com>,
        Jeff Layton <jlayton@poochiereds.net>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Jan 6, 2021 at 10:50 PM J . Bruce Fields <bfields@fieldses.org> wrote:
>
> These three patches seem fine.  To bikeshed just a bit more:
>
> On Wed, Jan 06, 2021 at 09:52:35AM +0200, Amir Goldstein wrote:
> >       /* We found a matching entry which is either in progress or done. */
> > -     nfsdstats.rchits++;
> > +     nfsd_stats_rc_hits_inc();
>
> Maybe make that something like
>
>         nfsd_stats_inc(NFSD_STATS_RC_HITS);
>
> and then we could avoid boilerplate like the below?
>

It looks like boilerplate, but every helper is a bit different,
so we would have to introduce several variants like:

         nfsd_net_stats_inc(nn, NFSD_NET_PAYLOAD_MISSES);

Which is the way I started, but realized it opens a big hole for
human mistakes in the form of:

         nfsd_net_stats_inc(nn, NFSD_STATS_RC_HITS);
         nfsd_stats_inc(NFSD_NET_PAYLOAD_MISSES);

And we have no way to detect those errors at compile time
because enum types are not strict types.

Also, in the next patch, three more helpers become unique
as they are made into helpers to account for both global and per-export
stats (fh_stale, io_read, io_write).
Making those helpers generic would require aligning the start of global
stats enum values with the per-export enum values and that is a source
of more human errors.

See the end result of stats.h. All the helpers are unique and the only ones
that remain generic are nfsd_stats_rc_*.
Making those generic would also require checking the range of the index
value is in the NFSD_STATS_RC_* range.

I also tried a version with boilerplate defining macros, i.e.:

NFSD_STATS_FUNCS(rc_hits, RC_HITS)
NFSD_STATS_FUNCS(fh_stale, FH_STALE)
NFSD_STATS_FUNCS(io_read, IO_READ)

But when I realized in the end it can only serve the rc_* counters,
I dropped it.

Thoughts?

Thanks,
Amir.


> --b.
>
> > +static inline void nfsd_stats_rc_hits_inc(void)
> > +{
> > +     percpu_counter_inc(&nfsdstats.counter[NFSD_STATS_RC_HITS]);
> > +}
> > +
> > +static inline void nfsd_stats_rc_misses_inc(void)
> > +{
> > +     percpu_counter_inc(&nfsdstats.counter[NFSD_STATS_RC_MISSES]);
> > +}
> > +
> > +static inline void nfsd_stats_rc_nocache_inc(void)
> > +{
> > +     percpu_counter_inc(&nfsdstats.counter[NFSD_STATS_RC_NOCACHE]);
> > +}
> > +
> > +static inline void nfsd_stats_fh_stale_inc(void)
> > +{
> > +     percpu_counter_inc(&nfsdstats.counter[NFSD_STATS_FH_STALE]);
> > +}
> > +
> > +static inline void nfsd_stats_io_read_add(s64 amount)
> > +{
> > +     percpu_counter_add(&nfsdstats.counter[NFSD_STATS_IO_READ], amount);
> > +}
> > +
> > +static inline void nfsd_stats_io_write_add(s64 amount)
> > +{
> > +     percpu_counter_add(&nfsdstats.counter[NFSD_STATS_IO_WRITE], amount);
> > +}
> > +
> > +static inline void nfsd_stats_payload_misses_inc(struct nfsd_net *nn)
> > +{
> > +     percpu_counter_inc(&nn->counter[NFSD_NET_PAYLOAD_MISSES]);
> > +}
> > +
> > +static inline void nfsd_stats_drc_mem_usage_add(struct nfsd_net *nn, s64 amount)
> > +{
> > +     percpu_counter_add(&nn->counter[NFSD_NET_DRC_MEM_USAGE], amount);
> > +}
> > +
> > +static inline void nfsd_stats_drc_mem_usage_sub(struct nfsd_net *nn, s64 amount)
> > +{
> > +     percpu_counter_sub(&nn->counter[NFSD_NET_DRC_MEM_USAGE], amount);
> > +}