ceph-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/4] ceph: forward average read/write/metadata latency
@ 2021-09-21 13:07 Venky Shankar
  2021-09-21 13:07 ` [PATCH v3 1/4] ceph: use "struct ceph_timespec" for r/w/m latencies Venky Shankar
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Venky Shankar @ 2021-09-21 13:07 UTC (permalink / raw)
  To: jlayton, pdonnell, xiubli; +Cc: ceph-devel, Venky Shankar

v3:
  - rework average/stdev handling by maintaining sum of squares
    and calculating standard deviation when sending metrics

Right now, cumulative read/write/metadata latencies are tracked
and are periodically forwarded to the MDS. These meterics are not
particularly useful. A much more useful metric is the average latency
and standard deviation (stdev) which is what this series of patches
aims to do.

The userspace (libcephfs+tool) changes are here::

          https://github.com/ceph/ceph/pull/41397

The math involved in keeping track of the average latency and stdev
are adjusted to closely mimic how its done in user space ceph (with
some restrictions obviously) as per::

          NEW_AVG = OLD_AVG + ((latency - OLD_AVG) / total_ops)
          NEW_STDEV = SQRT(SQ_SUM / (total_ops - 1))

Note that the cumulative latencies are still forwarded to the MDS but
the tool (cephfs-top) ignores it altogether.

Venky Shankar (4):
  ceph: use "struct ceph_timespec" for r/w/m latencies
  ceph: track average/stdev r/w/m latency
  ceph: include average/stddev r/w/m latency in mds metrics
  ceph: use tracked average r/w/m latencies to display metrics in
    debugfs

 fs/ceph/debugfs.c |   6 +--
 fs/ceph/metric.c  | 109 ++++++++++++++++++++++++----------------------
 fs/ceph/metric.h  |  62 ++++++++++++++++----------
 3 files changed, 101 insertions(+), 76 deletions(-)

-- 
2.31.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v3 1/4] ceph: use "struct ceph_timespec" for r/w/m latencies
  2021-09-21 13:07 [PATCH v3 0/4] ceph: forward average read/write/metadata latency Venky Shankar
@ 2021-09-21 13:07 ` Venky Shankar
  2021-09-21 13:07 ` [PATCH v3 2/4] ceph: track average/stdev r/w/m latency Venky Shankar
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 12+ messages in thread
From: Venky Shankar @ 2021-09-21 13:07 UTC (permalink / raw)
  To: jlayton, pdonnell, xiubli; +Cc: ceph-devel, Venky Shankar

Signed-off-by: Venky Shankar <vshankar@redhat.com>
---
 fs/ceph/metric.c | 12 ++++++------
 fs/ceph/metric.h | 11 ++++-------
 2 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/fs/ceph/metric.c b/fs/ceph/metric.c
index 04d5df29bbbf..226dc38e2909 100644
--- a/fs/ceph/metric.c
+++ b/fs/ceph/metric.c
@@ -64,8 +64,8 @@ static bool ceph_mdsc_send_metrics(struct ceph_mds_client *mdsc,
 	read->header.data_len = cpu_to_le32(sizeof(*read) - header_len);
 	sum = m->read_latency_sum;
 	jiffies_to_timespec64(sum, &ts);
-	read->sec = cpu_to_le32(ts.tv_sec);
-	read->nsec = cpu_to_le32(ts.tv_nsec);
+	read->lat.tv_sec = cpu_to_le32(ts.tv_sec);
+	read->lat.tv_nsec = cpu_to_le32(ts.tv_nsec);
 	items++;
 
 	/* encode the write latency metric */
@@ -76,8 +76,8 @@ static bool ceph_mdsc_send_metrics(struct ceph_mds_client *mdsc,
 	write->header.data_len = cpu_to_le32(sizeof(*write) - header_len);
 	sum = m->write_latency_sum;
 	jiffies_to_timespec64(sum, &ts);
-	write->sec = cpu_to_le32(ts.tv_sec);
-	write->nsec = cpu_to_le32(ts.tv_nsec);
+	write->lat.tv_sec = cpu_to_le32(ts.tv_sec);
+	write->lat.tv_nsec = cpu_to_le32(ts.tv_nsec);
 	items++;
 
 	/* encode the metadata latency metric */
@@ -88,8 +88,8 @@ static bool ceph_mdsc_send_metrics(struct ceph_mds_client *mdsc,
 	meta->header.data_len = cpu_to_le32(sizeof(*meta) - header_len);
 	sum = m->metadata_latency_sum;
 	jiffies_to_timespec64(sum, &ts);
-	meta->sec = cpu_to_le32(ts.tv_sec);
-	meta->nsec = cpu_to_le32(ts.tv_nsec);
+	meta->lat.tv_sec = cpu_to_le32(ts.tv_sec);
+	meta->lat.tv_nsec = cpu_to_le32(ts.tv_nsec);
 	items++;
 
 	/* encode the dentry lease metric */
diff --git a/fs/ceph/metric.h b/fs/ceph/metric.h
index 0133955a3c6a..103ed736f9d2 100644
--- a/fs/ceph/metric.h
+++ b/fs/ceph/metric.h
@@ -2,7 +2,7 @@
 #ifndef _FS_CEPH_MDS_METRIC_H
 #define _FS_CEPH_MDS_METRIC_H
 
-#include <linux/types.h>
+#include <linux/ceph/types.h>
 #include <linux/percpu_counter.h>
 #include <linux/ktime.h>
 
@@ -60,22 +60,19 @@ struct ceph_metric_cap {
 /* metric read latency header */
 struct ceph_metric_read_latency {
 	struct ceph_metric_header header;
-	__le32 sec;
-	__le32 nsec;
+	struct ceph_timespec lat;
 } __packed;
 
 /* metric write latency header */
 struct ceph_metric_write_latency {
 	struct ceph_metric_header header;
-	__le32 sec;
-	__le32 nsec;
+	struct ceph_timespec lat;
 } __packed;
 
 /* metric metadata latency header */
 struct ceph_metric_metadata_latency {
 	struct ceph_metric_header header;
-	__le32 sec;
-	__le32 nsec;
+	struct ceph_timespec lat;
 } __packed;
 
 /* metric dentry lease header */
-- 
2.31.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v3 2/4] ceph: track average/stdev r/w/m latency
  2021-09-21 13:07 [PATCH v3 0/4] ceph: forward average read/write/metadata latency Venky Shankar
  2021-09-21 13:07 ` [PATCH v3 1/4] ceph: use "struct ceph_timespec" for r/w/m latencies Venky Shankar
@ 2021-09-21 13:07 ` Venky Shankar
  2021-09-22 12:17   ` Jeff Layton
  2021-09-21 13:07 ` [PATCH v3 3/4] ceph: include average/stddev r/w/m latency in mds metrics Venky Shankar
  2021-09-21 13:07 ` [PATCH v3 4/4] ceph: use tracked average r/w/m latencies to display metrics in debugfs Venky Shankar
  3 siblings, 1 reply; 12+ messages in thread
From: Venky Shankar @ 2021-09-21 13:07 UTC (permalink / raw)
  To: jlayton, pdonnell, xiubli; +Cc: ceph-devel, Venky Shankar

Update the math involved to closely mimic how its done in
user land. This does not make a lot of difference to the
execution speed.

Signed-off-by: Venky Shankar <vshankar@redhat.com>
---
 fs/ceph/metric.c | 63 +++++++++++++++++++++---------------------------
 fs/ceph/metric.h |  3 +++
 2 files changed, 31 insertions(+), 35 deletions(-)

diff --git a/fs/ceph/metric.c b/fs/ceph/metric.c
index 226dc38e2909..ca758bff69ca 100644
--- a/fs/ceph/metric.c
+++ b/fs/ceph/metric.c
@@ -245,6 +245,7 @@ int ceph_metric_init(struct ceph_client_metric *m)
 
 	spin_lock_init(&m->read_metric_lock);
 	m->read_latency_sq_sum = 0;
+	m->avg_read_latency = 0;
 	m->read_latency_min = KTIME_MAX;
 	m->read_latency_max = 0;
 	m->total_reads = 0;
@@ -255,6 +256,7 @@ int ceph_metric_init(struct ceph_client_metric *m)
 
 	spin_lock_init(&m->write_metric_lock);
 	m->write_latency_sq_sum = 0;
+	m->avg_write_latency = 0;
 	m->write_latency_min = KTIME_MAX;
 	m->write_latency_max = 0;
 	m->total_writes = 0;
@@ -265,6 +267,7 @@ int ceph_metric_init(struct ceph_client_metric *m)
 
 	spin_lock_init(&m->metadata_metric_lock);
 	m->metadata_latency_sq_sum = 0;
+	m->avg_metadata_latency = 0;
 	m->metadata_latency_min = KTIME_MAX;
 	m->metadata_latency_max = 0;
 	m->total_metadatas = 0;
@@ -322,20 +325,25 @@ void ceph_metric_destroy(struct ceph_client_metric *m)
 		max = new;			\
 }
 
