On Mon, Apr 05, 2010 at 04:29:06PM -0700, Divyesh Shah wrote:
quoted text
> On Mon, Apr 5, 2010 at 9:06 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Fri, Apr 02, 2010 at 06:56:35PM -0700, Divyesh Shah wrote:
> >> - io_service_time (the actual time in ns taken by the dis to service the IO)
> >> - io_wait_time (the time spent waiting in the IO shceduler queues before
> >>   getting serviced)
> >> - io_serviced (number of IOs serviced from this blkio_group)
> >> - io_service_bytes (Number of bytes served for this cgroup)
> >>
> >> These stats are accumulated per operation type helping us to distinguish between
> >> read and write, and sync and async IO. This patch does not increment any of
> >> these stats.
> >>
> >> Signed-off-by: Divyesh Shah<dpshah@google.com>
> >> ---
> >>
> >>  Documentation/cgroups/blkio-controller.txt |   33 +++++
> >>  block/blk-cgroup.c                         |  179 +++++++++++++++++++++++++---
> >>  block/blk-cgroup.h                         |   41 +++++-
> >>  block/cfq-iosched.c                        |    2
> >>  4 files changed, 229 insertions(+), 26 deletions(-)
> >>
> >> diff --git a/Documentation/cgroups/blkio-controller.txt b/Documentation/cgroups/blkio-controller.txt
> >> index 630879c..ededdca 100644
> >> --- a/Documentation/cgroups/blkio-controller.txt
> >> +++ b/Documentation/cgroups/blkio-controller.txt
> >> @@ -75,6 +75,9 @@ CONFIG_DEBUG_BLK_CGROUP
> >>
> >>  Details of cgroup files
> >>  =======================
> >> +Writing an int to any of the stats files (which excludes weight) will result
> >> +in all stats for that cgroup to be erased.
> >> +
> >>  - blkio.weight
> >>       - Specifies per cgroup weight.
> >>
> >> @@ -92,6 +95,36 @@ Details of cgroup files
> >>         third field specifies the number of sectors transferred by the
> >>         group to/from the device.
> >>
> >> +- blkio.io_service_bytes
> >> +     - Number of bytes transferred to/from the disk by the group. These
> >> +       are further divided by the type of operation - read or write, sync
> >> +       or async. First two fields specify the major and minor number of the
> >> +       device, third field specifies the operation type and the fourth field
> >> +       specifies the number of bytes.
> >> +
> >> +- blkio.io_serviced
> >> +     - Number of IOs completed to/from the disk by the group. These
> >> +       are further divided by the type of operation - read or write, sync
> >> +       or async. First two fields specify the major and minor number of the
> >> +       device, third field specifies the operation type and the fourth field
> >> +       specifies the number of IOs.
> >> +
> >> +- blkio.io_service_time
> >> +     - Total amount of time spent by the device to service the IOs for this
> >> +       cgroup. This is in nanoseconds to make it meaningful for flash
> >> +       devices too. This time is further divided by the type of operation -
> >> +       read or write, sync or async. First two fields specify the major and
> >> +       minor number of the device, third field specifies the operation type
> >> +       and the fourth field specifies the io_service_time in ns.
> >
> > Hi Divyesh,
> >
> > Looking the third patch where you acutally increment the stats, this is
> > how service time is calculated.
> >
> > - Save start_time in rq, when driver actually removes the request from
> >  request queue.
> > - at request completion time calculate service time.
> >  service_time = now - start_time.
> >
> > This works very well if driver/device does not have NCQ and process one
> > request at a time. But with NCQ, we can multiple requests in the driver
> > queue at the same time then we can run into issues.
> >
> > - With NCQ, time becomes cumulative. So if three requests rq1, rq2 and rq3
> >  are in disk queue, and if requests are processed in the order rq1,rq2,rq3 by
> >  the disk (no parallel channles), then rq1 and rq2's completion time is
> >  added in rq3. So total service time of the group can be much more than
> >  actual time elapsed. That does not seem right. Did you face this issue in
> >  your testing?
> 
> You are right. With NCQ, the io_service_time numbers aren't exact.
> I've only tested this without NCQ for which these stats are accurate
> and very useful. With NCQ turned on I don't see a good way of getting
> this information accurately since only the disk knows when it actually
> starts to service a given request. With NCQ, io_service_time will have
> cumulative time as you pointed out which may still be useful for some
> high-level analysis if you also have the average queue depth for the
> device.
> 

Most of the modern disks now seem to be be NCQ. But I guess we can keep
those stats as it is useful for you on non NCQ setup.

quoted text
> io_wait_time stat makes sense with or w/o NCQ since its defined to
> only include the scheduler queueing time for each IO and is cumulative
> by design. I should probably reword the definition of io_service_time
> to indicate time after dispatch to the driver and request completion
> and also add a comment of how NCQ affects this stat. Sounds good?

I am not sure if io_wait_time is cumulative by design. Think of this,
somebody says that this cgroup has waited for 1s on CFQ queue but actually
only .5 seconds have elapsed since the observation started.

So I would say that please document it very properly in blkio-controller.txt
so that it is clear what exactly these numbers represent and what to expect on
NCQ disks.

Thanks
Vivek
 
quoted text
> 
> -Divyesh
> 
> >
> > Same is the case with io_wait_time, Because time is cumulative, actual
> > io_wait_time, can be more than real time elapsed.
> >
> > Is this the intention and you are finding even cumulative time useful?
> >
> >> +
> >> +- blkio.io_wait_time
> >> +     - Total amount of time the IO spent waiting in the scheduler queues for
> >> +       service. This is in nanoseconds to make it meaningful for flash
> >> +       devices too. This time is further divided by the type of operation -
> >> +       read or write, sync or async. First two fields specify the major and
> >> +       minor number of the device, third field specifies the operation type
> >> +       and the fourth field specifies the io_wait_time in ns.
> >> +
> >>  - blkio.dequeue
> >>       - Debugging aid only enabled if CONFIG_DEBUG_CFQ_IOSCHED=y. This
> >>         gives the statistics about how many a times a group was dequeued
> >> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> >> index 5be3981..cac10b2 100644
> >> --- a/block/blk-cgroup.c
> >> +++ b/block/blk-cgroup.c
> >> @@ -17,6 +17,8 @@
> >>  #include <linux/err.h>
> >>  #include "blk-cgroup.h"
> >>
> >> +#define MAX_KEY_LEN 100
> >> +
> >>  static DEFINE_SPINLOCK(blkio_list_lock);
> >>  static LIST_HEAD(blkio_list);
> >>
> >> @@ -55,12 +57,15 @@ struct blkio_cgroup *cgroup_to_blkio_cgroup(struct cgroup *cgroup)
> >>  }
> >>  EXPORT_SYMBOL_GPL(cgroup_to_blkio_cgroup);
> >>
> >> -void blkiocg_update_blkio_group_stats(struct blkio_group *blkg,
> >> -                                             unsigned long time)
> >> +void blkiocg_update_timeslice_used(struct blkio_group *blkg, unsigned long time)
> >>  {
> >> -     blkg->time += time;
> >> +     unsigned long flags;
> >> +
> >> +     spin_lock_irqsave(&blkg->stats_lock, flags);
> >> +     blkg->stats.time += time;
> >> +     spin_unlock_irqrestore(&blkg->stats_lock, flags);
> >>  }
> >> -EXPORT_SYMBOL_GPL(blkiocg_update_blkio_group_stats);
> >> +EXPORT_SYMBOL_GPL(blkiocg_update_timeslice_used);
> >>
> >>  void blkiocg_add_blkio_group(struct blkio_cgroup *blkcg,
> >>                       struct blkio_group *blkg, void *key, dev_t dev)
> >> @@ -170,13 +175,119 @@ blkiocg_weight_write(struct cgroup *cgroup, struct cftype *cftype, u64 val)
> >>       return 0;
> >>  }
> >>
> >> -#define SHOW_FUNCTION_PER_GROUP(__VAR)                                       \
> >> +static int
> >> +blkiocg_reset_write(struct cgroup *cgroup, struct cftype *cftype, u64 val)
> >> +{
> >> +     struct blkio_cgroup *blkcg;
> >> +     struct blkio_group *blkg;
> >> +     struct hlist_node *n;
> >> +     struct blkio_group_stats *stats;
> >> +
> >> +     blkcg = cgroup_to_blkio_cgroup(cgroup);
> >> +     spin_lock_irq(&blkcg->lock);
> >> +     hlist_for_each_entry(blkg, n, &blkcg->blkg_list, blkcg_node) {
> >> +             spin_lock(&blkg->stats_lock);
> >> +             stats = &blkg->stats;
> >> +             memset(stats, 0, sizeof(struct blkio_group_stats));
> >> +             spin_unlock(&blkg->stats_lock);
> >> +     }
> >> +     spin_unlock_irq(&blkcg->lock);
> >> +     return 0;
> >> +}
> >> +
> >> +static void blkio_get_key_name(int type, dev_t dev, char *str, int chars_left)
> >> +{
> >> +     snprintf(str, chars_left, "%u:%u", MAJOR(dev), MINOR(dev));
> >> +     chars_left -= strlen(str);
> >> +     if (chars_left <= 0) {
> >> +             printk(KERN_WARNING
> >> +                     "Possibly incorrect cgroup stat display format");
> >> +             return;
> >> +     }
> >> +     switch (type) {
> >> +     case IO_READ:
> >> +             strlcat(str, " Read", chars_left);
> >> +             break;
> >> +     case IO_WRITE:
> >> +             strlcat(str, " Write", chars_left);
> >> +             break;
> >> +     case IO_SYNC:
> >> +             strlcat(str, " Sync", chars_left);
> >> +             break;
> >> +     case IO_ASYNC:
> >> +             strlcat(str, " Async", chars_left);
> >> +             break;
> >> +     case IO_TYPE_TOTAL:
> >> +             strlcat(str, " Total", chars_left);
> >> +             break;
> >> +     default:
> >> +             strlcat(str, " Invalid", chars_left);
> >> +     }
> >> +}
> >> +
> >> +typedef uint64_t (get_var) (struct blkio_group *, int);
> >> +
> >> +static uint64_t blkio_get_typed_stat(struct blkio_group *blkg,
> >> +             struct cgroup_map_cb *cb, get_var *getvar, dev_t dev)
> >> +{
> >> +     uint64_t disk_total;
> >> +     char key_str[MAX_KEY_LEN];
> >> +     int type;
> >> +
> >> +     for (type = 0; type < IO_TYPE_TOTAL; type++) {
> >> +             blkio_get_key_name(type, dev, key_str, MAX_KEY_LEN);
> >> +             cb->fill(cb, key_str, getvar(blkg, type));
> >> +     }
> >> +     disk_total = getvar(blkg, IO_READ) + getvar(blkg, IO_WRITE);
> >> +     blkio_get_key_name(IO_TYPE_TOTAL, dev, key_str, MAX_KEY_LEN);
> >> +     cb->fill(cb, key_str, disk_total);
> >> +     return disk_total;
> >> +}
> >> +
> >> +static uint64_t blkio_get_stat(struct blkio_group *blkg,
> >> +             struct cgroup_map_cb *cb, get_var *getvar, dev_t dev)
> >> +{
> >> +     uint64_t var = getvar(blkg, 0);
> >> +     char str[10];
> >> +
> >> +     snprintf(str, 10, "%u:%u", MAJOR(dev), MINOR(dev));
> >> +     cb->fill(cb, str, var);
> >> +     return var;
> >> +}
> >> +
> >> +#define GET_STAT_INDEXED(__VAR)                                              \
> >> +uint64_t blkio_get_##__VAR##_stat(struct blkio_group *blkg, int type)        \
> >> +{                                                                    \
> >> +     return blkg->stats.__VAR[type];                                 \
> >> +}                                                                    \
> >> +
> >> +GET_STAT_INDEXED(io_service_bytes);
> >> +GET_STAT_INDEXED(io_serviced);
> >> +GET_STAT_INDEXED(io_service_time);
> >> +GET_STAT_INDEXED(io_wait_time);
> >> +#undef GET_STAT_INDEXED
> >> +
> >> +#define GET_STAT(__VAR)                                                      \
> >> +uint64_t blkio_get_##__VAR##_stat(struct blkio_group *blkg, int dummy)       \
> >> +{                                                                    \
> >> +     return blkg->stats.__VAR;                                       \
> >> +}
> >> +
> >> +GET_STAT(time);
> >> +GET_STAT(sectors);
> >> +#ifdef CONFIG_DEBUG_BLK_CGROUP
> >> +GET_STAT(dequeue);
> >> +#endif
> >> +#undef GET_STAT
> >> +
> >> +#define SHOW_FUNCTION_PER_GROUP(__VAR, get_stats, getvar, show_total)        \
> >>  static int blkiocg_##__VAR##_read(struct cgroup *cgroup,             \
> >> -                     struct cftype *cftype, struct seq_file *m)      \
> >> +             struct cftype *cftype, struct cgroup_map_cb *cb)        \
> >>  {                                                                    \
> >>       struct blkio_cgroup *blkcg;                                     \
> >>       struct blkio_group *blkg;                                       \
> >>       struct hlist_node *n;                                           \
> >> +     uint64_t cgroup_total = 0;                                      \
> >>                                                                       \
> >>       if (!cgroup_lock_live_group(cgroup))                            \
> >>               return -ENODEV;                                         \
> >> @@ -184,19 +295,32 @@ static int blkiocg_##__VAR##_read(struct cgroup *cgroup,                \
> >>       blkcg = cgroup_to_blkio_cgroup(cgroup);                         \
> >>       rcu_read_lock();                                                \
> >>       hlist_for_each_entry_rcu(blkg, n, &blkcg->blkg_list, blkcg_node) {\
> >> -             if (blkg->dev)                                          \
> >> -                     seq_printf(m, "%u:%u %lu\n", MAJOR(blkg->dev),  \
> >> -                              MINOR(blkg->dev), blkg->__VAR);        \
> >> +             if (blkg->dev) {                                        \
> >> +                     spin_lock_irq(&blkg->stats_lock);               \
> >> +                     cgroup_total += get_stats(blkg, cb, getvar,     \
> >> +                                             blkg->dev);             \
> >> +                     spin_unlock_irq(&blkg->stats_lock);             \
> >> +             }                                                       \
> >>       }                                                               \
> >> +     if (show_total)                                                 \
> >> +             cb->fill(cb, "Total", cgroup_total);                    \
> >>       rcu_read_unlock();                                              \
> >>       cgroup_unlock();                                                \
> >>       return 0;                                                       \
> >>  }
> >>
> >> -SHOW_FUNCTION_PER_GROUP(time);
> >> -SHOW_FUNCTION_PER_GROUP(sectors);
> >> +SHOW_FUNCTION_PER_GROUP(time, blkio_get_stat, blkio_get_time_stat, 0);
> >> +SHOW_FUNCTION_PER_GROUP(sectors, blkio_get_stat, blkio_get_sectors_stat, 0);
> >> +SHOW_FUNCTION_PER_GROUP(io_service_bytes, blkio_get_typed_stat,
> >> +                     blkio_get_io_service_bytes_stat, 1);
> >> +SHOW_FUNCTION_PER_GROUP(io_serviced, blkio_get_typed_stat,
> >> +                     blkio_get_io_serviced_stat, 1);
> >> +SHOW_FUNCTION_PER_GROUP(io_service_time, blkio_get_typed_stat,
> >> +                     blkio_get_io_service_time_stat, 1);
> >> +SHOW_FUNCTION_PER_GROUP(io_wait_time, blkio_get_typed_stat,
> >> +                     blkio_get_io_wait_time_stat, 1);
> >>  #ifdef CONFIG_DEBUG_BLK_CGROUP
> >> -SHOW_FUNCTION_PER_GROUP(dequeue);
> >> +SHOW_FUNCTION_PER_GROUP(dequeue, blkio_get_stat, blkio_get_dequeue_stat, 0);
> >>  #endif
> >>  #undef SHOW_FUNCTION_PER_GROUP
> >>
> >> @@ -204,7 +328,7 @@ SHOW_FUNCTION_PER_GROUP(dequeue);
> >>  void blkiocg_update_blkio_group_dequeue_stats(struct blkio_group *blkg,
> >>                       unsigned long dequeue)
> >>  {
> >> -     blkg->dequeue += dequeue;
> >> +     blkg->stats.dequeue += dequeue;
> >>  }
> >>  EXPORT_SYMBOL_GPL(blkiocg_update_blkio_group_dequeue_stats);
> >>  #endif
> >> @@ -217,16 +341,39 @@ struct cftype blkio_files[] = {
> >>       },
> >>       {
> >>               .name = "time",
> >> -             .read_seq_string = blkiocg_time_read,
> >> +             .read_map = blkiocg_time_read,
> >> +             .write_u64 = blkiocg_reset_write,
> >>       },
> >>       {
> >>               .name = "sectors",
> >> -             .read_seq_string = blkiocg_sectors_read,
> >> +             .read_map = blkiocg_sectors_read,
> >> +             .write_u64 = blkiocg_reset_write,
> >> +     },
> >> +     {
> >> +             .name = "io_service_bytes",
> >> +             .read_map = blkiocg_io_service_bytes_read,
> >> +             .write_u64 = blkiocg_reset_write,
> >> +     },
> >> +     {
> >> +             .name = "io_serviced",
> >> +             .read_map = blkiocg_io_serviced_read,
> >> +             .write_u64 = blkiocg_reset_write,
> >> +     },
> >> +     {
> >> +             .name = "io_service_time",
> >> +             .read_map = blkiocg_io_service_time_read,
> >> +             .write_u64 = blkiocg_reset_write,
> >> +     },
> >> +     {
> >> +             .name = "io_wait_time",
> >> +             .read_map = blkiocg_io_wait_time_read,
> >> +             .write_u64 = blkiocg_reset_write,
> >>       },
> >>  #ifdef CONFIG_DEBUG_BLK_CGROUP
> >>         {
> >>               .name = "dequeue",
> >> -             .read_seq_string = blkiocg_dequeue_read,
> >> +             .read_map = blkiocg_dequeue_read,
> >> +             .write_u64 = blkiocg_reset_write,
> >>         },
> >>  #endif
> >>  };
> >> diff --git a/block/blk-cgroup.h b/block/blk-cgroup.h
> >> index fe44517..e8600b0 100644
> >> --- a/block/blk-cgroup.h
> >> +++ b/block/blk-cgroup.h
> >> @@ -23,6 +23,14 @@ extern struct cgroup_subsys blkio_subsys;
> >>  #define blkio_subsys_id blkio_subsys.subsys_id
> >>  #endif
> >>
> >> +enum io_type {
> >> +     IO_READ = 0,
> >> +     IO_WRITE,
> >> +     IO_SYNC,
> >> +     IO_ASYNC,
> >> +     IO_TYPE_TOTAL
> >> +};
> >> +
> >>  struct blkio_cgroup {
> >>       struct cgroup_subsys_state css;
> >>       unsigned int weight;
> >> @@ -30,6 +38,23 @@ struct blkio_cgroup {
> >>       struct hlist_head blkg_list;
> >>  };
> >>
> >> +struct blkio_group_stats {
> >> +     /* total disk time and nr sectors dispatched by this group */
> >> +     uint64_t time;
> >> +     uint64_t sectors;
> >> +     /* Total disk time used by IOs in ns */
> >> +     uint64_t io_service_time[IO_TYPE_TOTAL];
> >> +     uint64_t io_service_bytes[IO_TYPE_TOTAL]; /* Total bytes transferred */
> >> +     /* Total IOs serviced, post merge */
> >> +     uint64_t io_serviced[IO_TYPE_TOTAL];
> >> +     /* Total time spent waiting in scheduler queue in ns */
> >> +     uint64_t io_wait_time[IO_TYPE_TOTAL];
> >> +#ifdef CONFIG_DEBUG_BLK_CGROUP
> >> +     /* How many times this group has been removed from service tree */
> >> +     unsigned long dequeue;
> >> +#endif
> >> +};
> >> +
> >>  struct blkio_group {
> >>       /* An rcu protected unique identifier for the group */
> >>       void *key;
> >> @@ -38,15 +63,13 @@ struct blkio_group {
> >>  #ifdef CONFIG_DEBUG_BLK_CGROUP
> >>       /* Store cgroup path */
> >>       char path[128];
> >> -     /* How many times this group has been removed from service tree */
> >> -     unsigned long dequeue;
> >>  #endif
> >>       /* The device MKDEV(major, minor), this group has been created for */
> >> -     dev_t   dev;
> >> +     dev_t dev;
> >>
> >> -     /* total disk time and nr sectors dispatched by this group */
> >> -     unsigned long time;
> >> -     unsigned long sectors;
> >> +     /* Need to serialize the stats in the case of reset/update */
> >> +     spinlock_t stats_lock;
> >> +     struct blkio_group_stats stats;
> >>  };
> >>
> >>  typedef void (blkio_unlink_group_fn) (void *key, struct blkio_group *blkg);
> >> @@ -105,8 +128,8 @@ extern void blkiocg_add_blkio_group(struct blkio_cgroup *blkcg,
> >>  extern int blkiocg_del_blkio_group(struct blkio_group *blkg);
> >>  extern struct blkio_group *blkiocg_lookup_group(struct blkio_cgroup *blkcg,
> >>                                               void *key);
> >> -void blkiocg_update_blkio_group_stats(struct blkio_group *blkg,
> >> -                                             unsigned long time);
> >> +void blkiocg_update_timeslice_used(struct blkio_group *blkg,
> >> +                                     unsigned long time);
> >>  #else
> >>  struct cgroup;
> >>  static inline struct blkio_cgroup *
> >> @@ -122,7 +145,7 @@ blkiocg_del_blkio_group(struct blkio_group *blkg) { return 0; }
> >>
> >>  static inline struct blkio_group *
> >>  blkiocg_lookup_group(struct blkio_cgroup *blkcg, void *key) { return NULL; }
> >> -static inline void blkiocg_update_blkio_group_stats(struct blkio_group *blkg,
> >> +static inline void blkiocg_update_timeslice_used(struct blkio_group *blkg,
> >>                                               unsigned long time) {}
> >>  #endif
> >>  #endif /* _BLK_CGROUP_H */
> >> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> >> index c18e348..d91df9f 100644
> >> --- a/block/cfq-iosched.c
> >> +++ b/block/cfq-iosched.c
> >> @@ -914,7 +914,7 @@ static void cfq_group_served(struct cfq_data *cfqd, struct cfq_group *cfqg,
> >>
> >>       cfq_log_cfqg(cfqd, cfqg, "served: vt=%llu min_vt=%llu", cfqg->vdisktime,
> >>                                       st->min_vdisktime);
> >> -     blkiocg_update_blkio_group_stats(&cfqg->blkg, used_sl);
> >> +     blkiocg_update_timeslice_used(&cfqg->blkg, used_sl);
> >>  }
> >>
> >>  #ifdef CONFIG_CFQ_GROUP_IOSCHED
> >
--
unsubscribe notice
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/