Messages in this thread Patch in this message | ![/](/images/icornerl.gif) | | Date | Mon, 19 Mar 2007 18:06:02 -0700 (PDT) | From | Christoph Lameter <> | Subject | Re: [QUICKLIST 1/5] Quicklists for page table pages V3 |
| |
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| ![\](/images/icornerr.gif) |