-static inline void __update_stdev(ktime_t total, ktime_t lsum,
-				  ktime_t *sq_sump, ktime_t lat)
+static inline void __update_latency(ktime_t *ctotal, ktime_t *lsum,
+				    ktime_t *lavg, ktime_t *min, ktime_t *max,
+				    ktime_t *sum_sq, ktime_t lat)
 {
-	ktime_t avg, sq;
+	ktime_t total, avg;
 
-	if (unlikely(total == 1))
-		return;
+	total = ++(*ctotal);
+	*lsum += lat;
+
+	METRIC_UPDATE_MIN_MAX(*min, *max, lat);
 
-	/* the sq is (lat - old_avg) * (lat - new_avg) */
-	avg = DIV64_U64_ROUND_CLOSEST((lsum - lat), (total - 1));
-	sq = lat - avg;
-	avg = DIV64_U64_ROUND_CLOSEST(lsum, total);
-	sq = sq * (lat - avg);
-	*sq_sump += sq;
+	if (unlikely(total == 1)) {
+		*lavg = lat;
+		*sum_sq = 0;
+	} else {
+		avg = *lavg + div64_s64(lat - *lavg, total);
+		*sum_sq += (lat - *lavg)*(lat - avg);
+		*lavg = avg;
+	}
 }
 
 void ceph_update_read_metrics(struct ceph_client_metric *m,
@@ -343,23 +351,18 @@ void ceph_update_read_metrics(struct ceph_client_metric *m,
 			      unsigned int size, int rc)
 {
 	ktime_t lat = ktime_sub(r_end, r_start);
-	ktime_t total;
 
 	if (unlikely(rc < 0 && rc != -ENOENT && rc != -ETIMEDOUT))
 		return;
 
 	spin_lock(&m->read_metric_lock);
-	total = ++m->total_reads;
 	m->read_size_sum += size;
-	m->read_latency_sum += lat;
 	METRIC_UPDATE_MIN_MAX(m->read_size_min,
 			      m->read_size_max,
 			      size);
-	METRIC_UPDATE_MIN_MAX(m->read_latency_min,
-			      m->read_latency_max,
-			      lat);
-	__update_stdev(total, m->read_latency_sum,
-		       &m->read_latency_sq_sum, lat);
+	__update_latency(&m->total_reads, &m->read_latency_sum,
+			 &m->avg_read_latency, &m->read_latency_min,
+			 &m->read_latency_max, &m->read_latency_sq_sum, lat);
 	spin_unlock(&m->read_metric_lock);
 }
 
@@ -368,23 +371,18 @@ void ceph_update_write_metrics(struct ceph_client_metric *m,
 			       unsigned int size, int rc)
 {
 	ktime_t lat = ktime_sub(r_end, r_start);
-	ktime_t total;
 
 	if (unlikely(rc && rc != -ETIMEDOUT))
 		return;
 
 	spin_lock(&m->write_metric_lock);
-	total = ++m->total_writes;
 	m->write_size_sum += size;
-	m->write_latency_sum += lat;
 	METRIC_UPDATE_MIN_MAX(m->write_size_min,
 			      m->write_size_max,
 			      size);
-	METRIC_UPDATE_MIN_MAX(m->write_latency_min,
-			      m->write_latency_max,
-			      lat);
-	__update_stdev(total, m->write_latency_sum,
-		       &m->write_latency_sq_sum, lat);
+	__update_latency(&m->total_writes, &m->write_latency_sum,
+			 &m->avg_write_latency, &m->write_latency_min,
+			 &m->write_latency_max, &m->write_latency_sq_sum, lat);
 	spin_unlock(&m->write_metric_lock);
 }
 
@@ -393,18 +391,13 @@ void ceph_update_metadata_metrics(struct ceph_client_metric *m,
 				  int rc)
 {
 	ktime_t lat = ktime_sub(r_end, r_start);
-	ktime_t total;
 
 	if (unlikely(rc && rc != -ENOENT))
 		return;
 
 	spin_lock(&m->metadata_metric_lock);
-	total = ++m->total_metadatas;
-	m->metadata_latency_sum += lat;
-	METRIC_UPDATE_MIN_MAX(m->metadata_latency_min,
-			      m->metadata_latency_max,
-			      lat);
-	__update_stdev(total, m->metadata_latency_sum,
-		       &m->metadata_latency_sq_sum, lat);
+	__update_latency(&m->total_metadatas, &m->metadata_latency_sum,
+			 &m->avg_metadata_latency, &m->metadata_latency_min,
+			 &m->metadata_latency_max, &m->metadata_latency_sq_sum, lat);
 	spin_unlock(&m->metadata_metric_lock);
 }
diff --git a/fs/ceph/metric.h b/fs/ceph/metric.h
index 103ed736f9d2..0af02e212033 100644
--- a/fs/ceph/metric.h
+++ b/fs/ceph/metric.h
@@ -138,6 +138,7 @@ struct ceph_client_metric {
 	u64 read_size_min;
 	u64 read_size_max;
 	ktime_t read_latency_sum;
+	ktime_t avg_read_latency;
 	ktime_t read_latency_sq_sum;
 	ktime_t read_latency_min;
 	ktime_t read_latency_max;
@@ -148,6 +149,7 @@ struct ceph_client_metric {
 	u64 write_size_min;
 	u64 write_size_max;
 	ktime_t write_latency_sum;
+	ktime_t avg_write_latency;
 	ktime_t write_latency_sq_sum;
 	ktime_t write_latency_min;
 	ktime_t write_latency_max;
@@ -155,6 +157,7 @@ struct ceph_client_metric {
 	spinlock_t metadata_metric_lock;
 	u64 total_metadatas;
 	ktime_t metadata_latency_sum;
+	ktime_t avg_metadata_latency;
 	ktime_t metadata_latency_sq_sum;
 	ktime_t metadata_latency_min;
 	ktime_t metadata_latency_max;
-- 
2.31.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v3 3/4] ceph: include average/stddev r/w/m latency in mds metrics
  2021-09-21 13:07 [PATCH v3 0/4] ceph: forward average read/write/metadata latency Venky Shankar
  2021-09-21 13:07 ` [PATCH v3 1/4] ceph: use "struct ceph_timespec" for r/w/m latencies Venky Shankar
  2021-09-21 13:07 ` [PATCH v3 2/4] ceph: track average/stdev r/w/m latency Venky Shankar
@ 2021-09-21 13:07 ` Venky Shankar
  2021-09-21 13:07 ` [PATCH v3 4/4] ceph: use tracked average r/w/m latencies to display metrics in debugfs Venky Shankar
  3 siblings, 0 replies; 12+ messages in thread
From: Venky Shankar @ 2021-09-21 13:07 UTC (permalink / raw)
  To: jlayton, pdonnell, xiubli; +Cc: ceph-devel, Venky Shankar

The use of `jiffies_to_timespec64()` seems incorrect too, switch
that to `ktime_to_timespec64()`.

Signed-off-by: Venky Shankar <vshankar@redhat.com>
---
 fs/ceph/metric.c | 46 ++++++++++++++++++++++++++++++----------------
 fs/ceph/metric.h | 48 +++++++++++++++++++++++++++++++++---------------
 2 files changed, 63 insertions(+), 31 deletions(-)

diff --git a/fs/ceph/metric.c b/fs/ceph/metric.c
index ca758bff69ca..04ba98286382 100644
--- a/fs/ceph/metric.c
+++ b/fs/ceph/metric.c
@@ -8,6 +8,20 @@
 #include "metric.h"
 #include "mds_client.h"
 
+static void to_ceph_timespec(struct ceph_timespec *ts, ktime_t val)
+{
+	struct timespec64 t = ktime_to_timespec64(val);
+	ts->tv_sec = cpu_to_le32(t.tv_sec);
+	ts->tv_nsec = cpu_to_le32(t.tv_nsec);
+}
+
+static ktime_t calc_stdev(ktime_t sq_sum, u64 total)
+{
+	if (total > 1)
+		return int_sqrt64(DIV64_U64_ROUND_CLOSEST(sq_sum, total - 1));
+	return 0;
+}
+
 static bool ceph_mdsc_send_metrics(struct ceph_mds_client *mdsc,
 				   struct ceph_mds_session *s)
 {
@@ -26,10 +40,10 @@ static bool ceph_mdsc_send_metrics(struct ceph_mds_client *mdsc,
 	u64 nr_caps = atomic64_read(&m->total_caps);
 	u32 header_len = sizeof(struct ceph_metric_header);
 	struct ceph_msg *msg;
-	struct timespec64 ts;
 	s64 sum;
 	s32 items = 0;
 	s32 len;
+        ktime_t stdev;
 
 	len = sizeof(*head) + sizeof(*cap) + sizeof(*read) + sizeof(*write)
 	      + sizeof(*meta) + sizeof(*dlease) + sizeof(*files)
@@ -59,37 +73,37 @@ static bool ceph_mdsc_send_metrics(struct ceph_mds_client *mdsc,
 	/* encode the read latency metric */
 	read = (struct ceph_metric_read_latency *)(cap + 1);
 	read->header.type = cpu_to_le32(CLIENT_METRIC_TYPE_READ_LATENCY);
-	read->header.ver = 1;
+	read->header.ver = 2;
 	read->header.compat = 1;
 	read->header.data_len = cpu_to_le32(sizeof(*read) - header_len);
-	sum = m->read_latency_sum;
-	jiffies_to_timespec64(sum, &ts);
-	read->lat.tv_sec = cpu_to_le32(ts.tv_sec);
-	read->lat.tv_nsec = cpu_to_le32(ts.tv_nsec);
+	to_ceph_timespec(&read->lat, m->read_latency_sum);
+	to_ceph_timespec(&read->avg, m->avg_read_latency);
+	stdev = calc_stdev(m->read_latency_sq_sum, m->total_reads);
+	to_ceph_timespec(&read->stdev, stdev);
 	items++;
 
 	/* encode the write latency metric */
 	write = (struct ceph_metric_write_latency *)(read + 1);
 	write->header.type = cpu_to_le32(CLIENT_METRIC_TYPE_WRITE_LATENCY);
-	write->header.ver = 1;
+	write->header.ver = 2;
 	write->header.compat = 1;
 	write->header.data_len = cpu_to_le32(sizeof(*write) - header_len);
-	sum = m->write_latency_sum;
-	jiffies_to_timespec64(sum, &ts);
-	write->lat.tv_sec = cpu_to_le32(ts.tv_sec);
-	write->lat.tv_nsec = cpu_to_le32(ts.tv_nsec);
+	to_ceph_timespec(&write->lat, m->write_latency_sum);
+	to_ceph_timespec(&write->avg, m->avg_write_latency);
+	stdev = calc_stdev(m->write_latency_sq_sum, m->total_writes);
+	to_ceph_timespec(&write->stdev, stdev);
 	items++;
 
 	/* encode the metadata latency metric */
 	meta = (struct ceph_metric_metadata_latency *)(write + 1);
 	meta->header.type = cpu_to_le32(CLIENT_METRIC_TYPE_METADATA_LATENCY);
-	meta->header.ver = 1;
+	meta->header.ver = 2;
 	meta->header.compat = 1;
 	meta->header.data_len = cpu_to_le32(sizeof(*meta) - header_len);
-	sum = m->metadata_latency_sum;
-	jiffies_to_timespec64(sum, &ts);
-	meta->lat.tv_sec = cpu_to_le32(ts.tv_sec);
-	meta->lat.tv_nsec = cpu_to_le32(ts.tv_nsec);
+	to_ceph_timespec(&meta->lat, m->metadata_latency_sum);
+	to_ceph_timespec(&meta->avg, m->avg_metadata_latency);
+	stdev = calc_stdev(m->metadata_latency_sq_sum, m->total_metadatas);
+	to_ceph_timespec(&meta->stdev, stdev);
 	items++;
 
 	/* encode the dentry lease metric */
diff --git a/fs/ceph/metric.h b/fs/ceph/metric.h
index 0af02e212033..028c5fbc6299 100644
--- a/fs/ceph/metric.h
+++ b/fs/ceph/metric.h
@@ -19,27 +19,39 @@ enum ceph_metric_type {
 	CLIENT_METRIC_TYPE_OPENED_INODES,
 	CLIENT_METRIC_TYPE_READ_IO_SIZES,
 	CLIENT_METRIC_TYPE_WRITE_IO_SIZES,
-
-	CLIENT_METRIC_TYPE_MAX = CLIENT_METRIC_TYPE_WRITE_IO_SIZES,
+	CLIENT_METRIC_TYPE_AVG_READ_LATENCY,
+	CLIENT_METRIC_TYPE_STDEV_READ_LATENCY,
+	CLIENT_METRIC_TYPE_AVG_WRITE_LATENCY,
+	CLIENT_METRIC_TYPE_STDEV_WRITE_LATENCY,
+	CLIENT_METRIC_TYPE_AVG_METADATA_LATENCY,
+	CLIENT_METRIC_TYPE_STDEV_METADATA_LATENCY,
+
+	CLIENT_METRIC_TYPE_MAX = CLIENT_METRIC_TYPE_STDEV_METADATA_LATENCY,
 };
 
 /*
  * This will always have the highest metric bit value
  * as the last element of the array.
  */
-#define CEPHFS_METRIC_SPEC_CLIENT_SUPPORTED {	\
-	CLIENT_METRIC_TYPE_CAP_INFO,		\
-	CLIENT_METRIC_TYPE_READ_LATENCY,	\
-	CLIENT_METRIC_TYPE_WRITE_LATENCY,	\
-	CLIENT_METRIC_TYPE_METADATA_LATENCY,	\
-	CLIENT_METRIC_TYPE_DENTRY_LEASE,	\
-	CLIENT_METRIC_TYPE_OPENED_FILES,	\
-	CLIENT_METRIC_TYPE_PINNED_ICAPS,	\
-	CLIENT_METRIC_TYPE_OPENED_INODES,	\
-	CLIENT_METRIC_TYPE_READ_IO_SIZES,	\
-	CLIENT_METRIC_TYPE_WRITE_IO_SIZES,	\
-						\
-	CLIENT_METRIC_TYPE_MAX,			\
+#define CEPHFS_METRIC_SPEC_CLIENT_SUPPORTED {	    \
+	CLIENT_METRIC_TYPE_CAP_INFO,		    \
+	CLIENT_METRIC_TYPE_READ_LATENCY,	    \
+	CLIENT_METRIC_TYPE_WRITE_LATENCY,	    \
+	CLIENT_METRIC_TYPE_METADATA_LATENCY,	    \
+	CLIENT_METRIC_TYPE_DENTRY_LEASE,	    \
+	CLIENT_METRIC_TYPE_OPENED_FILES,	    \
+	CLIENT_METRIC_TYPE_PINNED_ICAPS,	    \
+	CLIENT_METRIC_TYPE_OPENED_INODES,	    \
+	CLIENT_METRIC_TYPE_READ_IO_SIZES,	    \
+	CLIENT_METRIC_TYPE_WRITE_IO_SIZES,	    \
+	CLIENT_METRIC_TYPE_AVG_READ_LATENCY,	    \
+	CLIENT_METRIC_TYPE_STDEV_READ_LATENCY,	    \
+	CLIENT_METRIC_TYPE_AVG_WRITE_LATENCY,	    \
+	CLIENT_METRIC_TYPE_STDEV_WRITE_LATENCY,	    \
+	CLIENT_METRIC_TYPE_AVG_METADATA_LATENCY,    \
+	CLIENT_METRIC_TYPE_STDEV_METADATA_LATENCY,  \
+						    \
+	CLIENT_METRIC_TYPE_MAX,			    \
 }
 
 struct ceph_metric_header {
@@ -61,18 +73,24 @@ struct ceph_metric_cap {
 struct ceph_metric_read_latency {
 	struct ceph_metric_header header;
 	struct ceph_timespec lat;
+	struct ceph_timespec avg;
+	struct ceph_timespec stdev;
 } __packed;
 
 /* metric write latency header */
 struct ceph_metric_write_latency {
 	struct ceph_metric_header header;
 	struct ceph_timespec lat;
+	struct ceph_timespec avg;
+	struct ceph_timespec stdev;
 } __packed;
 
 /* metric metadata latency header */
 struct ceph_metric_metadata_latency {
 	struct ceph_metric_header header;
 	struct ceph_timespec lat;
+	struct ceph_timespec avg;
+	struct ceph_timespec stdev;
 } __packed;
 
 /* metric dentry lease header */
-- 
2.31.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v3 4/4] ceph: use tracked average r/w/m latencies to display metrics in debugfs
  2021-09-21 13:07 [PATCH v3 0/4] ceph: forward average read/write/metadata latency Venky Shankar
                   ` (2 preceding siblings ...)
  2021-09-21 13:07 ` [PATCH v3 3/4] ceph: include average/stddev r/w/m latency in mds metrics Venky Shankar
@ 2021-09-21 13:07 ` Venky Shankar
  3 siblings, 0 replies; 12+ messages in thread
From: Venky Shankar @ 2021-09-21 13:07 UTC (permalink / raw)
  To: jlayton, pdonnell, xiubli; +Cc: ceph-devel, Venky Shankar

Signed-off-by: Venky Shankar <vshankar@redhat.com>
---
 fs/ceph/debugfs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c
index 38b78b45811f..f6972853dc48 100644
--- a/fs/ceph/debugfs.c
+++ b/fs/ceph/debugfs.c
@@ -172,7 +172,7 @@ static int metric_show(struct seq_file *s, void *p)
 	spin_lock(&m->read_metric_lock);
 	total = m->total_reads;
 	sum = m->read_latency_sum;
-	avg = total > 0 ? DIV64_U64_ROUND_CLOSEST(sum, total) : 0;
+	avg = m->avg_read_latency;
 	min = m->read_latency_min;
 	max = m->read_latency_max;
 	sq = m->read_latency_sq_sum;
@@ -182,7 +182,7 @@ static int metric_show(struct seq_file *s, void *p)
 	spin_lock(&m->write_metric_lock);
 	total = m->total_writes;
 	sum = m->write_latency_sum;
-	avg = total > 0 ? DIV64_U64_ROUND_CLOSEST(sum, total) : 0;
+	avg = m->avg_write_latency;
 	min = m->write_latency_min;
 	max = m->write_latency_max;
 	sq = m->write_latency_sq_sum;
@@ -192,7 +192,7 @@ static int metric_show(struct seq_file *s, void *p)
 	spin_lock(&m->metadata_metric_lock);
 	total = m->total_metadatas;
 	sum = m->metadata_latency_sum;
-	avg = total > 0 ? DIV64_U64_ROUND_CLOSEST(sum, total) : 0;
+	avg = m->avg_metadata_latency;
 	min = m->metadata_latency_min;
 	max = m->metadata_latency_max;
 	sq = m->metadata_latency_sq_sum;
-- 
2.31.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 2/4] ceph: track average/stdev r/w/m latency
  2021-09-21 13:07 ` [PATCH v3 2/4] ceph: track average/stdev r/w/m latency Venky Shankar
@ 2021-09-22 12:17   ` Jeff Layton
  2021-09-22 12:33     ` Venky Shankar
  0 siblings, 1 reply; 12+ messages in thread
From: Jeff Layton @ 2021-09-22 12:17 UTC (permalink / raw)
  To: Venky Shankar, pdonnell, xiubli; +Cc: ceph-devel

On Tue, 2021-09-21 at 18:37 +0530, Venky Shankar wrote:
> Update the math involved to closely mimic how its done in
> user land. This does not make a lot of difference to the
> execution speed.
> 
> Signed-off-by: Venky Shankar <vshankar@redhat.com>
> ---
>  fs/ceph/metric.c | 63 +++++++++++++++++++++---------------------------
>  fs/ceph/metric.h |  3 +++
>  2 files changed, 31 insertions(+), 35 deletions(-)
> 
> diff --git a/fs/ceph/metric.c b/fs/ceph/metric.c
> index 226dc38e2909..ca758bff69ca 100644
> --- a/fs/ceph/metric.c
> +++ b/fs/ceph/metric.c
> @@ -245,6 +245,7 @@ int ceph_metric_init(struct ceph_client_metric *m)
>  
>  	spin_lock_init(&m->read_metric_lock);
>  	m->read_latency_sq_sum = 0;
> +	m->avg_read_latency = 0;
>  	m->read_latency_min = KTIME_MAX;
>  	m->read_latency_max = 0;
>  	m->total_reads = 0;
> @@ -255,6 +256,7 @@ int ceph_metric_init(struct ceph_client_metric *m)
>  
>  	spin_lock_init(&m->write_metric_lock);
>  	m->write_latency_sq_sum = 0;
> +	m->avg_write_latency = 0;
>  	m->write_latency_min = KTIME_MAX;
>  	m->write_latency_max = 0;
>  	m->total_writes = 0;
> @@ -265,6 +267,7 @@ int ceph_metric_init(struct ceph_client_metric *m)
>  
>  	spin_lock_init(&m->metadata_metric_lock);
>  	m->metadata_latency_sq_sum = 0;
> +	m->avg_metadata_latency = 0;
>  	m->metadata_latency_min = KTIME_MAX;
>  	m->metadata_latency_max = 0;
>  	m->total_metadatas = 0;
> @@ -322,20 +325,25 @@ void ceph_metric_destroy(struct ceph_client_metric *m)
>  		max = new;			\
>  }
>  
> -static inline void __update_stdev(ktime_t total, ktime_t lsum,
> -				  ktime_t *sq_sump, ktime_t lat)
> +static inline void __update_latency(ktime_t *ctotal, ktime_t *lsum,
> +				    ktime_t *lavg, ktime_t *min, ktime_t *max,
> +				    ktime_t *sum_sq, ktime_t lat)
>  {
> -	ktime_t avg, sq;
> +	ktime_t total, avg;
>  
> -	if (unlikely(total == 1))
> -		return;
> +	total = ++(*ctotal);
> +	*lsum += lat;
> +
> +	METRIC_UPDATE_MIN_MAX(*min, *max, lat);
>  
> -	/* the sq is (lat - old_avg) * (lat - new_avg) */
> -	avg = DIV64_U64_ROUND_CLOSEST((lsum - lat), (total - 1));
> -	sq = lat - avg;
> -	avg = DIV64_U64_ROUND_CLOSEST(lsum, total);
> -	sq = sq * (lat - avg);
> -	*sq_sump += sq;
> +	if (unlikely(total == 1)) {
> +		*lavg = lat;
> +		*sum_sq = 0;
> +	} else {
> +		avg = *lavg + div64_s64(lat - *lavg, total);
> +		*sum_sq += (lat - *lavg)*(lat - avg);
> +		*lavg = avg;
> +	}
>  }
>  
>  void ceph_update_read_metrics(struct ceph_client_metric *m,
> @@ -343,23 +351,18 @@ void ceph_update_read_metrics(struct ceph_client_metric *m,
>  			      unsigned int size, int rc)
>  {
>  	ktime_t lat = ktime_sub(r_end, r_start);
> -	ktime_t total;
>  
>  	if (unlikely(rc < 0 && rc != -ENOENT && rc != -ETIMEDOUT))
>  		return;
>  
>  	spin_lock(&m->read_metric_lock);
> -	total = ++m->total_reads;
>  	m->read_size_sum += size;
> -	m->read_latency_sum += lat;
>  	METRIC_UPDATE_MIN_MAX(m->read_size_min,
>  			      m->read_size_max,
>  			      size);
> -	METRIC_UPDATE_MIN_MAX(m->read_latency_min,
> -			      m->read_latency_max,
> -			      lat);
> -	__update_stdev(total, m->read_latency_sum,
> -		       &m->read_latency_sq_sum, lat);
> +	__update_latency(&m->total_reads, &m->read_latency_sum,
> +			 &m->avg_read_latency, &m->read_latency_min,
> +			 &m->read_latency_max, &m->read_latency_sq_sum, lat);

Do we really need to calculate the std deviation on every update? We
have to figure that in most cases, this stuff will be collected but only
seldom viewed.

ISTM that we ought to collect just the bare minimum of info on each
update, and save the more expensive calculations for the tool presenting
this info.

>  	spin_unlock(&m->read_metric_lock);
>  }
>  
> @@ -368,23 +371,18 @@ void ceph_update_write_metrics(struct ceph_client_metric *m,
>  			       unsigned int size, int rc)
>  {
>  	ktime_t lat = ktime_sub(r_end, r_start);
> -	ktime_t total;
>  
>  	if (unlikely(rc && rc != -ETIMEDOUT))
>  		return;
>  
>  	spin_lock(&m->write_metric_lock);
> -	total = ++m->total_writes;
>  	m->write_size_sum += size;
> -	m->write_latency_sum += lat;
>  	METRIC_UPDATE_MIN_MAX(m->write_size_min,
>  			      m->write_size_max,
>  			      size);
> -	METRIC_UPDATE_MIN_MAX(m->write_latency_min,
> -			      m->write_latency_max,
> -			      lat);
> -	__update_stdev(total, m->write_latency_sum,
> -		       &m->write_latency_sq_sum, lat);
> +	__update_latency(&m->total_writes, &m->write_latency_sum,
> +			 &m->avg_write_latency, &m->write_latency_min,
> +			 &m->write_latency_max, &m->write_latency_sq_sum, lat);
>  	spin_unlock(&m->write_metric_lock);
>  }
>  
> @@ -393,18 +391,13 @@ void ceph_update_metadata_metrics(struct ceph_client_metric *m,
>  				  int rc)
>  {
>  	ktime_t lat = ktime_sub(r_end, r_start);
> -	ktime_t total;
>  
>  	if (unlikely(rc && rc != -ENOENT))
>  		return;
>  
>  	spin_lock(&m->metadata_metric_lock);
> -	total = ++m->total_metadatas;
> -	m->metadata_latency_sum += lat;
> -	METRIC_UPDATE_MIN_MAX(m->metadata_latency_min,
> -			      m->metadata_latency_max,
> -			      lat);
> -	__update_stdev(total, m->metadata_latency_sum,
> -		       &m->metadata_latency_sq_sum, lat);
> +	__update_latency(&m->total_metadatas, &m->metadata_latency_sum,
> +			 &m->avg_metadata_latency, &m->metadata_latency_min,
> +			 &m->metadata_latency_max, &m->metadata_latency_sq_sum, lat);
>  	spin_unlock(&m->metadata_metric_lock);
>  }
> diff --git a/fs/ceph/metric.h b/fs/ceph/metric.h
> index 103ed736f9d2..0af02e212033 100644
> --- a/fs/ceph/metric.h
> +++ b/fs/ceph/metric.h
> @@ -138,6 +138,7 @@ struct ceph_client_metric {
>  	u64 read_size_min;
>  	u64 read_size_max;
>  	ktime_t read_latency_sum;
> +	ktime_t avg_read_latency;
>  	ktime_t read_latency_sq_sum;
>  	ktime_t read_latency_min;
>  	ktime_t read_latency_max;
> @@ -148,6 +149,7 @@ struct ceph_client_metric {
>  	u64 write_size_min;
>  	u64 write_size_max;
>  	ktime_t write_latency_sum;
> +	ktime_t avg_write_latency;
>  	ktime_t write_latency_sq_sum;
>  	ktime_t write_latency_min;
>  	ktime_t write_latency_max;
> @@ -155,6 +157,7 @@ struct ceph_client_metric {
>  	spinlock_t metadata_metric_lock;
>  	u64 total_metadatas;
>  	ktime_t metadata_latency_sum;
> +	ktime_t avg_metadata_latency;
>  	ktime_t metadata_latency_sq_sum;
>  	ktime_t metadata_latency_min;
>  	ktime_t metadata_latency_max;

-- 
Jeff Layton <jlayton@redhat.com>


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 2/4] ceph: track average/stdev r/w/m latency
  2021-09-22 12:17   ` Jeff Layton
@ 2021-09-22 12:33     ` Venky Shankar
  2021-09-22 13:44       ` Venky Shankar
  2021-09-22 13:45       ` Xiubo Li
  0 siblings, 2 replies; 12+ messages in thread
From: Venky Shankar @ 2021-09-22 12:33 UTC (permalink / raw)
  To: Jeff Layton; +Cc: Patrick Donnelly, Xiubo Li, ceph-devel

On Wed, Sep 22, 2021 at 5:47 PM Jeff Layton <jlayton@redhat.com> wrote:
>
> On Tue, 2021-09-21 at 18:37 +0530, Venky Shankar wrote:
> > Update the math involved to closely mimic how its done in
> > user land. This does not make a lot of difference to the
> > execution speed.
> >
> > Signed-off-by: Venky Shankar <vshankar@redhat.com>
> > ---
> >  fs/ceph/metric.c | 63 +++++++++++++++++++++---------------------------
> >  fs/ceph/metric.h |  3 +++
> >  2 files changed, 31 insertions(+), 35 deletions(-)
> >
> > diff --git a/fs/ceph/metric.c b/fs/ceph/metric.c
> > index 226dc38e2909..ca758bff69ca 100644
> > --- a/fs/ceph/metric.c
> > +++ b/fs/ceph/metric.c
> > @@ -245,6 +245,7 @@ int ceph_metric_init(struct ceph_client_metric *m)
> >
> >       spin_lock_init(&m->read_metric_lock);
> >       m->read_latency_sq_sum = 0;
> > +     m->avg_read_latency = 0;
> >       m->read_latency_min = KTIME_MAX;
> >       m->read_latency_max = 0;
> >       m->total_reads = 0;
> > @@ -255,6 +256,7 @@ int ceph_metric_init(struct ceph_client_metric *m)
> >
> >       spin_lock_init(&m->write_metric_lock);
> >       m->write_latency_sq_sum = 0;
> > +     m->avg_write_latency = 0;
> >       m->write_latency_min = KTIME_MAX;
> >       m->write_latency_max = 0;
> >       m->total_writes = 0;
> > @@ -265,6 +267,7 @@ int ceph_metric_init(struct ceph_client_metric *m)
> >
> >       spin_lock_init(&m->metadata_metric_lock);
> >       m->metadata_latency_sq_sum = 0;
> > +     m->avg_metadata_latency = 0;
> >       m->metadata_latency_min = KTIME_MAX;
> >       m->metadata_latency_max = 0;
> >       m->total_metadatas = 0;
> > @@ -322,20 +325,25 @@ void ceph_metric_destroy(struct ceph_client_metric *m)
> >               max = new;                      \
> >  }
> >
> > -static inline void __update_stdev(ktime_t total, ktime_t lsum,
> > -                               ktime_t *sq_sump, ktime_t lat)
> > +static inline void __update_latency(ktime_t *ctotal, ktime_t *lsum,
> > +                                 ktime_t *lavg, ktime_t *min, ktime_t *max,
> > +                                 ktime_t *sum_sq, ktime_t lat)
> >  {
> > -     ktime_t avg, sq;
> > +     ktime_t total, avg;
> >
> > -     if (unlikely(total == 1))
> > -             return;
> > +     total = ++(*ctotal);
> > +     *lsum += lat;
> > +
> > +     METRIC_UPDATE_MIN_MAX(*min, *max, lat);
> >
> > -     /* the sq is (lat - old_avg) * (lat - new_avg) */
> > -     avg = DIV64_U64_ROUND_CLOSEST((lsum - lat), (total - 1));
> > -     sq = lat - avg;
> > -     avg = DIV64_U64_ROUND_CLOSEST(lsum, total);
> > -     sq = sq * (lat - avg);
> > -     *sq_sump += sq;
> > +     if (unlikely(total == 1)) {
> > +             *lavg = lat;
> > +             *sum_sq = 0;
> > +     } else {
> > +             avg = *lavg + div64_s64(lat - *lavg, total);
> > +             *sum_sq += (lat - *lavg)*(lat - avg);
> > +             *lavg = avg;
> > +     }
> >  }
> >
> >  void ceph_update_read_metrics(struct ceph_client_metric *m,
> > @@ -343,23 +351,18 @@ void ceph_update_read_metrics(struct ceph_client_metric *m,
> >                             unsigned int size, int rc)
> >  {
> >       ktime_t lat = ktime_sub(r_end, r_start);
> > -     ktime_t total;
> >
> >       if (unlikely(rc < 0 && rc != -ENOENT && rc != -ETIMEDOUT))
> >               return;
> >
> >       spin_lock(&m->read_metric_lock);
> > -     total = ++m->total_reads;
> >       m->read_size_sum += size;
> > -     m->read_latency_sum += lat;
> >       METRIC_UPDATE_MIN_MAX(m->read_size_min,
> >                             m->read_size_max,
> >                             size);
> > -     METRIC_UPDATE_MIN_MAX(m->read_latency_min,
> > -                           m->read_latency_max,
> > -                           lat);
> > -     __update_stdev(total, m->read_latency_sum,
> > -                    &m->read_latency_sq_sum, lat);
> > +     __update_latency(&m->total_reads, &m->read_latency_sum,
> > +                      &m->avg_read_latency, &m->read_latency_min,
> > +                      &m->read_latency_max, &m->read_latency_sq_sum, lat);
>
> Do we really need to calculate the std deviation on every update? We
> have to figure that in most cases, this stuff will be collected but only
> seldom viewed.
>
> ISTM that we ought to collect just the bare minimum of info on each
> update, and save the more expensive calculations for the tool presenting
> this info.

Yeh, that's probably the plan we want going forward when introducing
new metrics.

FWIW, we could start doing it with this itself. It's just that the
user land PRs are approved and those do the way it is done here.

I'm ok with moving math crunching to the tool and it should not be a
major change to this patchset.

>
> >       spin_unlock(&m->read_metric_lock);
> >  }
> >
> > @@ -368,23 +371,18 @@ void ceph_update_write_metrics(struct ceph_client_metric *m,
> >                              unsigned int size, int rc)
> >  {
> >       ktime_t lat = ktime_sub(r_end, r_start);
> > -     ktime_t total;
> >
> >       if (unlikely(rc && rc != -ETIMEDOUT))
> >               return;
> >
> >       spin_lock(&m->write_metric_lock);
> > -     total = ++m->total_writes;
> >       m->write_size_sum += size;
> > -     m->write_latency_sum += lat;
> >       METRIC_UPDATE_MIN_MAX(m->write_size_min,
> >                             m->write_size_max,
> >                             size);
> > -     METRIC_UPDATE_MIN_MAX(m->write_latency_min,
> > -                           m->write_latency_max,
> > -                           lat);
> > -     __update_stdev(total, m->write_latency_sum,
> > -                    &m->write_latency_sq_sum, lat);
> > +     __update_latency(&m->total_writes, &m->write_latency_sum,
> > +                      &m->avg_write_latency, &m->write_latency_min,
> > +                      &m->write_latency_max, &m->write_latency_sq_sum, lat);
> >       spin_unlock(&m->write_metric_lock);
> >  }
> >
> > @@ -393,18 +391,13 @@ void ceph_update_metadata_metrics(struct ceph_client_metric *m,
> >                                 int rc)
> >  {
> >       ktime_t lat = ktime_sub(r_end, r_start);
> > -     ktime_t total;
> >
> >       if (unlikely(rc && rc != -ENOENT))
> >               return;
> >
> >       spin_lock(&m->metadata_metric_lock);
> > -     total = ++m->total_metadatas;
> > -     m->metadata_latency_sum += lat;
> > -     METRIC_UPDATE_MIN_MAX(m->metadata_latency_min,
> > -                           m->metadata_latency_max,
> > -                           lat);
> > -     __update_stdev(total, m->metadata_latency_sum,
> > -                    &m->metadata_latency_sq_sum, lat);
> > +     __update_latency(&m->total_metadatas, &m->metadata_latency_sum,
> > +                      &m->avg_metadata_latency, &m->metadata_latency_min,
> > +                      &m->metadata_latency_max, &m->metadata_latency_sq_sum, lat);
> >       spin_unlock(&m->metadata_metric_lock);
> >  }
> > diff --git a/fs/ceph/metric.h b/fs/ceph/metric.h
> > index 103ed736f9d2..0af02e212033 100644
> > --- a/fs/ceph/metric.h
> > +++ b/fs/ceph/metric.h
> > @@ -138,6 +138,7 @@ struct ceph_client_metric {
> >       u64 read_size_min;
> >       u64 read_size_max;
> >       ktime_t read_latency_sum;
> > +     ktime_t avg_read_latency;
> >       ktime_t read_latency_sq_sum;
> >       ktime_t read_latency_min;
> >       ktime_t read_latency_max;
> > @@ -148,6 +149,7 @@ struct ceph_client_metric {
> >       u64 write_size_min;
> >       u64 write_size_max;
> >       ktime_t write_latency_sum;
> > +     ktime_t avg_write_latency;
> >       ktime_t write_latency_sq_sum;
> >       ktime_t write_latency_min;
> >       ktime_t write_latency_max;
> > @@ -155,6 +157,7 @@ struct ceph_client_metric {
> >       spinlock_t metadata_metric_lock;
> >       u64 total_metadatas;
> >       ktime_t metadata_latency_sum;
> > +     ktime_t avg_metadata_latency;
> >       ktime_t metadata_latency_sq_sum;
> >       ktime_t metadata_latency_min;
> >       ktime_t metadata_latency_max;
>
> --
> Jeff Layton <jlayton@redhat.com>
>


-- 
Cheers,
Venky


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 2/4] ceph: track average/stdev r/w/m latency
  2021-09-22 12:33     ` Venky Shankar
@ 2021-09-22 13:44       ` Venky Shankar
  2021-09-22 14:34         ` Xiubo Li
  2021-09-22 13:45       ` Xiubo Li
  1 sibling, 1 reply; 12+ messages in thread
From: Venky Shankar @ 2021-09-22 13:44 UTC (permalink / raw)
  To: Jeff Layton; +Cc: Patrick Donnelly, Xiubo Li, ceph-devel

On Wed, Sep 22, 2021 at 6:03 PM Venky Shankar <vshankar@redhat.com> wrote:
>
> On Wed, Sep 22, 2021 at 5:47 PM Jeff Layton <jlayton@redhat.com> wrote:
> >
> > On Tue, 2021-09-21 at 18:37 +0530, Venky Shankar wrote:
> > > Update the math involved to closely mimic how its done in
> > > user land. This does not make a lot of difference to the
> > > execution speed.
> > >
> > > Signed-off-by: Venky Shankar <vshankar@redhat.com>
> > > ---
> > >  fs/ceph/metric.c | 63 +++++++++++++++++++++---------------------------
> > >  fs/ceph/metric.h |  3 +++
> > >  2 files changed, 31 insertions(+), 35 deletions(-)
> > >
> > > diff --git a/fs/ceph/metric.c b/fs/ceph/metric.c
> > > index 226dc38e2909..ca758bff69ca 100644
> > > --- a/fs/ceph/metric.c
> > > +++ b/fs/ceph/metric.c
> > > @@ -245,6 +245,7 @@ int ceph_metric_init(struct ceph_client_metric *m)
> > >
> > >       spin_lock_init(&m->read_metric_lock);
> > >       m->read_latency_sq_sum = 0;
> > > +     m->avg_read_latency = 0;
> > >       m->read_latency_min = KTIME_MAX;
> > >       m->read_latency_max = 0;
> > >       m->total_reads = 0;
> > > @@ -255,6 +256,7 @@ int ceph_metric_init(struct ceph_client_metric *m)
> > >
> > >       spin_lock_init(&m->write_metric_lock);
> > >       m->write_latency_sq_sum = 0;
> > > +     m->avg_write_latency = 0;
> > >       m->write_latency_min = KTIME_MAX;
> > >       m->write_latency_max = 0;
> > >       m->total_writes = 0;
> > > @@ -265,6 +267,7 @@ int ceph_metric_init(struct ceph_client_metric *m)
> > >
> > >       spin_lock_init(&m->metadata_metric_lock);
> > >       m->metadata_latency_sq_sum = 0;
> > > +     m->avg_metadata_latency = 0;
> > >       m->metadata_latency_min = KTIME_MAX;
> > >       m->metadata_latency_max = 0;
> > >       m->total_metadatas = 0;
> > > @@ -322,20 +325,25 @@ void ceph_metric_destroy(struct ceph_client_metric *m)
> > >               max = new;                      \
> > >  }
> > >
> > > -static inline void __update_stdev(ktime_t total, ktime_t lsum,
> > > -                               ktime_t *sq_sump, ktime_t lat)
> > > +static inline void __update_latency(ktime_t *ctotal, ktime_t *lsum,
> > > +                                 ktime_t *lavg, ktime_t *min, ktime_t *max,
> > > +                                 ktime_t *sum_sq, ktime_t lat)
> > >  {
> > > -     ktime_t avg, sq;
> > > +     ktime_t total, avg;
> > >
> > > -     if (unlikely(total == 1))
> > > -             return;
> > > +     total = ++(*ctotal);
> > > +     *lsum += lat;
> > > +
> > > +     METRIC_UPDATE_MIN_MAX(*min, *max, lat);
> > >
> > > -     /* the sq is (lat - old_avg) * (lat - new_avg) */
> > > -     avg = DIV64_U64_ROUND_CLOSEST((lsum - lat), (total - 1));
> > > -     sq = lat - avg;
> > > -     avg = DIV64_U64_ROUND_CLOSEST(lsum, total);
> > > -     sq = sq * (lat - avg);
> > > -     *sq_sump += sq;
> > > +     if (unlikely(total == 1)) {
> > > +             *lavg = lat;
> > > +             *sum_sq = 0;
> > > +     } else {
> > > +             avg = *lavg + div64_s64(lat - *lavg, total);
> > > +             *sum_sq += (lat - *lavg)*(lat - avg);
> > > +             *lavg = avg;
> > > +     }
> > >  }
> > >
> > >  void ceph_update_read_metrics(struct ceph_client_metric *m,
> > > @@ -343,23 +351,18 @@ void ceph_update_read_metrics(struct ceph_client_metric *m,
> > >                             unsigned int size, int rc)
> > >  {
> > >       ktime_t lat = ktime_sub(r_end, r_start);
> > > -     ktime_t total;
> > >
> > >       if (unlikely(rc < 0 && rc != -ENOENT && rc != -ETIMEDOUT))
> > >               return;
> > >
> > >       spin_lock(&m->read_metric_lock);
> > > -     total = ++m->total_reads;
> > >       m->read_size_sum += size;
> > > -     m->read_latency_sum += lat;
> > >       METRIC_UPDATE_MIN_MAX(m->read_size_min,
> > >                             m->read_size_max,
> > >                             size);
> > > -     METRIC_UPDATE_MIN_MAX(m->read_latency_min,
> > > -                           m->read_latency_max,
> > > -                           lat);
> > > -     __update_stdev(total, m->read_latency_sum,
> > > -                    &m->read_latency_sq_sum, lat);
> > > +     __update_latency(&m->total_reads, &m->read_latency_sum,
> > > +                      &m->avg_read_latency, &m->read_latency_min,
> > > +                      &m->read_latency_max, &m->read_latency_sq_sum, lat);
> >
> > Do we really need to calculate the std deviation on every update? We
> > have to figure that in most cases, this stuff will be collected but only
> > seldom viewed.
> >
> > ISTM that we ought to collect just the bare minimum of info on each
> > update, and save the more expensive calculations for the tool presenting
> > this info.
>
> Yeh, that's probably the plan we want going forward when introducing
> new metrics.
>
> FWIW, we could start doing it with this itself. It's just that the
> user land PRs are approved and those do the way it is done here.
>
> I'm ok with moving math crunching to the tool and it should not be a
> major change to this patchset.

So, I kind of recall why we did it this way -- metrics exchange
between MDS and ceph-mgr is restricted to std::pair<uint64_t,
uint64_t>. That would probably need to be expanded to carry variable
sized objects. I remember seeing a PR that does something like that.
Need to dig it up...

The other way would be to exchange the necessary information needed
for the calculation as a separate types but that's not really clean
and results in unnecessary bloating.

>
> >
> > >       spin_unlock(&m->read_metric_lock);
> > >  }
> > >
> > > @@ -368,23 +371,18 @@ void ceph_update_write_metrics(struct ceph_client_metric *m,
> > >                              unsigned int size, int rc)
> > >  {
> > >       ktime_t lat = ktime_sub(r_end, r_start);
> > > -     ktime_t total;
> > >
> > >       if (unlikely(rc && rc != -ETIMEDOUT))
> > >               return;
> > >
> > >       spin_lock(&m->write_metric_lock);
> > > -     total = ++m->total_writes;
> > >       m->write_size_sum += size;
> > > -     m->write_latency_sum += lat;
> > >       METRIC_UPDATE_MIN_MAX(m->write_size_min,
> > >                             m->write_size_max,
> > >                             size);
> > > -     METRIC_UPDATE_MIN_MAX(m->write_latency_min,
> > > -                           m->write_latency_max,
> > > -                           lat);
> > > -     __update_stdev(total, m->write_latency_sum,
> > > -                    &m->write_latency_sq_sum, lat);
> > > +     __update_latency(&m->total_writes, &m->write_latency_sum,
> > > +                      &m->avg_write_latency, &m->write_latency_min,
> > > +                      &m->write_latency_max, &m->write_latency_sq_sum, lat);
> > >       spin_unlock(&m->write_metric_lock);
> > >  }
> > >
> > > @@ -393,18 +391,13 @@ void ceph_update_metadata_metrics(struct ceph_client_metric *m,
> > >                                 int rc)
> > >  {
> > >       ktime_t lat = ktime_sub(r_end, r_start);
> > > -     ktime_t total;
> > >
> > >       if (unlikely(rc && rc != -ENOENT))
> > >               return;
> > >
> > >       spin_lock(&m->metadata_metric_lock);
> > > -     total = ++m->total_metadatas;
> > > -     m->metadata_latency_sum += lat;
> > > -     METRIC_UPDATE_MIN_MAX(m->metadata_latency_min,
> > > -                           m->metadata_latency_max,
> > > -                           lat);
> > > -     __update_stdev(total, m->metadata_latency_sum,
> > > -                    &m->metadata_latency_sq_sum, lat);
> > > +     __update_latency(&m->total_metadatas, &m->metadata_latency_sum,
> > > +                      &m->avg_metadata_latency, &m->metadata_latency_min,
> > > +                      &m->metadata_latency_max, &m->metadata_latency_sq_sum, lat);
> > >       spin_unlock(&m->metadata_metric_lock);
> > >  }
> > > diff --git a/fs/ceph/metric.h b/fs/ceph/metric.h
> > > index 103ed736f9d2..0af02e212033 100644
> > > --- a/fs/ceph/metric.h
> > > +++ b/fs/ceph/metric.h
> > > @@ -138,6 +138,7 @@ struct ceph_client_metric {
> > >       u64 read_size_min;
> > >       u64 read_size_max;
> > >       ktime_t read_latency_sum;
> > > +     ktime_t avg_read_latency;
> > >       ktime_t read_latency_sq_sum;
> > >       ktime_t read_latency_min;
> > >       ktime_t read_latency_max;
> > > @@ -148,6 +149,7 @@ struct ceph_client_metric {
> > >       u64 write_size_min;
> > >       u64 write_size_max;
> > >       ktime_t write_latency_sum;
> > > +     ktime_t avg_write_latency;
> > >       ktime_t write_latency_sq_sum;
> > >       ktime_t write_latency_min;
> > >       ktime_t write_latency_max;
> > > @@ -155,6 +157,7 @@ struct ceph_client_metric {
> > >       spinlock_t metadata_metric_lock;
> > >       u64 total_metadatas;
> > >       ktime_t metadata_latency_sum;
> > > +     ktime_t avg_metadata_latency;
> > >       ktime_t metadata_latency_sq_sum;
> > >       ktime_t metadata_latency_min;
> > >       ktime_t metadata_latency_max;
> >
> > --
> > Jeff Layton <jlayton@redhat.com>
> >
>
>
> --
> Cheers,
> Venky



-- 
Cheers,
Venky


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 2/4] ceph: track average/stdev r/w/m latency
  2021-09-22 12:33     ` Venky Shankar
  2021-09-22 13:44       ` Venky Shankar
@ 2021-09-22 13:45       ` Xiubo Li
  1 sibling, 0 replies; 12+ messages in thread
From: Xiubo Li @ 2021-09-22 13:45 UTC (permalink / raw)
  To: Venky Shankar, Jeff Layton; +Cc: Patrick Donnelly, ceph-devel


On 9/22/21 8:33 PM, Venky Shankar wrote:
> On Wed, Sep 22, 2021 at 5:47 PM Jeff Layton <jlayton@redhat.com> wrote:
>> On Tue, 2021-09-21 at 18:37 +0530, Venky Shankar wrote:
>>> Update the math involved to closely mimic how its done in
>>> user land. This does not make a lot of difference to the
>>> execution speed.
>>>
>>> Signed-off-by: Venky Shankar <vshankar@redhat.com>
>>> ---
>>>   fs/ceph/metric.c | 63 +++++++++++++++++++++---------------------------
>>>   fs/ceph/metric.h |  3 +++
>>>   2 files changed, 31 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/fs/ceph/metric.c b/fs/ceph/metric.c
>>> index 226dc38e2909..ca758bff69ca 100644
>>> --- a/fs/ceph/metric.c
>>> +++ b/fs/ceph/metric.c
>>> @@ -245,6 +245,7 @@ int ceph_metric_init(struct ceph_client_metric *m)
>>>
>>>        spin_lock_init(&m->read_metric_lock);
>>>        m->read_latency_sq_sum = 0;
>>> +     m->avg_read_latency = 0;
>>>        m->read_latency_min = KTIME_MAX;
>>>        m->read_latency_max = 0;
>>>        m->total_reads = 0;
>>> @@ -255,6 +256,7 @@ int ceph_metric_init(struct ceph_client_metric *m)
>>>
>>>        spin_lock_init(&m->write_metric_lock);
>>>        m->write_latency_sq_sum = 0;
>>> +     m->avg_write_latency = 0;
>>>        m->write_latency_min = KTIME_MAX;
>>>        m->write_latency_max = 0;
>>>        m->total_writes = 0;
>>> @@ -265,6 +267,7 @@ int ceph_metric_init(struct ceph_client_metric *m)
>>>
>>>        spin_lock_init(&m->metadata_metric_lock);
>>>        m->metadata_latency_sq_sum = 0;
>>> +     m->avg_metadata_latency = 0;
>>>        m->metadata_latency_min = KTIME_MAX;
>>>        m->metadata_latency_max = 0;
>>>        m->total_metadatas = 0;
>>> @@ -322,20 +325,25 @@ void ceph_metric_destroy(struct ceph_client_metric *m)
>>>                max = new;                      \
>>>   }
>>>
>>> -static inline void __update_stdev(ktime_t total, ktime_t lsum,
>>> -                               ktime_t *sq_sump, ktime_t lat)
>>> +static inline void __update_latency(ktime_t *ctotal, ktime_t *lsum,
>>> +                                 ktime_t *lavg, ktime_t *min, ktime_t *max,
>>> +                                 ktime_t *sum_sq, ktime_t lat)
>>>   {
>>> -     ktime_t avg, sq;
>>> +     ktime_t total, avg;
>>>
>>> -     if (unlikely(total == 1))
>>> -             return;
>>> +     total = ++(*ctotal);
>>> +     *lsum += lat;
>>> +
>>> +     METRIC_UPDATE_MIN_MAX(*min, *max, lat);
>>>
>>> -     /* the sq is (lat - old_avg) * (lat - new_avg) */
>>> -     avg = DIV64_U64_ROUND_CLOSEST((lsum - lat), (total - 1));
>>> -     sq = lat - avg;
>>> -     avg = DIV64_U64_ROUND_CLOSEST(lsum, total);
>>> -     sq = sq * (lat - avg);
>>> -     *sq_sump += sq;
>>> +     if (unlikely(total == 1)) {
>>> +             *lavg = lat;
>>> +             *sum_sq = 0;
>>> +     } else {
>>> +             avg = *lavg + div64_s64(lat - *lavg, total);
>>> +             *sum_sq += (lat - *lavg)*(lat - avg);
>>> +             *lavg = avg;
>>> +     }
>>>   }
>>>
>>>   void ceph_update_read_metrics(struct ceph_client_metric *m,
>>> @@ -343,23 +351,18 @@ void ceph_update_read_metrics(struct ceph_client_metric *m,
>>>                              unsigned int size, int rc)
>>>   {
>>>        ktime_t lat = ktime_sub(r_end, r_start);
>>> -     ktime_t total;
>>>
>>>        if (unlikely(rc < 0 && rc != -ENOENT && rc != -ETIMEDOUT))
>>>                return;
>>>
>>>        spin_lock(&m->read_metric_lock);
>>> -     total = ++m->total_reads;
>>>        m->read_size_sum += size;
>>> -     m->read_latency_sum += lat;
>>>        METRIC_UPDATE_MIN_MAX(m->read_size_min,
>>>                              m->read_size_max,
>>>                              size);
>>> -     METRIC_UPDATE_MIN_MAX(m->read_latency_min,
>>> -                           m->read_latency_max,
>>> -                           lat);
>>> -     __update_stdev(total, m->read_latency_sum,
>>> -                    &m->read_latency_sq_sum, lat);
>>> +     __update_latency(&m->total_reads, &m->read_latency_sum,
>>> +                      &m->avg_read_latency, &m->read_latency_min,
>>> +                      &m->read_latency_max, &m->read_latency_sq_sum, lat);
>> Do we really need to calculate the std deviation on every update? We
>> have to figure that in most cases, this stuff will be collected but only
>> seldom viewed.
>>
>> ISTM that we ought to collect just the bare minimum of info on each
>> update, and save the more expensive calculations for the tool presenting
>> this info.
> Yeh, that's probably the plan we want going forward when introducing
> new metrics.
>
> FWIW, we could start doing it with this itself. It's just that the
> user land PRs are approved and those do the way it is done here.
>
> I'm ok with moving math crunching to the tool and it should not be a
> major change to this patchset.

Yeah, I am also prefer to do this in the tool instead of here on each 
update.


>
>>>        spin_unlock(&m->read_metric_lock);
>>>   }
>>>
>>> @@ -368,23 +371,18 @@ void ceph_update_write_metrics(struct ceph_client_metric *m,
>>>                               unsigned int size, int rc)
>>>   {
>>>        ktime_t lat = ktime_sub(r_end, r_start);
>>> -     ktime_t total;
>>>
>>>        if (unlikely(rc && rc != -ETIMEDOUT))
>>>                return;
>>>
>>>        spin_lock(&m->write_metric_lock);
>>> -     total = ++m->total_writes;
>>>        m->write_size_sum += size;
>>> -     m->write_latency_sum += lat;
>>>        METRIC_UPDATE_MIN_MAX(m->write_size_min,
>>>                              m->write_size_max,
>>>                              size);
>>> -     METRIC_UPDATE_MIN_MAX(m->write_latency_min,
>>> -                           m->write_latency_max,
>>> -                           lat);
>>> -     __update_stdev(total, m->write_latency_sum,
>>> -                    &m->write_latency_sq_sum, lat);
>>> +     __update_latency(&m->total_writes, &m->write_latency_sum,
>>> +                      &m->avg_write_latency, &m->write_latency_min,
>>> +                      &m->write_latency_max, &m->write_latency_sq_sum, lat);
>>>        spin_unlock(&m->write_metric_lock);
>>>   }
>>>
>>> @@ -393,18 +391,13 @@ void ceph_update_metadata_metrics(struct ceph_client_metric *m,
>>>                                  int rc)
>>>   {
>>>        ktime_t lat = ktime_sub(r_end, r_start);
>>> -     ktime_t total;
>>>
>>>        if (unlikely(rc && rc != -ENOENT))
>>>                return;
>>>
>>>        spin_lock(&m->metadata_metric_lock);
>>> -     total = ++m->total_metadatas;
>>> -     m->metadata_latency_sum += lat;
>>> -     METRIC_UPDATE_MIN_MAX(m->metadata_latency_min,
>>> -                           m->metadata_latency_max,
>>> -                           lat);
>>> -     __update_stdev(total, m->metadata_latency_sum,
>>> -                    &m->metadata_latency_sq_sum, lat);
>>> +     __update_latency(&m->total_metadatas, &m->metadata_latency_sum,
>>> +                      &m->avg_metadata_latency, &m->metadata_latency_min,
>>> +                      &m->metadata_latency_max, &m->metadata_latency_sq_sum, lat);
>>>        spin_unlock(&m->metadata_metric_lock);
>>>   }
>>> diff --git a/fs/ceph/metric.h b/fs/ceph/metric.h
>>> index 103ed736f9d2..0af02e212033 100644
>>> --- a/fs/ceph/metric.h
>>> +++ b/fs/ceph/metric.h
>>> @@ -138,6 +138,7 @@ struct ceph_client_metric {
>>>        u64 read_size_min;
>>>        u64 read_size_max;
>>>        ktime_t read_latency_sum;
>>> +     ktime_t avg_read_latency;
>>>        ktime_t read_latency_sq_sum;
>>>        ktime_t read_latency_min;
>>>        ktime_t read_latency_max;
>>> @@ -148,6 +149,7 @@ struct ceph_client_metric {
>>>        u64 write_size_min;
>>>        u64 write_size_max;
>>>        ktime_t write_latency_sum;
>>> +     ktime_t avg_write_latency;
>>>        ktime_t write_latency_sq_sum;
>>>        ktime_t write_latency_min;
>>>        ktime_t write_latency_max;
>>> @@ -155,6 +157,7 @@ struct ceph_client_metric {
>>>        spinlock_t metadata_metric_lock;
>>>        u64 total_metadatas;
>>>        ktime_t metadata_latency_sum;
>>> +     ktime_t avg_metadata_latency;
>>>        ktime_t metadata_latency_sq_sum;
>>>        ktime_t metadata_latency_min;
>>>        ktime_t metadata_latency_max;
>> --
>> Jeff Layton <jlayton@redhat.com>
>>
>


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 2/4] ceph: track average/stdev r/w/m latency
  2021-09-22 13:44       ` Venky Shankar
@ 2021-09-22 14:34         ` Xiubo Li
  2021-09-22 14:37           ` Xiubo Li
  2021-09-22 17:10           ` Venky Shankar
  0 siblings, 2 replies; 12+ messages in thread
From: Xiubo Li @ 2021-09-22 14:34 UTC (permalink / raw)
  To: Venky Shankar, Jeff Layton; +Cc: Patrick Donnelly, ceph-devel


On 9/22/21 9:44 PM, Venky Shankar wrote:
> On Wed, Sep 22, 2021 at 6:03 PM Venky Shankar <vshankar@redhat.com> wrote:
>> On Wed, Sep 22, 2021 at 5:47 PM Jeff Layton <jlayton@redhat.com> wrote:
>>> On Tue, 2021-09-21 at 18:37 +0530, Venky Shankar wrote:
>>>> Update the math involved to closely mimic how its done in
>>>> user land. This does not make a lot of difference to the
>>>> execution speed.
>>>>
>>>> Signed-off-by: Venky Shankar <vshankar@redhat.com>
>>>> ---
>>>>   fs/ceph/metric.c | 63 +++++++++++++++++++++---------------------------
>>>>   fs/ceph/metric.h |  3 +++
>>>>   2 files changed, 31 insertions(+), 35 deletions(-)
>>>>
>>>> diff --git a/fs/ceph/metric.c b/fs/ceph/metric.c
>>>> index 226dc38e2909..ca758bff69ca 100644
>>>> --- a/fs/ceph/metric.c
>>>> +++ b/fs/ceph/metric.c
>>>> @@ -245,6 +245,7 @@ int ceph_metric_init(struct ceph_client_metric *m)
>>>>
>>>>        spin_lock_init(&m->read_metric_lock);
>>>>        m->read_latency_sq_sum = 0;
>>>> +     m->avg_read_latency = 0;
>>>>        m->read_latency_min = KTIME_MAX;
>>>>        m->read_latency_max = 0;
>>>>        m->total_reads = 0;
>>>> @@ -255,6 +256,7 @@ int ceph_metric_init(struct ceph_client_metric *m)
>>>>
>>>>        spin_lock_init(&m->write_metric_lock);
>>>>        m->write_latency_sq_sum = 0;
>>>> +     m->avg_write_latency = 0;
>>>>        m->write_latency_min = KTIME_MAX;
>>>>        m->write_latency_max = 0;
>>>>        m->total_writes = 0;
>>>> @@ -265,6 +267,7 @@ int ceph_metric_init(struct ceph_client_metric *m)
>>>>
>>>>        spin_lock_init(&m->metadata_metric_lock);
>>>>        m->metadata_latency_sq_sum = 0;
>>>> +     m->avg_metadata_latency = 0;
>>>>        m->metadata_latency_min = KTIME_MAX;
>>>>        m->metadata_latency_max = 0;
>>>>        m->total_metadatas = 0;
>>>> @@ -322,20 +325,25 @@ void ceph_metric_destroy(struct ceph_client_metric *m)
>>>>                max = new;                      \
>>>>   }
>>>>
>>>> -static inline void __update_stdev(ktime_t total, ktime_t lsum,
>>>> -                               ktime_t *sq_sump, ktime_t lat)
>>>> +static inline void __update_latency(ktime_t *ctotal, ktime_t *lsum,
>>>> +                                 ktime_t *lavg, ktime_t *min, ktime_t *max,
>>>> +                                 ktime_t *sum_sq, ktime_t lat)
>>>>   {
>>>> -     ktime_t avg, sq;
>>>> +     ktime_t total, avg;
>>>>
>>>> -     if (unlikely(total == 1))
>>>> -             return;
>>>> +     total = ++(*ctotal);
>>>> +     *lsum += lat;
>>>> +
>>>> +     METRIC_UPDATE_MIN_MAX(*min, *max, lat);
>>>>
>>>> -     /* the sq is (lat - old_avg) * (lat - new_avg) */
>>>> -     avg = DIV64_U64_ROUND_CLOSEST((lsum - lat), (total - 1));
>>>> -     sq = lat - avg;
>>>> -     avg = DIV64_U64_ROUND_CLOSEST(lsum, total);
>>>> -     sq = sq * (lat - avg);
>>>> -     *sq_sump += sq;
>>>> +     if (unlikely(total == 1)) {
>>>> +             *lavg = lat;
>>>> +             *sum_sq = 0;
>>>> +     } else {
>>>> +             avg = *lavg + div64_s64(lat - *lavg, total);
>>>> +             *sum_sq += (lat - *lavg)*(lat - avg);
>>>> +             *lavg = avg;
>>>> +     }
>>>>   }
>>>>
>>>>   void ceph_update_read_metrics(struct ceph_client_metric *m,
>>>> @@ -343,23 +351,18 @@ void ceph_update_read_metrics(struct ceph_client_metric *m,
>>>>                              unsigned int size, int rc)
>>>>   {
>>>>        ktime_t lat = ktime_sub(r_end, r_start);
>>>> -     ktime_t total;
>>>>
>>>>        if (unlikely(rc < 0 && rc != -ENOENT && rc != -ETIMEDOUT))
>>>>                return;
>>>>
>>>>        spin_lock(&m->read_metric_lock);
>>>> -     total = ++m->total_reads;
>>>>        m->read_size_sum += size;
>>>> -     m->read_latency_sum += lat;
>>>>        METRIC_UPDATE_MIN_MAX(m->read_size_min,
>>>>                              m->read_size_max,
>>>>                              size);
>>>> -     METRIC_UPDATE_MIN_MAX(m->read_latency_min,
>>>> -                           m->read_latency_max,
>>>> -                           lat);
>>>> -     __update_stdev(total, m->read_latency_sum,
>>>> -                    &m->read_latency_sq_sum, lat);
>>>> +     __update_latency(&m->total_reads, &m->read_latency_sum,
>>>> +                      &m->avg_read_latency, &m->read_latency_min,
>>>> +                      &m->read_latency_max, &m->read_latency_sq_sum, lat);
>>> Do we really need to calculate the std deviation on every update? We
>>> have to figure that in most cases, this stuff will be collected but only
>>> seldom viewed.
>>>
>>> ISTM that we ought to collect just the bare minimum of info on each
>>> update, and save the more expensive calculations for the tool presenting
>>> this info.
>> Yeh, that's probably the plan we want going forward when introducing
>> new metrics.
>>
>> FWIW, we could start doing it with this itself. It's just that the
>> user land PRs are approved and those do the way it is done here.
>>
>> I'm ok with moving math crunching to the tool and it should not be a
>> major change to this patchset.
> So, I kind of recall why we did it this way -- metrics exchange
> between MDS and ceph-mgr is restricted to std::pair<uint64_t,
> uint64_t>.

Before I pushed one patch to improve this and switched it to a list or 
something else, but revert it dues to some reason.


> That would probably need to be expanded to carry variable
> sized objects. I remember seeing a PR that does something like that.
> Need to dig it up...
>
> The other way would be to exchange the necessary information needed
> for the calculation as a separate types but that's not really clean
> and results in unnecessary bloating.
>
>>>>        spin_unlock(&m->read_metric_lock);
>>>>   }
>>>>
>>>> @@ -368,23 +371,18 @@ void ceph_update_write_metrics(struct ceph_client_metric *m,
>>>>                               unsigned int size, int rc)
>>>>   {
>>>>        ktime_t lat = ktime_sub(r_end, r_start);
>>>> -     ktime_t total;
>>>>
>>>>        if (unlikely(rc && rc != -ETIMEDOUT))
>>>>                return;
>>>>
>>>>        spin_lock(&m->write_metric_lock);
>>>> -     total = ++m->total_writes;
>>>>        m->write_size_sum += size;
>>>> -     m->write_latency_sum += lat;
>>>>        METRIC_UPDATE_MIN_MAX(m->write_size_min,
>>>>                              m->write_size_max,
>>>>                              size);
>>>> -     METRIC_UPDATE_MIN_MAX(m->write_latency_min,
>>>> -                           m->write_latency_max,
>>>> -                           lat);
>>>> -     __update_stdev(total, m->write_latency_sum,
>>>> -                    &m->write_latency_sq_sum, lat);
>>>> +     __update_latency(&m->total_writes, &m->write_latency_sum,
>>>> +                      &m->avg_write_latency, &m->write_latency_min,
>>>> +                      &m->write_latency_max, &m->write_latency_sq_sum, lat);
>>>>        spin_unlock(&m->write_metric_lock);
>>>>   }
>>>>
>>>> @@ -393,18 +391,13 @@ void ceph_update_metadata_metrics(struct ceph_client_metric *m,
>>>>                                  int rc)
>>>>   {
>>>>        ktime_t lat = ktime_sub(r_end, r_start);
>>>> -     ktime_t total;
>>>>
>>>>        if (unlikely(rc && rc != -ENOENT))
>>>>                return;
>>>>
>>>>        spin_lock(&m->metadata_metric_lock);
>>>> -     total = ++m->total_metadatas;
>>>> -     m->metadata_latency_sum += lat;
>>>> -     METRIC_UPDATE_MIN_MAX(m->metadata_latency_min,
>>>> -                           m->metadata_latency_max,
>>>> -                           lat);
>>>> -     __update_stdev(total, m->metadata_latency_sum,
>>>> -                    &m->metadata_latency_sq_sum, lat);
>>>> +     __update_latency(&m->total_metadatas, &m->metadata_latency_sum,
>>>> +                      &m->avg_metadata_latency, &m->metadata_latency_min,
>>>> +                      &m->metadata_latency_max, &m->metadata_latency_sq_sum, lat);
>>>>        spin_unlock(&m->metadata_metric_lock);
>>>>   }
>>>> diff --git a/fs/ceph/metric.h b/fs/ceph/metric.h
>>>> index 103ed736f9d2..0af02e212033 100644
>>>> --- a/fs/ceph/metric.h
>>>> +++ b/fs/ceph/metric.h
>>>> @@ -138,6 +138,7 @@ struct ceph_client_metric {
>>>>        u64 read_size_min;
>>>>        u64 read_size_max;
>>>>        ktime_t read_latency_sum;
>>>> +     ktime_t avg_read_latency;
>>>>        ktime_t read_latency_sq_sum;
>>>>        ktime_t read_latency_min;
>>>>        ktime_t read_latency_max;
>>>> @@ -148,6 +149,7 @@ struct ceph_client_metric {
>>>>        u64 write_size_min;
>>>>        u64 write_size_max;
>>>>        ktime_t write_latency_sum;
>>>> +     ktime_t avg_write_latency;
>>>>        ktime_t write_latency_sq_sum;
>>>>        ktime_t write_latency_min;
>>>>        ktime_t write_latency_max;
>>>> @@ -155,6 +157,7 @@ struct ceph_client_metric {
>>>>        spinlock_t metadata_metric_lock;
>>>>        u64 total_metadatas;
>>>>        ktime_t metadata_latency_sum;
>>>> +     ktime_t avg_metadata_latency;
>>>>        ktime_t metadata_latency_sq_sum;
>>>>        ktime_t metadata_latency_min;
>>>>        ktime_t metadata_latency_max;
>>> --
>>> Jeff Layton <jlayton@redhat.com>
>>>
>>
>> --
>> Cheers,
>> Venky
>
>


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 2/4] ceph: track average/stdev r/w/m latency
  2021-09-22 14:34         ` Xiubo Li
@ 2021-09-22 14:37           ` Xiubo Li
  2021-09-22 17:10           ` Venky Shankar
  1 sibling, 0 replies; 12+ messages in thread
From: Xiubo Li @ 2021-09-22 14:37 UTC (permalink / raw)
  To: Venky Shankar, Jeff Layton; +Cc: Patrick Donnelly, ceph-devel


On 9/22/21 10:34 PM, Xiubo Li wrote:
>
> On 9/22/21 9:44 PM, Venky Shankar wrote:
>> On Wed, Sep 22, 2021 at 6:03 PM Venky Shankar <vshankar@redhat.com> 
>> wrote:
>>> On Wed, Sep 22, 2021 at 5:47 PM Jeff Layton <jlayton@redhat.com> wrote:
>>>> On Tue, 2021-09-21 at 18:37 +0530, Venky Shankar wrote:
>>>>> Update the math involved to closely mimic how its done in
>>>>> user land. This does not make a lot of difference to the
>>>>> execution speed.
>>>>>
>>>>> Signed-off-by: Venky Shankar <vshankar@redhat.com>
>>>>> ---
>>>>>   fs/ceph/metric.c | 63 
>>>>> +++++++++++++++++++++---------------------------
>>>>>   fs/ceph/metric.h |  3 +++
>>>>>   2 files changed, 31 insertions(+), 35 deletions(-)
>>>>>
>>>>> diff --git a/fs/ceph/metric.c b/fs/ceph/metric.c
>>>>> index 226dc38e2909..ca758bff69ca 100644
>>>>> --- a/fs/ceph/metric.c
>>>>> +++ b/fs/ceph/metric.c
>>>>> @@ -245,6 +245,7 @@ int ceph_metric_init(struct ceph_client_metric 
>>>>> *m)
>>>>>
>>>>>        spin_lock_init(&m->read_metric_lock);
>>>>>        m->read_latency_sq_sum = 0;
>>>>> +     m->avg_read_latency = 0;
>>>>>        m->read_latency_min = KTIME_MAX;
>>>>>        m->read_latency_max = 0;
>>>>>        m->total_reads = 0;
>>>>> @@ -255,6 +256,7 @@ int ceph_metric_init(struct ceph_client_metric 
>>>>> *m)
>>>>>
>>>>>        spin_lock_init(&m->write_metric_lock);
>>>>>        m->write_latency_sq_sum = 0;
>>>>> +     m->avg_write_latency = 0;
>>>>>        m->write_latency_min = KTIME_MAX;
>>>>>        m->write_latency_max = 0;
>>>>>        m->total_writes = 0;
>>>>> @@ -265,6 +267,7 @@ int ceph_metric_init(struct ceph_client_metric 
>>>>> *m)
>>>>>
>>>>>        spin_lock_init(&m->metadata_metric_lock);
>>>>>        m->metadata_latency_sq_sum = 0;
>>>>> +     m->avg_metadata_latency = 0;
>>>>>        m->metadata_latency_min = KTIME_MAX;
>>>>>        m->metadata_latency_max = 0;
>>>>>        m->total_metadatas = 0;
>>>>> @@ -322,20 +325,25 @@ void ceph_metric_destroy(struct 
>>>>> ceph_client_metric *m)
>>>>>                max = new;                      \
>>>>>   }
>>>>>
>>>>> -static inline void __update_stdev(ktime_t total, ktime_t lsum,
>>>>> -                               ktime_t *sq_sump, ktime_t lat)
>>>>> +static inline void __update_latency(ktime_t *ctotal, ktime_t *lsum,
>>>>> +                                 ktime_t *lavg, ktime_t *min, 
>>>>> ktime_t *max,
>>>>> +                                 ktime_t *sum_sq, ktime_t lat)
>>>>>   {
>>>>> -     ktime_t avg, sq;
>>>>> +     ktime_t total, avg;
>>>>>
>>>>> -     if (unlikely(total == 1))
>>>>> -             return;
>>>>> +     total = ++(*ctotal);
>>>>> +     *lsum += lat;
>>>>> +
>>>>> +     METRIC_UPDATE_MIN_MAX(*min, *max, lat);
>>>>>
>>>>> -     /* the sq is (lat - old_avg) * (lat - new_avg) */
>>>>> -     avg = DIV64_U64_ROUND_CLOSEST((lsum - lat), (total - 1));
>>>>> -     sq = lat - avg;
>>>>> -     avg = DIV64_U64_ROUND_CLOSEST(lsum, total);
>>>>> -     sq = sq * (lat - avg);
>>>>> -     *sq_sump += sq;
>>>>> +     if (unlikely(total == 1)) {
>>>>> +             *lavg = lat;
>>>>> +             *sum_sq = 0;
>>>>> +     } else {
>>>>> +             avg = *lavg + div64_s64(lat - *lavg, total);
>>>>> +             *sum_sq += (lat - *lavg)*(lat - avg);
>>>>> +             *lavg = avg;
>>>>> +     }
>>>>>   }
>>>>>
>>>>>   void ceph_update_read_metrics(struct ceph_client_metric *m,
>>>>> @@ -343,23 +351,18 @@ void ceph_update_read_metrics(struct 
>>>>> ceph_client_metric *m,
>>>>>                              unsigned int size, int rc)
>>>>>   {
>>>>>        ktime_t lat = ktime_sub(r_end, r_start);
>>>>> -     ktime_t total;
>>>>>
>>>>>        if (unlikely(rc < 0 && rc != -ENOENT && rc != -ETIMEDOUT))
>>>>>                return;
>>>>>
>>>>>        spin_lock(&m->read_metric_lock);
>>>>> -     total = ++m->total_reads;
>>>>>        m->read_size_sum += size;
>>>>> -     m->read_latency_sum += lat;
>>>>>        METRIC_UPDATE_MIN_MAX(m->read_size_min,
>>>>>                              m->read_size_max,
>>>>>                              size);
>>>>> -     METRIC_UPDATE_MIN_MAX(m->read_latency_min,
>>>>> -                           m->read_latency_max,
>>>>> -                           lat);
>>>>> -     __update_stdev(total, m->read_latency_sum,
>>>>> -                    &m->read_latency_sq_sum, lat);
>>>>> +     __update_latency(&m->total_reads, &m->read_latency_sum,
>>>>> +                      &m->avg_read_latency, &m->read_latency_min,
>>>>> +                      &m->read_latency_max, 
>>>>> &m->read_latency_sq_sum, lat);
>>>> Do we really need to calculate the std deviation on every update? We
>>>> have to figure that in most cases, this stuff will be collected but 
>>>> only
>>>> seldom viewed.
>>>>
>>>> ISTM that we ought to collect just the bare minimum of info on each
>>>> update, and save the more expensive calculations for the tool 
>>>> presenting
>>>> this info.
>>> Yeh, that's probably the plan we want going forward when introducing
>>> new metrics.
>>>
>>> FWIW, we could start doing it with this itself. It's just that the
>>> user land PRs are approved and those do the way it is done here.
>>>
>>> I'm ok with moving math crunching to the tool and it should not be a
>>> major change to this patchset.
>> So, I kind of recall why we did it this way -- metrics exchange
>> between MDS and ceph-mgr is restricted to std::pair<uint64_t,
>> uint64_t>.
>
> Before I pushed one patch to improve this and switched it to a list or 
> something else, but revert it dues to some reason.
>
>
Please see 
https://github.com/ceph/ceph/pull/40514#issuecomment-822415454 and in 
the commit 
https://github.com/ceph/ceph/commit/7912e746b9d91d22fb307cc4149fbe683e4f9f35.


>> That would probably need to be expanded to carry variable
>> sized objects. I remember seeing a PR that does something like that.
>> Need to dig it up...
>>
>> The other way would be to exchange the necessary information needed
>> for the calculation as a separate types but that's not really clean
>> and results in unnecessary bloating.
>>
>>>>> spin_unlock(&m->read_metric_lock);
>>>>>   }
>>>>>
>>>>> @@ -368,23 +371,18 @@ void ceph_update_write_metrics(struct 
>>>>> ceph_client_metric *m,
>>>>>                               unsigned int size, int rc)
>>>>>   {
>>>>>        ktime_t lat = ktime_sub(r_end, r_start);
>>>>> -     ktime_t total;
>>>>>
>>>>>        if (unlikely(rc && rc != -ETIMEDOUT))
>>>>>                return;
>>>>>
>>>>>        spin_lock(&m->write_metric_lock);
>>>>> -     total = ++m->total_writes;
>>>>>        m->write_size_sum += size;
>>>>> -     m->write_latency_sum += lat;
>>>>>        METRIC_UPDATE_MIN_MAX(m->write_size_min,
>>>>>                              m->write_size_max,
>>>>>                              size);
>>>>> -     METRIC_UPDATE_MIN_MAX(m->write_latency_min,
>>>>> -                           m->write_latency_max,
>>>>> -                           lat);
>>>>> -     __update_stdev(total, m->write_latency_sum,
>>>>> -                    &m->write_latency_sq_sum, lat);
>>>>> +     __update_latency(&m->total_writes, &m->write_latency_sum,
>>>>> +                      &m->avg_write_latency, &m->write_latency_min,
>>>>> +                      &m->write_latency_max, 
>>>>> &m->write_latency_sq_sum, lat);
>>>>>        spin_unlock(&m->write_metric_lock);
>>>>>   }
>>>>>
>>>>> @@ -393,18 +391,13 @@ void ceph_update_metadata_metrics(struct 
>>>>> ceph_client_metric *m,
>>>>>                                  int rc)
>>>>>   {
>>>>>        ktime_t lat = ktime_sub(r_end, r_start);
>>>>> -     ktime_t total;
>>>>>
>>>>>        if (unlikely(rc && rc != -ENOENT))
>>>>>                return;
>>>>>
>>>>>        spin_lock(&m->metadata_metric_lock);
>>>>> -     total = ++m->total_metadatas;
>>>>> -     m->metadata_latency_sum += lat;
>>>>> -     METRIC_UPDATE_MIN_MAX(m->metadata_latency_min,
>>>>> -                           m->metadata_latency_max,
>>>>> -                           lat);
>>>>> -     __update_stdev(total, m->metadata_latency_sum,
>>>>> -                    &m->metadata_latency_sq_sum, lat);
>>>>> +     __update_latency(&m->total_metadatas, &m->metadata_latency_sum,
>>>>> +                      &m->avg_metadata_latency, 
>>>>> &m->metadata_latency_min,
>>>>> +                      &m->metadata_latency_max, 
>>>>> &m->metadata_latency_sq_sum, lat);
>>>>>        spin_unlock(&m->metadata_metric_lock);
>>>>>   }
>>>>> diff --git a/fs/ceph/metric.h b/fs/ceph/metric.h
>>>>> index 103ed736f9d2..0af02e212033 100644
>>>>> --- a/fs/ceph/metric.h
>>>>> +++ b/fs/ceph/metric.h
>>>>> @@ -138,6 +138,7 @@ struct ceph_client_metric {
>>>>>        u64 read_size_min;
>>>>>        u64 read_size_max;
>>>>>        ktime_t read_latency_sum;
>>>>> +     ktime_t avg_read_latency;
>>>>>        ktime_t read_latency_sq_sum;
>>>>>        ktime_t read_latency_min;
>>>>>        ktime_t read_latency_max;
>>>>> @@ -148,6 +149,7 @@ struct ceph_client_metric {
>>>>>        u64 write_size_min;
>>>>>        u64 write_size_max;
>>>>>        ktime_t write_latency_sum;
>>>>> +     ktime_t avg_write_latency;
>>>>>        ktime_t write_latency_sq_sum;
>>>>>        ktime_t write_latency_min;
>>>>>        ktime_t write_latency_max;
>>>>> @@ -155,6 +157,7 @@ struct ceph_client_metric {
>>>>>        spinlock_t metadata_metric_lock;
>>>>>        u64 total_metadatas;
>>>>>        ktime_t metadata_latency_sum;
>>>>> +     ktime_t avg_metadata_latency;
>>>>>        ktime_t metadata_latency_sq_sum;
>>>>>        ktime_t metadata_latency_min;
>>>>>        ktime_t metadata_latency_max;
>>>> -- 
>>>> Jeff Layton <jlayton@redhat.com>
>>>>
>>>
>>> -- 
>>> Cheers,
>>> Venky
>>
>>


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 2/4] ceph: track average/stdev r/w/m latency
  2021-09-22 14:34         ` Xiubo Li
  2021-09-22 14:37           ` Xiubo Li
@ 2021-09-22 17:10           ` Venky Shankar
  1 sibling, 0 replies; 12+ messages in thread
From: Venky Shankar @ 2021-09-22 17:10 UTC (permalink / raw)
  To: Xiubo Li; +Cc: Jeff Layton, Patrick Donnelly, ceph-devel

On Wed, Sep 22, 2021 at 8:04 PM Xiubo Li <xiubli@redhat.com> wrote:
>
>
> On 9/22/21 9:44 PM, Venky Shankar wrote:
> > On Wed, Sep 22, 2021 at 6:03 PM Venky Shankar <vshankar@redhat.com> wrote:
> >> On Wed, Sep 22, 2021 at 5:47 PM Jeff Layton <jlayton@redhat.com> wrote:
> >>> On Tue, 2021-09-21 at 18:37 +0530, Venky Shankar wrote:
> >>>> Update the math involved to closely mimic how its done in
> >>>> user land. This does not make a lot of difference to the
> >>>> execution speed.
> >>>>
> >>>> Signed-off-by: Venky Shankar <vshankar@redhat.com>
> >>>> ---
> >>>>   fs/ceph/metric.c | 63 +++++++++++++++++++++---------------------------
> >>>>   fs/ceph/metric.h |  3 +++
> >>>>   2 files changed, 31 insertions(+), 35 deletions(-)
> >>>>
> >>>> diff --git a/fs/ceph/metric.c b/fs/ceph/metric.c
> >>>> index 226dc38e2909..ca758bff69ca 100644
> >>>> --- a/fs/ceph/metric.c
> >>>> +++ b/fs/ceph/metric.c
> >>>> @@ -245,6 +245,7 @@ int ceph_metric_init(struct ceph_client_metric *m)
> >>>>
> >>>>        spin_lock_init(&m->read_metric_lock);
> >>>>        m->read_latency_sq_sum = 0;
> >>>> +     m->avg_read_latency = 0;
> >>>>        m->read_latency_min = KTIME_MAX;
> >>>>        m->read_latency_max = 0;
> >>>>        m->total_reads = 0;
> >>>> @@ -255,6 +256,7 @@ int ceph_metric_init(struct ceph_client_metric *m)
> >>>>
> >>>>        spin_lock_init(&m->write_metric_lock);
> >>>>        m->write_latency_sq_sum = 0;
> >>>> +     m->avg_write_latency = 0;
> >>>>        m->write_latency_min = KTIME_MAX;
> >>>>        m->write_latency_max = 0;
> >>>>        m->total_writes = 0;
> >>>> @@ -265,6 +267,7 @@ int ceph_metric_init(struct ceph_client_metric *m)
> >>>>
> >>>>        spin_lock_init(&m->metadata_metric_lock);
> >>>>        m->metadata_latency_sq_sum = 0;
> >>>> +     m->avg_metadata_latency = 0;
> >>>>        m->metadata_latency_min = KTIME_MAX;
> >>>>        m->metadata_latency_max = 0;
> >>>>        m->total_metadatas = 0;
> >>>> @@ -322,20 +325,25 @@ void ceph_metric_destroy(struct ceph_client_metric *m)
> >>>>                max = new;                      \
> >>>>   }
> >>>>
> >>>> -static inline void __update_stdev(ktime_t total, ktime_t lsum,
> >>>> -                               ktime_t *sq_sump, ktime_t lat)
> >>>> +static inline void __update_latency(ktime_t *ctotal, ktime_t *lsum,
> >>>> +                                 ktime_t *lavg, ktime_t *min, ktime_t *max,
> >>>> +                                 ktime_t *sum_sq, ktime_t lat)
> >>>>   {
> >>>> -     ktime_t avg, sq;
> >>>> +     ktime_t total, avg;
> >>>>
> >>>> -     if (unlikely(total == 1))
> >>>> -             return;
> >>>> +     total = ++(*ctotal);
> >>>> +     *lsum += lat;
> >>>> +
> >>>> +     METRIC_UPDATE_MIN_MAX(*min, *max, lat);
> >>>>
> >>>> -     /* the sq is (lat - old_avg) * (lat - new_avg) */
> >>>> -     avg = DIV64_U64_ROUND_CLOSEST((lsum - lat), (total - 1));
> >>>> -     sq = lat - avg;
> >>>> -     avg = DIV64_U64_ROUND_CLOSEST(lsum, total);
> >>>> -     sq = sq * (lat - avg);
> >>>> -     *sq_sump += sq;
> >>>> +     if (unlikely(total == 1)) {
> >>>> +             *lavg = lat;
> >>>> +             *sum_sq = 0;
> >>>> +     } else {
> >>>> +             avg = *lavg + div64_s64(lat - *lavg, total);
> >>>> +             *sum_sq += (lat - *lavg)*(lat - avg);
> >>>> +             *lavg = avg;
> >>>> +     }
> >>>>   }
> >>>>
> >>>>   void ceph_update_read_metrics(struct ceph_client_metric *m,
> >>>> @@ -343,23 +351,18 @@ void ceph_update_read_metrics(struct ceph_client_metric *m,
> >>>>                              unsigned int size, int rc)
> >>>>   {
> >>>>        ktime_t lat = ktime_sub(r_end, r_start);
> >>>> -     ktime_t total;
> >>>>
> >>>>        if (unlikely(rc < 0 && rc != -ENOENT && rc != -ETIMEDOUT))
> >>>>                return;
> >>>>
> >>>>        spin_lock(&m->read_metric_lock);
> >>>> -     total = ++m->total_reads;
> >>>>        m->read_size_sum += size;
> >>>> -     m->read_latency_sum += lat;
> >>>>        METRIC_UPDATE_MIN_MAX(m->read_size_min,
> >>>>                              m->read_size_max,
> >>>>                              size);
> >>>> -     METRIC_UPDATE_MIN_MAX(m->read_latency_min,
> >>>> -                           m->read_latency_max,
> >>>> -                           lat);
> >>>> -     __update_stdev(total, m->read_latency_sum,
> >>>> -                    &m->read_latency_sq_sum, lat);
> >>>> +     __update_latency(&m->total_reads, &m->read_latency_sum,
> >>>> +                      &m->avg_read_latency, &m->read_latency_min,
> >>>> +                      &m->read_latency_max, &m->read_latency_sq_sum, lat);
> >>> Do we really need to calculate the std deviation on every update? We
> >>> have to figure that in most cases, this stuff will be collected but only
> >>> seldom viewed.
> >>>
> >>> ISTM that we ought to collect just the bare minimum of info on each
> >>> update, and save the more expensive calculations for the tool presenting
> >>> this info.
> >> Yeh, that's probably the plan we want going forward when introducing
> >> new metrics.
> >>
> >> FWIW, we could start doing it with this itself. It's just that the
> >> user land PRs are approved and those do the way it is done here.
> >>
> >> I'm ok with moving math crunching to the tool and it should not be a
> >> major change to this patchset.
> > So, I kind of recall why we did it this way -- metrics exchange
> > between MDS and ceph-mgr is restricted to std::pair<uint64_t,
> > uint64_t>.
>
> Before I pushed one patch to improve this and switched it to a list or
> something else, but revert it dues to some reason.

Right. I do not recall the issue, but I think it's worth making it
customizable for future use-cases.

>
>
> > That would probably need to be expanded to carry variable
> > sized objects. I remember seeing a PR that does something like that.
> > Need to dig it up...
> >
> > The other way would be to exchange the necessary information needed
> > for the calculation as a separate types but that's not really clean
> > and results in unnecessary bloating.
> >
> >>>>        spin_unlock(&m->read_metric_lock);
> >>>>   }
> >>>>
> >>>> @@ -368,23 +371,18 @@ void ceph_update_write_metrics(struct ceph_client_metric *m,
> >>>>                               unsigned int size, int rc)
> >>>>   {
> >>>>        ktime_t lat = ktime_sub(r_end, r_start);
> >>>> -     ktime_t total;
> >>>>
> >>>>        if (unlikely(rc && rc != -ETIMEDOUT))
> >>>>                return;
> >>>>
> >>>>        spin_lock(&m->write_metric_lock);
> >>>> -     total = ++m->total_writes;
> >>>>        m->write_size_sum += size;
> >>>> -     m->write_latency_sum += lat;
> >>>>        METRIC_UPDATE_MIN_MAX(m->write_size_min,
> >>>>                              m->write_size_max,
> >>>>                              size);
> >>>> -     METRIC_UPDATE_MIN_MAX(m->write_latency_min,
> >>>> -                           m->write_latency_max,
> >>>> -                           lat);
> >>>> -     __update_stdev(total, m->write_latency_sum,
> >>>> -                    &m->write_latency_sq_sum, lat);
> >>>> +     __update_latency(&m->total_writes, &m->write_latency_sum,
> >>>> +                      &m->avg_write_latency, &m->write_latency_min,
> >>>> +                      &m->write_latency_max, &m->write_latency_sq_sum, lat);
> >>>>        spin_unlock(&m->write_metric_lock);
> >>>>   }
> >>>>
> >>>> @@ -393,18 +391,13 @@ void ceph_update_metadata_metrics(struct ceph_client_metric *m,
> >>>>                                  int rc)
> >>>>   {
> >>>>        ktime_t lat = ktime_sub(r_end, r_start);
> >>>> -     ktime_t total;
> >>>>
> >>>>        if (unlikely(rc && rc != -ENOENT))
> >>>>                return;
> >>>>
> >>>>        spin_lock(&m->metadata_metric_lock);
> >>>> -     total = ++m->total_metadatas;
> >>>> -     m->metadata_latency_sum += lat;
> >>>> -     METRIC_UPDATE_MIN_MAX(m->metadata_latency_min,
> >>>> -                           m->metadata_latency_max,
> >>>> -                           lat);
> >>>> -     __update_stdev(total, m->metadata_latency_sum,
> >>>> -                    &m->metadata_latency_sq_sum, lat);
> >>>> +     __update_latency(&m->total_metadatas, &m->metadata_latency_sum,
> >>>> +                      &m->avg_metadata_latency, &m->metadata_latency_min,
> >>>> +                      &m->metadata_latency_max, &m->metadata_latency_sq_sum, lat);
> >>>>        spin_unlock(&m->metadata_metric_lock);
> >>>>   }
> >>>> diff --git a/fs/ceph/metric.h b/fs/ceph/metric.h
> >>>> index 103ed736f9d2..0af02e212033 100644
> >>>> --- a/fs/ceph/metric.h
> >>>> +++ b/fs/ceph/metric.h
> >>>> @@ -138,6 +138,7 @@ struct ceph_client_metric {
> >>>>        u64 read_size_min;
> >>>>        u64 read_size_max;
> >>>>        ktime_t read_latency_sum;
> >>>> +     ktime_t avg_read_latency;
> >>>>        ktime_t read_latency_sq_sum;
> >>>>        ktime_t read_latency_min;
> >>>>        ktime_t read_latency_max;
> >>>> @@ -148,6 +149,7 @@ struct ceph_client_metric {
> >>>>        u64 write_size_min;
> >>>>        u64 write_size_max;
> >>>>        ktime_t write_latency_sum;
> >>>> +     ktime_t avg_write_latency;
> >>>>        ktime_t write_latency_sq_sum;
> >>>>        ktime_t write_latency_min;
> >>>>        ktime_t write_latency_max;
> >>>> @@ -155,6 +157,7 @@ struct ceph_client_metric {
> >>>>        spinlock_t metadata_metric_lock;
> >>>>        u64 total_metadatas;
> >>>>        ktime_t metadata_latency_sum;
> >>>> +     ktime_t avg_metadata_latency;
> >>>>        ktime_t metadata_latency_sq_sum;
> >>>>        ktime_t metadata_latency_min;
> >>>>        ktime_t metadata_latency_max;
> >>> --
> >>> Jeff Layton <jlayton@redhat.com>
> >>>
> >>
> >> --
> >> Cheers,
> >> Venky
> >
> >
>


-- 
Cheers,
Venky


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-09-22 17:11 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-21 13:07 [PATCH v3 0/4] ceph: forward average read/write/metadata latency Venky Shankar
2021-09-21 13:07 ` [PATCH v3 1/4] ceph: use "struct ceph_timespec" for r/w/m latencies Venky Shankar
2021-09-21 13:07 ` [PATCH v3 2/4] ceph: track average/stdev r/w/m latency Venky Shankar
2021-09-22 12:17   ` Jeff Layton
2021-09-22 12:33     ` Venky Shankar
2021-09-22 13:44       ` Venky Shankar
2021-09-22 14:34         ` Xiubo Li
2021-09-22 14:37           ` Xiubo Li
2021-09-22 17:10           ` Venky Shankar
2021-09-22 13:45       ` Xiubo Li
2021-09-21 13:07 ` [PATCH v3 3/4] ceph: include average/stddev r/w/m latency in mds metrics Venky Shankar
2021-09-21 13:07 ` [PATCH v3 4/4] ceph: use tracked average r/w/m latencies to display metrics in debugfs Venky Shankar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).