2006-12-05 05:21:35

by Chen, Kenneth W

Subject: [patch] add an iterator index in struct pagevec

pagevec is never expected to be more than PAGEVEC_SIZE, I think a
unsigned char is enough to count them. This patch makes nr, cold
to be unsigned char and also adds an iterator index. With that,
the size can be even bumped up by 1 to 15.

Signed-off-by: Ken Chen <[email protected]>


diff -Nurp linux-2.6.19/include/linux/pagevec.h linux-2.6.19.ken/include/linux/pagevec.h
--- linux-2.6.19/include/linux/pagevec.h 2006-11-29 13:57:37.000000000 -0800
+++ linux-2.6.19.ken/include/linux/pagevec.h 2006-12-04 19:18:21.000000000 -0800
@@ -8,15 +8,16 @@
#ifndef _LINUX_PAGEVEC_H
#define _LINUX_PAGEVEC_H

-/* 14 pointers + two long's align the pagevec structure to a power of two */
-#define PAGEVEC_SIZE 14
+/* 15 pointers + 3 char's align the pagevec structure to a power of two */
+#define PAGEVEC_SIZE 15

struct page;
struct address_space;

struct pagevec {
- unsigned long nr;
- unsigned long cold;
+ unsigned char nr;
+ unsigned char cold;
+ unsigned char idx;
struct page *pages[PAGEVEC_SIZE];
};

2006-12-05 05:45:32

by Andrew Morton

Subject: Re: [patch] add an iterator index in struct pagevec

On Mon, 4 Dec 2006 21:21:31 -0800
"Chen, Kenneth W" <[email protected]> wrote:

> pagevec is never expected to be more than PAGEVEC_SIZE, I think a
> unsigned char is enough to count them. This patch makes nr, cold
> to be unsigned char

Is that on the right side of the speed/space tradeoff?

> and also adds an iterator index. With that,
> the size can be even bumped up by 1 to 15.
>
> Signed-off-by: Ken Chen <[email protected]>
>
>
> diff -Nurp linux-2.6.19/include/linux/pagevec.h linux-2.6.19.ken/include/linux/pagevec.h
> --- linux-2.6.19/include/linux/pagevec.h 2006-11-29 13:57:37.000000000 -0800
> +++ linux-2.6.19.ken/include/linux/pagevec.h 2006-12-04 19:18:21.000000000 -0800
> @@ -8,15 +8,16 @@
> #ifndef _LINUX_PAGEVEC_H
> #define _LINUX_PAGEVEC_H
>
> -/* 14 pointers + two long's align the pagevec structure to a power of two */
> -#define PAGEVEC_SIZE 14
> +/* 15 pointers + 3 char's align the pagevec structure to a power of two */
> +#define PAGEVEC_SIZE 15
>
> struct page;
> struct address_space;
>
> struct pagevec {
> - unsigned long nr;
> - unsigned long cold;
> + unsigned char nr;
> + unsigned char cold;
> + unsigned char idx;
> struct page *pages[PAGEVEC_SIZE];
> };
>

I'd have thought that pagevec_init() would want to be involved in this, no?

I must say I'm a bit skeptical about the need for this. But I haven't
looked closely at the blockdev-specific dio code yet.

2006-12-05 06:21:57

by Chen, Kenneth W

Subject: RE: [patch] add an iterator index in struct pagevec

Andrew Morton wrote on Monday, December 04, 2006 9:45 PM
> On Mon, 4 Dec 2006 21:21:31 -0800
> "Chen, Kenneth W" <[email protected]> wrote:
>
> > pagevec is never expected to be more than PAGEVEC_SIZE, I think a
> > unsigned char is enough to count them. This patch makes nr, cold
> > to be unsigned char
>
> Is that on the right side of the speed/space tradeoff?

I haven't measured speed. Size wise, making them char shrinks vmlinux
text size by 112 bytes on x86_64 (using default config option).


> I must say I'm a bit skeptical about the need for this. But I haven't
> looked closely at the blockdev-specific dio code yet.

It was suggested to declare another struct that embeds pagevec to perform
iteration. But I prefer to have pagevec having the capability, it is
more compact this way.

It would be nice if you can review blockdev-specific dio code. I would
appreciate it very much.

- Ken