* 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