Minor NFSD global DRC clean-ups

I'm working on a series of global DRC improvements. A few minor clean-ups are ready for merge now.

Chuck Lever (5):
NFSD: Fix off-by-one in DRC bucket pruning limit
NFSD: Eliminate percpu counter contention in DRC memory accounting
NFSD: Eliminate percpu counter contention in reply cache statistics
NFSD: Eliminate percpu counter contention in IO byte accounting
NFSD: Document reply_cache_stats ABI

 .../ABI/testing/procfs-nfsd-reply_cache_stats | 38 +++++++++
 fs/nfsd/nfscache.c                            |  2 +-
 fs/nfsd/stats.h                               | 78 ++++++++++++++++---
 3 files changed, 108 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/ABI/testing/procfs-nfsd-reply_cache_stats

2.54.0

nfsd_prune_bucket_locked() evicts an entry before checking the freed count against @max. The check uses "++freed > max", which does not break until freed exceeds max, resulting in max + 1 evictions. Use ">=" so the limit stated in the function comment is honored.

---
 fs/nfsd/nfscache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 07a53a5b3d37..c7db532c8523 100644
@@ -277,7 +277,7 @@ nfsd_prune_bucket_locked(struct nfsd_net *nn, struct nfsd_drc_bucket *b,
 		nfsd_cacherep_unlink_locked(nn, b, rp);
 		list_add(&rp->c_lru, dispose);
 
-		if (max && ++freed > max)
+		if (max && ++freed >= max)
 			break;
 	}
 }

2.54.0

The DRC memory usage counter (NFSD_STATS_DRC_MEM_USAGE) tracks bytes, but percpu_counter_add() uses the global percpu_counter_batch threshold of max(32, 2*nr_cpus). Each DRC entry add or removal updates the counter by sizeof(struct nfsd_cacherep) (~144 bytes), which always exceeds the batch threshold. percpu_counter_add() then acquires the counter's global spinlock on every update, serializing all nfsd threads.

On a 10-CPU NFS server handling a high rate of non-idempotent NFSv3 operations, this lock accounts for a measurable fraction of total spin lock overhead because nfsd_cache_lookup() both inserts a new entry and prunes up to three old entries per RPC, producing 4-7 global lock acquisitions per operation.

Switch to percpu_counter_add_local() and percpu_counter_sub_local(), which batch with INT_MAX so that updates always remain on the per-CPU fast path regardless of the amount. The only reader of this counter uses percpu_counter_sum_positive(), which sums the per-CPU deltas under the global lock, so read accuracy is unaffected.

---
 fs/nfsd/stats.h | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

index 87736b7fbf28..15d30c045dc3 100644
@@ -60,14 +60,32 @@ static inline void nfsd_stats_payload_misses_inc(struct nfsd_net *nn)
 	percpu_counter_inc(&nn->counter[NFSD_STATS_PAYLOAD_MISSES]);
 }
 
+/**
+ * nfsd_stats_drc_mem_usage_add - Add memory used by a cache item
+ * @nn: target network namespace
+ * @amount: byte count
+ *
+ * percpu_counter_add_local() keeps updates on the per-CPU fast
+ * path. The sole reader, percpu_counter_sum_positive(), sums the
+ * per-CPU deltas, so batching locally does not lose accuracy.
+ */
 static inline void nfsd_stats_drc_mem_usage_add(struct nfsd_net *nn, s64 amount)
 {
-	percpu_counter_add(&nn->counter[NFSD_STATS_DRC_MEM_USAGE], amount);
+	percpu_counter_add_local(&nn->counter[NFSD_STATS_DRC_MEM_USAGE],
+				 amount);
 }
 
+/**
+ * nfsd_stats_drc_mem_usage_sub - Subtract memory used by a cache item
+ * @nn: target network namespace
+ * @amount: byte count
+ *
+ * See nfsd_stats_drc_mem_usage_add() for batching rationale.
+ */
 static inline void nfsd_stats_drc_mem_usage_sub(struct nfsd_net *nn, s64 amount)
 {
-	percpu_counter_sub(&nn->counter[NFSD_STATS_DRC_MEM_USAGE], amount);
+	percpu_counter_sub_local(&nn->counter[NFSD_STATS_DRC_MEM_USAGE],
+				 amount);
 }
 
 #ifdef CONFIG_NFSD_V4

2.54.0

Dear god -- I had no idea that this was a thing. I wonder if we have other places we're using percpu_counter_add() where we shouldn't?

Jeff Layton <[email protected]>

Each RPC passes through nfsd_cache_lookup(), which increments one of nfsd_stats_rc_hits_inc(), nfsd_stats_rc_misses_inc(), or nfsd_stats_rc_nocache_inc(). These helpers update per-net-namespace percpu_counters with percpu_counter_inc(), which applies the default batch threshold of max(32, 2*nr_cpus).

Once a CPU's local delta reaches that threshold, the update folds into the shared counter under its global spinlock. On a busy multi-CPU server this produces lock traffic on a counter cacheline shared across all CPUs, growing with the request rate.

Switch to percpu_counter_add_local(fbc, 1), which batches with INT_MAX so that increments always remain on the per-CPU fast path. This matches the treatment already applied to the IO byte and DRC memory counters. All readers of these counters use percpu_counter_sum_positive(), which sums the per-CPU deltas under the global lock, so read accuracy is unaffected.

---
 fs/nfsd/stats.h | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

index 15d30c045dc3..598ea45d1722 100644
@@ -16,19 +16,39 @@
 struct proc_dir_entry *nfsd_proc_stat_init(struct net *net);
 void nfsd_proc_stat_shutdown(struct net *net);
 
+/**
+ * nfsd_stats_rc_hits_inc - Count a duplicate reply cache hit
+ * @nn: target network namespace
+ *
+ * These reply cache counters are updated once per RPC. Readers use
+ * percpu_counter_sum_positive(), so local batching does not affect
+ * read accuracy.
+ */
 static inline void nfsd_stats_rc_hits_inc(struct nfsd_net *nn)
 {
-	percpu_counter_inc(&nn->counter[NFSD_STATS_RC_HITS]);
+	percpu_counter_add_local(&nn->counter[NFSD_STATS_RC_HITS], 1);
 }
 
+/**
+ * nfsd_stats_rc_misses_inc - Count a duplicate reply cache miss
+ * @nn: target network namespace
+ *
+ * See nfsd_stats_rc_hits_inc() for batching rationale.
+ */
 static inline void nfsd_stats_rc_misses_inc(struct nfsd_net *nn)
 {
-	percpu_counter_inc(&nn->counter[NFSD_STATS_RC_MISSES]);
+	percpu_counter_add_local(&nn->counter[NFSD_STATS_RC_MISSES], 1);
 }
 
+/**
+ * nfsd_stats_rc_nocache_inc - Count a request not cached in the reply cache
+ * @nn: target network namespace
+ *
+ * See nfsd_stats_rc_hits_inc() for batching rationale.
+ */
 static inline void nfsd_stats_rc_nocache_inc(struct nfsd_net *nn)
 {
-	percpu_counter_inc(&nn->counter[NFSD_STATS_RC_NOCACHE]);
+	percpu_counter_add_local(&nn->counter[NFSD_STATS_RC_NOCACHE], 1);
 }
 
 static inline void nfsd_stats_fh_stale_inc(struct nfsd_net *nn,

2.54.0

nfsd_stats_io_write_add() and nfsd_stats_io_read_add() accumulate byte counts in per-net-namespace and per-export percpu_counters using percpu_counter_add(), which applies the default batch threshold of max(32, 2*nr_cpus).

For a 4 KB NFS WRITE, the amount (4096) always exceeds this threshold, so percpu_counter_add_batch() acquires the counter's global spinlock on every update. Each WRITE RPC updates two counters (per-net and per-export), producing two global lock acquisitions per operation. Profiling on a 10-CPU RDMA NFS server shows 0.44% of total CPU cycles spent contending on these locks during a small random write workload.

Switch to percpu_counter_add_local(), which batches with INT_MAX so that updates always remain on the per-CPU fast path regardless of the amount. All readers of these counters already use percpu_counter_sum_positive(), which sums the per-CPU deltas under the global lock, so read accuracy is unaffected.

---
 fs/nfsd/stats.h | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

index 598ea45d1722..c8d7e3934c33 100644
@@ -59,20 +59,42 @@ static inline void nfsd_stats_fh_stale_inc(struct nfsd_net *nn,
 		percpu_counter_inc(&exp->ex_stats->counter[EXP_STATS_FH_STALE]);
 }
 
+/**
+ * nfsd_stats_io_read_add - Count number of bytes for an NFS READ
+ * @nn: target network namespace
+ * @exp: target export
+ * @amount: byte count
+ *
+ * These counters are updated on every READ request. Readers use
+ * percpu_counter_sum_positive(), so local batching does not affect
+ * read accuracy.
+ */
 static inline void nfsd_stats_io_read_add(struct nfsd_net *nn,
 					  struct svc_export *exp, s64 amount)
 {
-	percpu_counter_add(&nn->counter[NFSD_STATS_IO_READ], amount);
+	percpu_counter_add_local(&nn->counter[NFSD_STATS_IO_READ], amount);
 	if (exp && exp->ex_stats)
-		percpu_counter_add(&exp->ex_stats->counter[EXP_STATS_IO_READ], amount);
+		percpu_counter_add_local(&exp->ex_stats->counter[EXP_STATS_IO_READ],
+					 amount);
 }
 
+/**
+ * nfsd_stats_io_write_add - Count number of bytes for an NFS WRITE
+ * @nn: target network namespace
+ * @exp: target export
+ * @amount: byte count
+ *
+ * These counters are updated on every WRITE request. Readers use
+ * percpu_counter_sum_positive(), so local batching does not affect
+ * read accuracy.
+ */
 static inline void nfsd_stats_io_write_add(struct nfsd_net *nn,
 					   struct svc_export *exp, s64 amount)
 {
-	percpu_counter_add(&nn->counter[NFSD_STATS_IO_WRITE], amount);
+	percpu_counter_add_local(&nn->counter[NFSD_STATS_IO_WRITE], amount);
 	if (exp && exp->ex_stats)
-		percpu_counter_add(&exp->ex_stats->counter[EXP_STATS_IO_WRITE], amount);
+		percpu_counter_add_local(&exp->ex_stats->counter[EXP_STATS_IO_WRITE],
+					 amount);
 }
 
 static inline void nfsd_stats_payload_misses_inc(struct nfsd_net *nn)

2.54.0

/proc/fs/nfsd/reply_cache_stats has been present since v3.10 but has no entry in Documentation/ABI/. Add one under testing/ that documents the current field set, types, and parsing expectations.

This establishes a contract that parsers should match on field name rather than line position, allowing fields to be added or removed across kernel versions without breaking well-written consumers.

---
 .../ABI/testing/procfs-nfsd-reply_cache_stats | 38 +++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 Documentation/ABI/testing/procfs-nfsd-reply_cache_stats

new file mode 100644
index 000000000000..57ed5f8e6597
@@ -0,0 +1,38 @@
+What:		/proc/fs/nfsd/reply_cache_stats
+Date:		March 2013
+KernelVersion:	3.10
+Contact:	[email protected]
+Description:
+		Provides statistics for the NFS server duplicate reply
+		cache (DRC). The file contains one labeled field per
+		line. Each line has the form "field name:" followed by
+		whitespace and a decimal value.
+
+		Fields:
+
+		=======================  ======  ==========================
+		max entries              u32     Upper bound on cache size
+		num entries              u32     Current entry count
+		hash buckets             u32     Number of hash buckets
+		mem usage                s64     Bytes consumed by the DRC
+		cache hits               s64     Requests answered from cache
+		cache misses             s64     Requests not found in cache
+		not cached               s64     Idempotent requests that
+		                                 bypass the cache
+		payload misses           s64     XID matched but request
+		                                 checksum did not
+		longest chain len        u32     Longest hash chain observed
+		cachesize at longest     u32     Cache size when longest
+		                                 chain was recorded
+		=======================  ======  ==========================
+
+		Counter fields (cache hits, cache misses, not cached,
+		payload misses, mem usage) are maintained with per-cpu
+		counters and may briefly show stale values under
+		concurrent load. There is no way to reset these
+		counters; consumers should compute rates by sampling
+		over time.
+
+		New fields may be appended in future kernels. Parsers
+		should match on field name, not line position.
+Users:		nfs-utils (https://git.linux-nfs.org/?p=steved/nfs-utils.git)

2.54.0

All looks very sensible to me.

Thanks,
NeilBrown

I had no idea that percpu counters could devolve into spinlocking in certain conditions. Ick. I wonder if we need to do a pass over all of our percpu counter usage for the same sort of thing. ISTM that we pretty much always want to use the *_local versions for our use-cases.

In any case, for this series: