* Re: [BLOCK] Unify the seperate read/write io stat fields into arrays [not found] <200511021704.jA2H4X4u027306@hera.kernel.org> @ 2005-11-02 18:38 ` Chris Wedgwood 2005-11-03 7:31 ` Jens Axboe 2005-11-04 3:22 ` Jeff Garzik 1 sibling, 1 reply; 4+ messages in thread From: Chris Wedgwood @ 2005-11-02 18:38 UTC (permalink / raw) To: Linux Kernel Mailing List, Jens Axboe I wouldn't mind a comment with that: > struct disk_stats { > - unsigned read_sectors, write_sectors; > - unsigned reads, writes; > - unsigned read_merges, write_merges; > - unsigned read_ticks, write_ticks; /* Element 0 is for reads, 1 for writes */ > + unsigned sectors[2]; > + unsigned ios[2]; > + unsigned merges[2]; > + unsigned ticks[2]; > unsigned io_ticks; > unsigned time_in_queue; > }; ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [BLOCK] Unify the seperate read/write io stat fields into arrays 2005-11-02 18:38 ` [BLOCK] Unify the seperate read/write io stat fields into arrays Chris Wedgwood @ 2005-11-03 7:31 ` Jens Axboe 0 siblings, 0 replies; 4+ messages in thread From: Jens Axboe @ 2005-11-03 7:31 UTC (permalink / raw) To: Chris Wedgwood; +Cc: Linux Kernel Mailing List On Wed, Nov 02 2005, Chris Wedgwood wrote: > I wouldn't mind a comment with that: > > > struct disk_stats { > > - unsigned read_sectors, write_sectors; > > - unsigned reads, writes; > > - unsigned read_merges, write_merges; > > - unsigned read_ticks, write_ticks; > /* Element 0 is for reads, 1 for writes */ > > + unsigned sectors[2]; > > + unsigned ios[2]; > > + unsigned merges[2]; > > + unsigned ticks[2]; > > unsigned io_ticks; > > unsigned time_in_queue; > > }; Then send me such a comment patch :-) -- Jens Axboe ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [BLOCK] Unify the seperate read/write io stat fields into arrays [not found] <200511021704.jA2H4X4u027306@hera.kernel.org> 2005-11-02 18:38 ` [BLOCK] Unify the seperate read/write io stat fields into arrays Chris Wedgwood @ 2005-11-04 3:22 ` Jeff Garzik 2005-11-04 7:17 ` Jens Axboe 1 sibling, 1 reply; 4+ messages in thread From: Jeff Garzik @ 2005-11-04 3:22 UTC (permalink / raw) To: Linux Kernel Mailing List, Jens Axboe Linux Kernel Mailing List wrote: > tree fe4ce823e638ded151edcb142f28a240860f0d33 > parent d72d904a5367ad4ca3f2c9a2ce8c3a68f0b28bf0 > author Jens Axboe <axboe@suse.de> Tue, 01 Nov 2005 09:26:16 +0100 > committer Jens Axboe <axboe@suse.de> Tue, 01 Nov 2005 09:26:16 +0100 > > [BLOCK] Unify the seperate read/write io stat fields into arrays > > Instead of having ->read_sectors and ->write_sectors, combine the two > into ->sectors[2] and similar for the other fields. This saves a branch > several places in the io path, since we don't have to care for what the > actual io direction is. On my x86-64 box, that's 200 bytes less text in > just the core (not counting the various drivers). > > Signed-off-by: Jens Axboe <axboe@suse.de> > > drivers/block/genhd.c | 29 ++++++++++++++--------------- > drivers/block/ll_rw_blk.c | 40 ++++++++++++---------------------------- > drivers/md/linear.c | 10 +++------- > drivers/md/md.c | 4 ++-- > drivers/md/multipath.c | 10 +++------- > drivers/md/raid0.c | 10 +++------- > drivers/md/raid1.c | 12 ++++-------- > drivers/md/raid10.c | 12 ++++-------- > drivers/md/raid5.c | 10 +++------- > drivers/md/raid6main.c | 12 ++++-------- > fs/partitions/check.c | 7 ++++--- > include/linux/genhd.h | 10 +++++----- > 12 files changed, 61 insertions(+), 105 deletions(-) > > diff --git a/drivers/block/genhd.c b/drivers/block/genhd.c > index 486ce1f..54aec4a 100644 > --- a/drivers/block/genhd.c > +++ b/drivers/block/genhd.c > @@ -391,13 +391,12 @@ static ssize_t disk_stats_read(struct ge > "%8u %8u %8llu %8u " > "%8u %8u %8u" > "\n", > - disk_stat_read(disk, reads), disk_stat_read(disk, read_merges), > - (unsigned long long)disk_stat_read(disk, read_sectors), > - jiffies_to_msecs(disk_stat_read(disk, read_ticks)), > - disk_stat_read(disk, writes), > - disk_stat_read(disk, write_merges), > - (unsigned long long)disk_stat_read(disk, write_sectors), > - jiffies_to_msecs(disk_stat_read(disk, write_ticks)), > + disk_stat_read(disk, ios[0]), disk_stat_read(disk, merges[0]), > + (unsigned long long)disk_stat_read(disk, sectors[0]), > + jiffies_to_msecs(disk_stat_read(disk, ticks[0])), > + disk_stat_read(disk, ios[1]), disk_stat_read(disk, merges[1]), > + (unsigned long long)disk_stat_read(disk, sectors[1]), > + jiffies_to_msecs(disk_stat_read(disk, ticks[1])), > disk->in_flight, > jiffies_to_msecs(disk_stat_read(disk, io_ticks)), > jiffies_to_msecs(disk_stat_read(disk, time_in_queue))); > @@ -583,12 +582,12 @@ static int diskstats_show(struct seq_fil > preempt_enable(); > seq_printf(s, "%4d %4d %s %u %u %llu %u %u %u %llu %u %u %u %u\n", > gp->major, n + gp->first_minor, disk_name(gp, n, buf), > - disk_stat_read(gp, reads), disk_stat_read(gp, read_merges), > - (unsigned long long)disk_stat_read(gp, read_sectors), > - jiffies_to_msecs(disk_stat_read(gp, read_ticks)), > - disk_stat_read(gp, writes), disk_stat_read(gp, write_merges), > - (unsigned long long)disk_stat_read(gp, write_sectors), > - jiffies_to_msecs(disk_stat_read(gp, write_ticks)), > + disk_stat_read(gp, ios[0]), disk_stat_read(gp, merges[0]), > + (unsigned long long)disk_stat_read(gp, sectors[0]), > + jiffies_to_msecs(disk_stat_read(gp, ticks[0])), > + disk_stat_read(gp, ios[1]), disk_stat_read(gp, merges[1]), > + (unsigned long long)disk_stat_read(gp, sectors[1]), > + jiffies_to_msecs(disk_stat_read(gp, ticks[1])), > gp->in_flight, > jiffies_to_msecs(disk_stat_read(gp, io_ticks)), > jiffies_to_msecs(disk_stat_read(gp, time_in_queue))); While the overall patch is OK, the use of magic numbers makes the code less readable. fsck if I know whether "[0]" represents read or write. Please use named constants rather than [0] and [1]. Jeff ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [BLOCK] Unify the seperate read/write io stat fields into arrays 2005-11-04 3:22 ` Jeff Garzik @ 2005-11-04 7:17 ` Jens Axboe 0 siblings, 0 replies; 4+ messages in thread From: Jens Axboe @ 2005-11-04 7:17 UTC (permalink / raw) To: Jeff Garzik; +Cc: Linux Kernel Mailing List On Thu, Nov 03 2005, Jeff Garzik wrote: > Linux Kernel Mailing List wrote: > >tree fe4ce823e638ded151edcb142f28a240860f0d33 > >parent d72d904a5367ad4ca3f2c9a2ce8c3a68f0b28bf0 > >author Jens Axboe <axboe@suse.de> Tue, 01 Nov 2005 09:26:16 +0100 > >committer Jens Axboe <axboe@suse.de> Tue, 01 Nov 2005 09:26:16 +0100 > > > >[BLOCK] Unify the seperate read/write io stat fields into arrays > > > >Instead of having ->read_sectors and ->write_sectors, combine the two > >into ->sectors[2] and similar for the other fields. This saves a branch > >several places in the io path, since we don't have to care for what the > >actual io direction is. On my x86-64 box, that's 200 bytes less text in > >just the core (not counting the various drivers). > > > >Signed-off-by: Jens Axboe <axboe@suse.de> > > > > drivers/block/genhd.c | 29 ++++++++++++++--------------- > > drivers/block/ll_rw_blk.c | 40 ++++++++++++---------------------------- > > drivers/md/linear.c | 10 +++------- > > drivers/md/md.c | 4 ++-- > > drivers/md/multipath.c | 10 +++------- > > drivers/md/raid0.c | 10 +++------- > > drivers/md/raid1.c | 12 ++++-------- > > drivers/md/raid10.c | 12 ++++-------- > > drivers/md/raid5.c | 10 +++------- > > drivers/md/raid6main.c | 12 ++++-------- > > fs/partitions/check.c | 7 ++++--- > > include/linux/genhd.h | 10 +++++----- > > 12 files changed, 61 insertions(+), 105 deletions(-) > > > >diff --git a/drivers/block/genhd.c b/drivers/block/genhd.c > >index 486ce1f..54aec4a 100644 > >--- a/drivers/block/genhd.c > >+++ b/drivers/block/genhd.c > >@@ -391,13 +391,12 @@ static ssize_t disk_stats_read(struct ge > > "%8u %8u %8llu %8u " > > "%8u %8u %8u" > > "\n", > >- disk_stat_read(disk, reads), disk_stat_read(disk, > >read_merges), > >- (unsigned long long)disk_stat_read(disk, read_sectors), > >- jiffies_to_msecs(disk_stat_read(disk, read_ticks)), > >- disk_stat_read(disk, writes), > >- disk_stat_read(disk, write_merges), > >- (unsigned long long)disk_stat_read(disk, write_sectors), > >- jiffies_to_msecs(disk_stat_read(disk, write_ticks)), > >+ disk_stat_read(disk, ios[0]), disk_stat_read(disk, > >merges[0]), > >+ (unsigned long long)disk_stat_read(disk, sectors[0]), > >+ jiffies_to_msecs(disk_stat_read(disk, ticks[0])), > >+ disk_stat_read(disk, ios[1]), disk_stat_read(disk, > >merges[1]), > >+ (unsigned long long)disk_stat_read(disk, sectors[1]), > >+ jiffies_to_msecs(disk_stat_read(disk, ticks[1])), > > disk->in_flight, > > jiffies_to_msecs(disk_stat_read(disk, io_ticks)), > > jiffies_to_msecs(disk_stat_read(disk, time_in_queue))); > >@@ -583,12 +582,12 @@ static int diskstats_show(struct seq_fil > > preempt_enable(); > > seq_printf(s, "%4d %4d %s %u %u %llu %u %u %u %llu %u %u %u %u\n", > > gp->major, n + gp->first_minor, disk_name(gp, n, buf), > >- disk_stat_read(gp, reads), disk_stat_read(gp, read_merges), > >- (unsigned long long)disk_stat_read(gp, read_sectors), > >- jiffies_to_msecs(disk_stat_read(gp, read_ticks)), > >- disk_stat_read(gp, writes), disk_stat_read(gp, write_merges), > >- (unsigned long long)disk_stat_read(gp, write_sectors), > >- jiffies_to_msecs(disk_stat_read(gp, write_ticks)), > >+ disk_stat_read(gp, ios[0]), disk_stat_read(gp, merges[0]), > >+ (unsigned long long)disk_stat_read(gp, sectors[0]), > >+ jiffies_to_msecs(disk_stat_read(gp, ticks[0])), > >+ disk_stat_read(gp, ios[1]), disk_stat_read(gp, merges[1]), > >+ (unsigned long long)disk_stat_read(gp, sectors[1]), > >+ jiffies_to_msecs(disk_stat_read(gp, ticks[1])), > > gp->in_flight, > > jiffies_to_msecs(disk_stat_read(gp, io_ticks)), > > jiffies_to_msecs(disk_stat_read(gp, time_in_queue))); > > While the overall patch is OK, the use of magic numbers makes the code > less readable. fsck if I know whether "[0]" represents read or write. Does it matter, you know that they are reads and writes obviously. But yes, using READ/WRITE would be nicer. I'll queue it up. -- Jens Axboe ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2005-11-04 7:16 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <200511021704.jA2H4X4u027306@hera.kernel.org>
2005-11-02 18:38 ` [BLOCK] Unify the seperate read/write io stat fields into arrays Chris Wedgwood
2005-11-03 7:31 ` Jens Axboe
2005-11-04 3:22 ` Jeff Garzik
2005-11-04 7:17 ` Jens Axboe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox