From: Miklos Szeredi <[email protected]>
Clean up messy conditional calling of test_clear_page_writeback() from
both rotate_reclaimable_page() and end_page_writeback().
The only user of rotate_reclaimable_page() is end_page_writeback() so
this is OK.
Signed-off-by: Miklos Szeredi <[email protected]>
---
include/linux/swap.h | 2 +-
mm/filemap.c | 10 ++++++----
mm/swap.c | 37 ++++++++++++-------------------------
3 files changed, 19 insertions(+), 30 deletions(-)
Index: linux/include/linux/swap.h
===================================================================
--- linux.orig/include/linux/swap.h 2008-03-17 18:24:13.000000000 +0100
+++ linux/include/linux/swap.h 2008-03-17 18:25:38.000000000 +0100
@@ -177,7 +177,7 @@ extern void activate_page(struct page *)
extern void mark_page_accessed(struct page *);
extern void lru_add_drain(void);
extern int lru_add_drain_all(void);
-extern int rotate_reclaimable_page(struct page *page);
+extern void rotate_reclaimable_page(struct page *page);
extern void swap_setup(void);
/* linux/mm/vmscan.c */
Index: linux/mm/filemap.c
===================================================================
--- linux.orig/mm/filemap.c 2008-03-17 18:24:13.000000000 +0100
+++ linux/mm/filemap.c 2008-03-17 18:25:38.000000000 +0100
@@ -577,10 +577,12 @@ EXPORT_SYMBOL(unlock_page);
*/
void end_page_writeback(struct page *page)
{
- if (!TestClearPageReclaim(page) || rotate_reclaimable_page(page)) {
- if (!test_clear_page_writeback(page))
- BUG();
- }
+ if (TestClearPageReclaim(page))
+ rotate_reclaimable_page(page);
+
+ if (!test_clear_page_writeback(page))
+ BUG();
+
smp_mb__after_clear_bit();
wake_up_page(page, PG_writeback);
}
Index: linux/mm/swap.c
===================================================================
--- linux.orig/mm/swap.c 2008-03-17 18:24:13.000000000 +0100
+++ linux/mm/swap.c 2008-03-17 18:25:38.000000000 +0100
@@ -133,34 +133,21 @@ static void pagevec_move_tail(struct pag
* 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.
- *
- * Returns zero if it cleared PG_writeback.
*/
-int rotate_reclaimable_page(struct page *page)
+void rotate_reclaimable_page(struct page *page)
{
- struct pagevec *pvec;
- unsigned long flags;
-
- if (PageLocked(page))
- return 1;
- if (PageDirty(page))
- return 1;
- if (PageActive(page))
- return 1;
- if (!PageLRU(page))
- return 1;
-
- page_cache_get(page);
- local_irq_save(flags);
- pvec = &__get_cpu_var(lru_rotate_pvecs);
- if (!pagevec_add(pvec, page))
- pagevec_move_tail(pvec);
- local_irq_restore(flags);
-
- if (!test_clear_page_writeback(page))
- BUG();
+ if (!PageLocked(page) && !PageDirty(page) && !PageActive(page) &&
+ PageLRU(page)) {
+ struct pagevec *pvec;
+ unsigned long flags;
- return 0;
+ page_cache_get(page);
+ local_irq_save(flags);
+ pvec = &__get_cpu_var(lru_rotate_pvecs);
+ if (!pagevec_add(pvec, page))
+ pagevec_move_tail(pvec);
+ local_irq_restore(flags);
+ }
}
/*
--
On Mon, 2008-03-17 at 20:19 +0100, Miklos Szeredi wrote:
> plain text document attachment (rotate_reclaimable_page_cleanup.patch)
> From: Miklos Szeredi <[email protected]>
>
> Clean up messy conditional calling of test_clear_page_writeback() from
> both rotate_reclaimable_page() and end_page_writeback().
> -int rotate_reclaimable_page(struct page *page)
> +void rotate_reclaimable_page(struct page *page)
> {
> - struct pagevec *pvec;
> - unsigned long flags;
> -
> - if (PageLocked(page))
> - return 1;
> - if (PageDirty(page))
> - return 1;
> - if (PageActive(page))
> - return 1;
> - if (!PageLRU(page))
> - return 1;
Might be me, but I find the above easier to read than
> + if (!PageLocked(page) && !PageDirty(page) && !PageActive(page) &&
> + PageLRU(page)) {
>
> > -int rotate_reclaimable_page(struct page *page)
> > +void rotate_reclaimable_page(struct page *page)
> > {
> > - struct pagevec *pvec;
> > - unsigned long flags;
> > -
> > - if (PageLocked(page))
> > - return 1;
> > - if (PageDirty(page))
> > - return 1;
> > - if (PageActive(page))
> > - return 1;
> > - if (!PageLRU(page))
> > - return 1;
>
> Might be me, but I find the above easier to read than
>
> > + if (!PageLocked(page) && !PageDirty(page) && !PageActive(page) &&
> > + PageLRU(page)) {
> >
Matter of taste, returning from a middle of a function is generally to
be avoided (unless not). Anyway, this is just a side effect of the
main cleanup, so I think I'm entitled to choose the style I prefer ;)
Miklos
On Tue, 18 Mar 2008 12:56:34 +0100 Miklos Szeredi <[email protected]> wrote:
> > > -int rotate_reclaimable_page(struct page *page)
> > > +void rotate_reclaimable_page(struct page *page)
> > > {
> > > - struct pagevec *pvec;
> > > - unsigned long flags;
> > > -
> > > - if (PageLocked(page))
> > > - return 1;
> > > - if (PageDirty(page))
> > > - return 1;
> > > - if (PageActive(page))
> > > - return 1;
> > > - if (!PageLRU(page))
> > > - return 1;
> >
> > Might be me, but I find the above easier to read than
Me too, but (believe it or not) sometimes I will eschew comment ;)
> > > + if (!PageLocked(page) && !PageDirty(page) && !PageActive(page) &&
> > > + PageLRU(page)) {
> > >
>
> Matter of taste, returning from a middle of a function is generally to
> be avoided (unless not).
Avoiding multiple returns is more than a matter of taste: the practice is a
source of code bloat, resource leaks and locking errors.
But we do do it quite commonly at the start of the function when checking the
arguments, before the function has actually altered anything.