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.
> 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.
Mikulas
> [snip]
>
> Coly
>
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
