Re: [PATCH 2/4] dm stats: support precise timestamps

From: Coly Li <colyli@gmail.com>
To: Mikulas Patocka <mpatocka@redhat.com>,
	"Alasdair G. Kergon" <agk@redhat.com>,
	Mike Snitzer <msnitzer@redhat.com>
Cc: Tao Ma <boyu.mt@taobao.com>,
	dm-devel@redhat.com, Laurence Oberman <loberman@redhat.com>,
	Robin Dong <sanbai@alibaba-inc.com>
Subject: Re: [PATCH 2/4] dm stats: support precise timestamps
Date: Sun, 14 Jun 2015 01:03:34 +0800	[thread overview]
Message-ID: <557C6266.3040008@gmail.com> (raw)
In-Reply-To: <alpine.LRH.2.02.1506091721180.1545@file01.intranet.prod.int.rdu2.redhat.com>

在 15/6/10 上午5:21, Mikulas Patocka 写道:
> This patch makes it possible to use precise timestamps with nanosecond
> granularity in dm statistics.
>
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
>
> ---
>  Documentation/device-mapper/statistics.txt |   25 ++++-
>  drivers/md/dm-stats.c                      |  139 +++++++++++++++++++++--------
>  drivers/md/dm-stats.h                      |    4 
>  3 files changed, 125 insertions(+), 43 deletions(-)
>
> Index: linux-4.1-rc7/drivers/md/dm-stats.c
> ===================================================================
> --- linux-4.1-rc7.orig/drivers/md/dm-stats.c	2015-06-08 16:02:27.000000000 +0200
> +++ linux-4.1-rc7/drivers/md/dm-stats.c	2015-06-08 16:38:43.000000000 +0200
> @@ -33,13 +33,14 @@ struct dm_stat_percpu {
>  
[snip]
> @@ -560,8 +578,17 @@ void dm_stats_account_io(struct dm_stats
>  
>  	rcu_read_lock();
>  
> -	list_for_each_entry_rcu(s, &stats->list, list_entry)
> -		__dm_stat_bio(s, bi_rw, bi_sector, end_sector, end, duration, stats_aux);
> +	got_precise_time = false;
> +	list_for_each_entry_rcu(s, &stats->list, list_entry) {
> +		if (s->stat_flags & STAT_PRECISE_TIMESTAMPS && !got_precise_time) {
> +			if (!end)
> +				stats_aux->duration_ns = ktime_to_ns(ktime_get());
> +			else
> +				stats_aux->duration_ns = ktime_to_ns(ktime_get()) - stats_aux->duration_ns;
> +			got_precise_time = true;
> +		}
There are many conditions user space processes may query the global
timer resource frequently. ktime_get() will call spin_lock when
accessing global timer resource, calling ktime_get() for each list entry
might introduce locking contention when stats->list is quite large. A
satisfied solution might be something like this,

+    now = ktime_to_ns(ktime_get());
+    list_for_each_entry_rcu(s, &stats->list, list_entry) {
+        if (s->stat_flags & STAT_PRECISE_TIMESTAMPS && !got_precise_time) {
+            if (!end)
+                stats_aux->duration_ns = now;
+            else
+                stats_aux->duration_ns = now - stats_aux->duration_ns;
+            got_precise_time = true;
+        }

In this case the global timer could be access only once, locking
contention should be much less. Yes, it looks to be some inaccurate, but
for hard disk I/O latency measurement, the difference almost could be
ignored.
> +		__dm_stat_bio(s, bi_rw, bi_sector, end_sector, end, duration_jiffies, stats_aux);
> +	}
>  
>  	rcu_read_unlock();
>  }
[snip]
> @@ -772,21 +803,31 @@ static int message_stats_create(struct m
>  	unsigned long long start, end, len, step;
>  	unsigned divisor;
>  	const char *program_id, *aux_data;
> +	unsigned stat_flags = 0;
> +
> +	struct dm_arg_set as, as_backup;
> +	const char *a;
> +	unsigned feature_args;
>  
>  	/*
>  	 * Input format:
> -	 *   <range> <step> [<program_id> [<aux_data>]]
> +	 *   <range> <step> [<extra_parameters> <parameters>] [<program_id> [<aux_data>]]
>  	 */
>  
> -	if (argc < 3 || argc > 5)
> +	if (argc < 3)
>  		return -EINVAL;
>  
> -	if (!strcmp(argv[1], "-")) {
> +	as.argc = argc;
> +	as.argv = argv;
> +	dm_consume_args(&as, 1);
> +
> +	a = dm_shift_arg(&as);
> +	if (!strcmp(a, "-")) {
>  		start = 0;
>  		len = dm_get_size(md);
>  		if (!len)
>  			len = 1;
> -	} else if (sscanf(argv[1], "%llu+%llu%c", &start, &len, &dummy) != 2 ||
> +	} else if (sscanf(a, "%llu+%llu%c", &start, &len, &dummy) != 2 ||
>  		   start != (sector_t)start || len != (sector_t)len)
>  		return -EINVAL;
>  
> @@ -794,7 +835,8 @@ static int message_stats_create(struct m
>  	if (start >= end)
>  		return -EINVAL;
>  
> -	if (sscanf(argv[2], "/%u%c", &divisor, &dummy) == 1) {
> +	a = dm_shift_arg(&as);
> +	if (sscanf(a, "/%u%c", &divisor, &dummy) == 1) {
>  		if (!divisor)
>  			return -EINVAL;
>  		step = end - start;
> @@ -802,18 +844,39 @@ static int message_stats_create(struct m
>  			step++;
>  		if (!step)
>  			step = 1;
> -	} else if (sscanf(argv[2], "%llu%c", &step, &dummy) != 1 ||
> +	} else if (sscanf(a, "%llu%c", &step, &dummy) != 1 ||
>  		   step != (sector_t)step || !step)
>  		return -EINVAL;
>  
> +	as_backup = as;
> +	a = dm_shift_arg(&as);
> +	if (a && sscanf(a, "%u%c", &feature_args, &dummy) == 1) {
> +		while (feature_args--) {
> +			a = dm_shift_arg(&as);
> +			if (!a)
> +				return -EINVAL;
> +			if (!strcasecmp(a, "precise_timestamps"))
> +				stat_flags |= STAT_PRECISE_TIMESTAMPS;
> +			else
> +				return -EINVAL;
> +		}
> +	} else {
> +		as = as_backup;
> +	}
> +
>  	program_id = "-";
>  	aux_data = "-";
>  
> -	if (argc > 3)
> -		program_id = argv[3];
> +	a = dm_shift_arg(&as);
> +	if (a)
> +		program_id = a;
> +
> +	a = dm_shift_arg(&as);
> +	if (a)
> +		aux_data = a;
>  
> -	if (argc > 4)
> -		aux_data = argv[4];
> +	if (as.argc)
> +		return -EINVAL;
>  
If I could, I would simplify the design to remove the above changes in
message_stats_create(). Just my suggestion ...

[snip]

Coly

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel