Add pagevec_for_each_page() macro to iterate over all pages in a
pagevec. Non-trivial part is to keep track of page zone and relock
zone->lru_lock when switching to new zone.
This simplifies functions in mm/swap.c that process pages from pvec in a
batch.
(Patch is for 2.6.10-rc2)
Signed-off-by: Nikita Danilov <[email protected]>
include/linux/pagevec.h | 45 +++++++++++++++++++++++++++++++++++++++++++
mm/swap.c | 50 +++++++++---------------------------------------
2 files changed, 55 insertions(+), 40 deletions(-)
diff -puN include/linux/pagevec.h~pvec-cleanup include/linux/pagevec.h
--- bk-linux/include/linux/pagevec.h~pvec-cleanup 2004-11-21 17:01:02.606535952 +0300
+++ bk-linux-nikita/include/linux/pagevec.h 2004-11-21 17:01:02.611535192 +0300
@@ -83,3 +83,48 @@ static inline void pagevec_lru_add(struc
if (pagevec_count(pvec))
__pagevec_lru_add(pvec);
}
+
+/*
+ * Helper macro for pagevec_for_each_page(). This is called on each
+ * iteration before entering loop body. Checks if we are switching to
+ * the new zone so that ->lru_lock should be re-taken.
+ */
+#define __guardloop(_v, _i, _p, _z) \
+({ \
+ struct zone *pagezone; \
+ \
+ _p = (_v)->pages[i]; \
+ pagezone = page_zone(_p); \
+ \
+ if (pagezone != (_z)) { \
+ if (_z) \
+ spin_unlock_irq(&(_z)->lru_lock); \
+ _z = pagezone; \
+ spin_lock_irq(&(_z)->lru_lock); \
+ } \
+})
+
+/*
+ * Helper macro for pagevec_for_each_page(). This is called after last
+ * iteration to release zone->lru_lock if necessary.
+ */
+#define __postloop(_v, _i, _p, _z) \
+({ \
+ if ((_z) != NULL) \
+ spin_unlock_irq(&(_z)->lru_lock); \
+})
+
+/*
+ * Macro to iterate over all pages in pvec. Body of a loop is invoked with
+ * page's zone ->lru_lock held. This is used by various function in mm/swap.c
+ * to batch per-page operations that whould otherwise had to acquire hot
+ * zone->lru_lock for each page.
+ *
+ * This uses ugly preprocessor magic, but that's what preprocessor is all
+ * about.
+ */
+#define pagevec_for_each_page(_v, _i, _p, _z) \
+for (_i = 0, _z = NULL; \
+ ((_i) < pagevec_count(_v) && (__guardloop(_v, _i, _p, _z), 1)) || \
+ (__postloop(_v, _i, _p, _z), 0); \
+ (_i)++)
diff -puN mm/swap.c~pvec-cleanup mm/swap.c
--- bk-linux/mm/swap.c~pvec-cleanup 2004-11-21 17:01:02.609535496 +0300
+++ bk-linux-nikita/mm/swap.c 2004-11-21 17:01:02.611535192 +0300
@@ -116,19 +116,11 @@ void fastcall activate_page(struct page
static void __pagevec_mark_accessed(struct pagevec *pvec)
{
int i;
- struct zone *zone = NULL;
+ struct zone *zone;
+ struct page *page;
- for (i = 0; i < pagevec_count(pvec); i++) {
- struct page *page = pvec->pages[i];
- struct zone *pagezone = page_zone(page);
-
- if (pagezone != zone) {
- if (zone)
- local_unlock_irq(&zone->lru_lock);
- zone = pagezone;
- local_lock_irq(&zone->lru_lock);
- }
- if (!PageActive(page) && PageReferenced(page) && PageLRU(page)) {
+ pagevec_for_each_page(pvec, i, page, zone) {
+ if (!PageActive(page) && PageReferenced(page) && PageLRU(page)){
del_page_from_inactive_list(zone, page);
SetPageActive(page);
add_page_to_active_list(zone, page);
@@ -138,8 +130,6 @@ static void __pagevec_mark_accessed(stru
SetPageReferenced(page);
}
}
- if (zone)
- local_unlock_irq(&zone->lru_lock);
release_pages(pvec->pages, pvec->nr, pvec->cold);
pagevec_reinit(pvec);
}
@@ -322,25 +312,15 @@ void __pagevec_release_nonlru(struct pag
void __pagevec_lru_add(struct pagevec *pvec)
{
int i;
- struct zone *zone = NULL;
-
- for (i = 0; i < pagevec_count(pvec); i++) {
- struct page *page = pvec->pages[i];
- struct zone *pagezone = page_zone(page);
+ struct page *page;
+ struct zone *zone;
- if (pagezone != zone) {
- if (zone)
- spin_unlock_irq(&zone->lru_lock);
- zone = pagezone;
- spin_lock_irq(&zone->lru_lock);
- }
+ pagevec_for_each_page(pvec, i, page, zone) {
if (TestSetPageLRU(page))
BUG();
ClearPageSkipped(page);
add_page_to_inactive_list(zone, page);
}
- if (zone)
- spin_unlock_irq(&zone->lru_lock);
release_pages(pvec->pages, pvec->nr, pvec->cold);
pagevec_reinit(pvec);
}
@@ -350,26 +330,16 @@ EXPORT_SYMBOL(__pagevec_lru_add);
void __pagevec_lru_add_active(struct pagevec *pvec)
{
int i;
- struct zone *zone = NULL;
-
- for (i = 0; i < pagevec_count(pvec); i++) {
- struct page *page = pvec->pages[i];
- struct zone *pagezone = page_zone(page);
+ struct page *page;
+ struct zone *zone;
- if (pagezone != zone) {
- if (zone)
- spin_unlock_irq(&zone->lru_lock);
- zone = pagezone;
- spin_lock_irq(&zone->lru_lock);
- }
+ pagevec_for_each_page(pvec, i, page, zone) {
if (TestSetPageLRU(page))
BUG();
if (TestSetPageActive(page))
BUG();
add_page_to_active_list(zone, page);
}
- if (zone)
- spin_unlock_irq(&zone->lru_lock);
release_pages(pvec->pages, pvec->nr, pvec->cold);
pagevec_reinit(pvec);
}
_
Nikita Danilov <[email protected]> wrote:
>
> +#define pagevec_for_each_page(_v, _i, _p, _z) \
> +for (_i = 0, _z = NULL; \
> + ((_i) < pagevec_count(_v) && (__guardloop(_v, _i, _p, _z), 1)) || \
> + (__postloop(_v, _i, _p, _z), 0); \
> + (_i)++)
Sorry, this looks more like a dirtyup to me ;)
Add pagevec_for_each_page() macro to iterate over all pages in a
pagevec. Non-trivial part is to keep track of page zone and relock
zone->lru_lock when switching to new zone.
This simplifies functions in mm/swap.c that process pages from pvec in a
batch.
Signed-off-by: Nikita Danilov <[email protected]>
include/linux/pagevec.h | 14 ++++++++
mm/swap.c | 83 ++++++++++++++++++++++++------------------------
2 files changed, 57 insertions(+), 40 deletions(-)
diff -puN include/linux/pagevec.h~pvec-cleanup include/linux/pagevec.h
--- bk-linux/include/linux/pagevec.h~pvec-cleanup 2004-11-21 18:59:59.000000000 +0300
+++ bk-linux-nikita/include/linux/pagevec.h 2004-11-22 01:13:54.000000000 +0300
@@ -83,3 +83,17 @@ static inline void pagevec_lru_add(struc
if (pagevec_count(pvec))
__pagevec_lru_add(pvec);
}
+
+struct page *__pagevec_loop_next(struct pagevec *pvec,
+ struct zone **zone, int *i);
+
+/*
+ * Macro to iterate over all pages in pvec. Body of a loop is invoked with
+ * page's zone ->lru_lock held. This is used by various function in mm/swap.c
+ * to batch per-page operations that whould otherwise had to acquire hot
+ * zone->lru_lock for each page.
+ */
+#define pagevec_for_each_page(pvec, i, page, zone) \
+ for ((i) = 0, (zone) = NULL; \
+ ((page) = __pagevec_loop_next((pvec), &(zone), &(i))) != NULL; \
+ ++ (i))
diff -puN mm/swap.c~pvec-cleanup mm/swap.c
--- bk-linux/mm/swap.c~pvec-cleanup 2004-11-21 18:59:59.000000000 +0300
+++ bk-linux-nikita/mm/swap.c 2004-11-22 01:19:17.758782008 +0300
@@ -55,6 +55,39 @@ EXPORT_SYMBOL(put_page);
#endif
/*
+ * Helper function for include/linux/pagevec.h:pagevec_for_each_page macro.
+ *
+ * Returns @i-th page from @pvec, with page zone locked. @zone points to
+ * previously locked zone, it's updated (and zone is re-locked) if zone is
+ * changed.
+ */
+struct page *__pagevec_loop_next(struct pagevec *pvec,
+ struct zone **zone, int *i)
+{
+ struct page *page;
+ struct zone *next_zone;
+ struct zone *prev_zone;
+
+ prev_zone = *zone;
+ if (*i < pagevec_count(pvec)) {
+
+ page = pvec->pages[*i];
+ next_zone = page_zone(page);
+ if (next_zone != prev_zone) {
+ if (prev_zone != NULL)
+ spin_unlock_irq(&prev_zone->lru_lock);
+ *zone = next_zone;
+ spin_lock_irq(&next_zone->lru_lock);
+ }
+ } else {
+ page = NULL;
+ if (prev_zone != NULL)
+ spin_unlock_irq(&prev_zone->lru_lock);
+ }
+ return page;
+}
+
+/*
* Writeback is about to end against a page which has been marked for immediate
* reclaim. If it still appears to be reclaimable, move it to the tail of the
* inactive list. The page still has PageWriteback set, which will pin it.
@@ -116,19 +149,11 @@ void fastcall activate_page(struct page
static void __pagevec_mark_accessed(struct pagevec *pvec)
{
int i;
- struct zone *zone = NULL;
-
- for (i = 0; i < pagevec_count(pvec); i++) {
- struct page *page = pvec->pages[i];
- struct zone *pagezone = page_zone(page);
+ struct zone *zone;
+ struct page *page;
- if (pagezone != zone) {
- if (zone)
- local_unlock_irq(&zone->lru_lock);
- zone = pagezone;
- local_lock_irq(&zone->lru_lock);
- }
- if (!PageActive(page) && PageReferenced(page) && PageLRU(page)) {
+ pagevec_for_each_page(pvec, i, page, zone) {
+ if (!PageActive(page) && PageReferenced(page) && PageLRU(page)){
del_page_from_inactive_list(zone, page);
SetPageActive(page);
add_page_to_active_list(zone, page);
@@ -138,8 +163,6 @@ static void __pagevec_mark_accessed(stru
SetPageReferenced(page);
}
}
- if (zone)
- local_unlock_irq(&zone->lru_lock);
release_pages(pvec->pages, pvec->nr, pvec->cold);
pagevec_reinit(pvec);
}
@@ -322,25 +345,15 @@ void __pagevec_release_nonlru(struct pag
void __pagevec_lru_add(struct pagevec *pvec)
{
int i;
- struct zone *zone = NULL;
-
- for (i = 0; i < pagevec_count(pvec); i++) {
- struct page *page = pvec->pages[i];
- struct zone *pagezone = page_zone(page);
+ struct page *page;
+ struct zone *zone;
- if (pagezone != zone) {
- if (zone)
- spin_unlock_irq(&zone->lru_lock);
- zone = pagezone;
- spin_lock_irq(&zone->lru_lock);
- }
+ pagevec_for_each_page(pvec, i, page, zone) {
if (TestSetPageLRU(page))
BUG();
ClearPageSkipped(page);
add_page_to_inactive_list(zone, page);
}
- if (zone)
- spin_unlock_irq(&zone->lru_lock);
release_pages(pvec->pages, pvec->nr, pvec->cold);
pagevec_reinit(pvec);
}
@@ -350,26 +363,16 @@ EXPORT_SYMBOL(__pagevec_lru_add);
void __pagevec_lru_add_active(struct pagevec *pvec)
{
int i;
- struct zone *zone = NULL;
-
- for (i = 0; i < pagevec_count(pvec); i++) {
- struct page *page = pvec->pages[i];
- struct zone *pagezone = page_zone(page);
+ struct page *page;
+ struct zone *zone;
- if (pagezone != zone) {
- if (zone)
- spin_unlock_irq(&zone->lru_lock);
- zone = pagezone;
- spin_lock_irq(&zone->lru_lock);
- }
+ pagevec_for_each_page(pvec, i, page, zone) {
if (TestSetPageLRU(page))
BUG();
if (TestSetPageActive(page))
BUG();
add_page_to_active_list(zone, page);
}
- if (zone)
- spin_unlock_irq(&zone->lru_lock);
release_pages(pvec->pages, pvec->nr, pvec->cold);
pagevec_reinit(pvec);
}
_
On Mon, 22 Nov 2004, Nikita Danilov wrote:
> Andrew Morton writes:
> >
> > Sorry, this looks more like a dirtyup to me ;)
>
> Don't tell me you are not great fan on comma operator abuse. :)
>
> Anyway, idea is that by hiding complexity it loop macro, we get rid of a
> maze of pvec-loops in swap.c all alike.
>
> Attached is next, more typeful variant. Compilebootentested.
You're scaring me, Nikita. Those loops in mm/swap.c are easy to follow,
whyever do you want to obfuscate them with your own macro maze?
Ingenious for_each macros make sense where it's an idiom which is going
to be useful to many across the tree; but these are just a few instances
in a single source file.
Please find a better outlet for your talents!
Hugh
Hugh Dickins writes:
> On Mon, 22 Nov 2004, Nikita Danilov wrote:
> > Andrew Morton writes:
> > >
> > > Sorry, this looks more like a dirtyup to me ;)
> >
> > Don't tell me you are not great fan on comma operator abuse. :)
> >
> > Anyway, idea is that by hiding complexity it loop macro, we get rid of a
> > maze of pvec-loops in swap.c all alike.
> >
> > Attached is next, more typeful variant. Compilebootentested.
>
> You're scaring me, Nikita. Those loops in mm/swap.c are easy to follow,
> whyever do you want to obfuscate them with your own macro maze?
Because my intellectual capacity is limited, and it has little room left
for analyzing _multiple_ zone-lock-tracking sequences. It seems cleaner
to do this once, but as you put is elsewhere "it is a matter of personal
taste".
Besides, after I was recently subjected to looking at BSD kernel code, I
have morbid fear or any kind of mostly similar chunks of code
cut-n-pasted and then modified independently.
>
> Ingenious for_each macros make sense where it's an idiom which is going
> to be useful to many across the tree; but these are just a few instances
> in a single source file.
Yes, this makes sense.
>
> Please find a better outlet for your talents!
Heh, you know, from a few VM patches I have in the queue, I started
submitting least controversial ones. :)
>
> Hugh
Nikita.