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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



在 15/6/15 下午9:04, Mikulas Patocka 写道:
>
> On Sun, 14 Jun 2015, Coly Li wrote:
>
>> ? 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@xxxxxxxxxx>
>>>
>>> ---
>>>  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.
> The above code doesn't call ktime_get() for each list entry - it calls 
> ktime_get() only once, then sets got_precise_time, and doesn't call it 
> anymore.
>
>> 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;
>> +        }
> This is worse that the previous code - this piece of code calls 
> ktime_get() even if no list entry requires precise timestamps. The 
> previous code called ktime_get() only if there is at least one list entry 
> that has STAT_PRECISE_TIMESTAMPS set.
Yes you are right, I made a mistake here.

>> 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 ...
> I don't know what you mean - if you remove the above changes in 
> message_stats_create - then obviously - the user will not be able to 
> enable precise timestamps or histogram.
I just suggest a simpler interface. Multiple boundaries might be over
designed here, maybe a pre-defined latency boundaries will make 90%+
users happy. How about just a "precise_timestamps" parameter and
allocate the array dynamically still ?

Coly

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




[Index of Archives]     [DM Crypt]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Packaging]     [Fedora SELinux]     [Yosemite Discussion]     [KDE Users]     [Fedora Docs]

  Powered by Linux