Re: [QUICKLIST 1/5] Quicklists for page table pages V3

[Christoph Lameter]

On Mon, 19 Mar 2007, Andrew Morton wrote:

> > +
> > +#ifdef CONFIG_QUICKLIST
> > +
> > +#ifndef CONFIG_NR_QUICK
> > +#define CONFIG_NR_QUICK 1
> > +#endif
> 
> No, please don't define config items like this.  Do it in Kconfig.

They can be set up in the arch specific Kconfig. Ok. I moved the 
#ifndef .. #endif into mm/Kconfig.

> These guys seem to have multiple callsites for ia64 at least and probably
> would benefit from being uninlined.

Then they would no longer be optimizable. Right now one can compile out 
the constructor / destructor support and provide a constant list number 
as well as constant gfp masks. This can be very small and benefit 
tremendously from inlining.

Many arches do not need some features and there are only a few call 
sites.

> > +void quicklist_check(int nr, void (*dtor)(void *));
> > +unsigned long quicklist_total_size(void);
> > +
> > +#else
> > +void quicklist_check(int nr, void (*dtor)(void *))
> > +{
> > +}
> > +
> > +unsigned long quicklist_total_size(void)
> > +{
> > +	return 0;
> > +}
> > +#endif
> 
> That obviouslty won't link and wasn't tested.  Making these static inline
> will help.

Hmmm... We could drop these conmpletely. If an arch does not use 
quicklists then they should not be calling these.

> > +#include <linux/module.h>
> > +#include <linux/quicklist.h>
> > +
> > +DEFINE_PER_CPU(struct quicklist, quicklist)[CONFIG_NR_QUICK];
> 
> If we uninline those big inlines, this can perhaps be made static.

Yeah but we want the inlines.

> 
> > +#define MIN_PAGES		25
> > +#define MAX_FREES_PER_PASS	16
> > +#define FRACTION_OF_NODE_MEM	16
> 
> Are these constants optimal for all architectures?

I added them as parameters to quicklist_trim so that an arch 
can specify their own settings.

> > +	return min(pages_to_free, (long)MAX_FREES_PER_PASS);
> > +}
> 
> min_t and max_t are the standard way of avoiding that warning.  Or stick a
> UL on the constants (which is probably better).

We do not need those since the constants are now parameters.

> 
> > +void quicklist_check(int nr, void (*dtor)(void *))
> > +{
> > +	long pages_to_free;
> > +	struct quicklist *q;
> > +
> > +	q = &get_cpu_var(quicklist)[nr];
> > +	if (q->nr_pages > MIN_PAGES) {
> > +		pages_to_free = min_pages_to_free(q);
> > +
> > +		while (pages_to_free > 0) {
> > +			void *p = quicklist_alloc(nr, 0, NULL);
> > +
> > +			if (dtor)
> > +				dtor(p);
> > +			free_page((unsigned long)p);
> > +			pages_to_free--;
> > +		}
> > +	}
> > +	put_cpu_var(quicklist);
> > +}
> 
> The use of a literal 0 as a gfp_t is a bit ugly.  I assume that we don't
> care because we should never actually call into the page allocator for this
> caller.  But it's not terribly clear because there is no commentary
> describing what this function is supposed to do.

Right. Will add comments.

> The name foo_check() is unfortunate: it implies that the function checks
> something (ie: has no side-effects).  But this function _does_ change
> things and perhaps should be called quicklist_trim() or something like
> that.

Tradition. Dave initially named it check_pgt_cache it seems.
 
> This function lacks any commentary, but I was able to work it out.  I
> think.  Some nice comments would be, umm, nice.

ok. Here is a fixup patch:

Index: linux-2.6.21-rc3-mm2/include/linux/quicklist.h
===================================================================
--- linux-2.6.21-rc3-mm2.orig/include/linux/quicklist.h	2007-03-19 17:41:42.000000000 -0700
+++ linux-2.6.21-rc3-mm2/include/linux/quicklist.h	2007-03-19 17:47:34.000000000 -0700
@@ -13,10 +13,6 @@
 
 #ifdef CONFIG_QUICKLIST
 
-#ifndef CONFIG_NR_QUICK
-#define CONFIG_NR_QUICK 1
-#endif
-
 struct quicklist {
 	void *page;
 	int nr_pages;
@@ -77,18 +73,11 @@ static inline void quicklist_free(int nr
 	put_cpu_var(quicklist);
 }
 
-void quicklist_check(int nr, void (*dtor)(void *));
-unsigned long quicklist_total_size(void);
+void quicklist_trim(int nr, void (*dtor)(void *),
+	unsigned long min_pages, unsigned long max_free);
 
-#else
-void quicklist_check(int nr, void (*dtor)(void *))
-{
-}
+unsigned long quicklist_total_size(void);
 
-unsigned long quicklist_total_size(void)
-{
-	return 0;
-}
 #endif
 
 #endif /* LINUX_QUICKLIST_H */
Index: linux-2.6.21-rc3-mm2/mm/Kconfig
===================================================================
--- linux-2.6.21-rc3-mm2.orig/mm/Kconfig	2007-03-19 17:41:42.000000000 -0700
+++ linux-2.6.21-rc3-mm2/mm/Kconfig	2007-03-19 17:42:49.000000000 -0700
@@ -220,3 +220,7 @@ config DEBUG_READAHEAD
 
 	  Say N for production servers.
 
+config NR_QUICK
+	depends on QUICKLIST
+	default 1
+
Index: linux-2.6.21-rc3-mm2/mm/quicklist.c
===================================================================
--- linux-2.6.21-rc3-mm2.orig/mm/quicklist.c	2007-03-19 17:41:42.000000000 -0700
+++ linux-2.6.21-rc3-mm2/mm/quicklist.c	2007-03-19 17:53:45.000000000 -0700
@@ -21,39 +21,46 @@
 
 DEFINE_PER_CPU(struct quicklist, quicklist)[CONFIG_NR_QUICK];
 
-#define MIN_PAGES		25
-#define MAX_FREES_PER_PASS	16
 #define FRACTION_OF_NODE_MEM	16
 
-static unsigned long max_pages(void)
+static unsigned long max_pages(unsigned long min_pages)
 {
 	unsigned long node_free_pages, max;
 
 	node_free_pages = node_page_state(numa_node_id(),
 			NR_FREE_PAGES);
 	max = node_free_pages / FRACTION_OF_NODE_MEM;
-	return max(max, (unsigned long)MIN_PAGES);
+	return max(max, min_pages);
 }
 
-static long min_pages_to_free(struct quicklist *q)
+static long min_pages_to_free(struct quicklist *q,
+	unsigned long min_pages, long max_free)
 {
 	long pages_to_free;
 
-	pages_to_free = q->nr_pages - max_pages();
+	pages_to_free = q->nr_pages - max_pages(min_pages);
 
-	return min(pages_to_free, (long)MAX_FREES_PER_PASS);
+	return min(pages_to_free, max_free);
 }
 
-void quicklist_check(int nr, void (*dtor)(void *))
+/*
+ * Trim down the number of pages in the quicklist
+ */
+void quicklist_trim(int nr, void (*dtor)(void *),
+	unsigned long min_pages, unsigned long max_free)
 {
 	long pages_to_free;
 	struct quicklist *q;
 
 	q = &get_cpu_var(quicklist)[nr];
-	if (q->nr_pages > MIN_PAGES) {
-		pages_to_free = min_pages_to_free(q);
+	if (q->nr_pages > min_pages) {
+		pages_to_free = min_pages_to_free(q, min_pages, max_free);
 
 		while (pages_to_free > 0) {
+			/*
+			 * We pass a gfp_t of 0 to quicklist_alloc here
+			 * because we will never call into the page allocator.
+			 */
 			void *p = quicklist_alloc(nr, 0, NULL);
 
 			if (dtor)
Index: linux-2.6.21-rc3-mm2/arch/i386/mm/pgtable.c
===================================================================
--- linux-2.6.21-rc3-mm2.orig/arch/i386/mm/pgtable.c	2007-03-19 17:42:44.000000000 -0700
+++ linux-2.6.21-rc3-mm2/arch/i386/mm/pgtable.c	2007-03-19 17:42:49.000000000 -0700
@@ -299,6 +299,6 @@ void pgd_free(pgd_t *pgd)
 
 void check_pgt_cache(void)
 {
-	quicklist_check(QUICK_PGD, pgd_dtor);
-	quicklist_check(QUICK_PMD, NULL);
+	quicklist_trim(QUICK_PGD, pgd_dtor, 25, 16);
+	quicklist_trim(QUICK_PMD, NULL, 25, 16);
 }
Index: linux-2.6.21-rc3-mm2/include/asm-x86_64/pgalloc.h
===================================================================
--- linux-2.6.21-rc3-mm2.orig/include/asm-x86_64/pgalloc.h	2007-03-19 17:42:46.000000000 -0700
+++ linux-2.6.21-rc3-mm2/include/asm-x86_64/pgalloc.h	2007-03-19 17:42:49.000000000 -0700
@@ -121,7 +121,7 @@ static inline void pte_free(struct page 
 
 static inline void check_pgt_cache(void)
 {
-	quicklist_check(QUICK_PGD, pgd_dtor);
-	quicklist_check(QUICK_PT, NULL);
+	quicklist_trim(QUICK_PGD, pgd_dtor, 25, 16);
+	quicklist_trim(QUICK_PT, NULL, 25, 16);
 }
 #endif /* _X86_64_PGALLOC_H */
Index: linux-2.6.21-rc3-mm2/include/asm-sparc64/pgalloc.h
===================================================================
--- linux-2.6.21-rc3-mm2.orig/include/asm-sparc64/pgalloc.h	2007-03-19 17:42:47.000000000 -0700
+++ linux-2.6.21-rc3-mm2/include/asm-sparc64/pgalloc.h	2007-03-19 17:42:49.000000000 -0700
@@ -67,7 +67,7 @@ static inline void pte_free(struct page 
 
 static inline void check_pgt_cache(void)
 {
-	quicklist_check(0, NULL);
+	quicklist_trim(0, NULL, 25, 16);
 }
 
 #endif /* _SPARC64_PGALLOC_H */
Index: linux-2.6.21-rc3-mm2/include/asm-ia64/pgalloc.h
===================================================================
--- linux-2.6.21-rc3-mm2.orig/include/asm-ia64/pgalloc.h	2007-03-19 17:42:43.000000000 -0700
+++ linux-2.6.21-rc3-mm2/include/asm-ia64/pgalloc.h	2007-03-19 17:42:59.000000000 -0700
@@ -106,7 +106,7 @@ static inline void pte_free_kernel(pte_t
 
 static inline void check_pgt_cache(void)
 {
-	quicklist_check(0, NULL);
+	quicklist_trim(0, NULL, 25, 16);
 }
 
 #define __pte_free_tlb(tlb, pte)	pte_free(pte)


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/