Re: [PATCHv2 3/4] mm: pack compound_dtor and compound_order into one word in struct page
On Tue, Aug 18, 2015 at 06:05:31PM +0200, Michal Hocko wrote:
> On Mon 17-08-15 18:09:04, Kirill A. Shutemov wrote:
> [...]
> > +/* Keep the enum in sync with compound_page_dtors array in mm/page_alloc.c
> > */
> > +enum {
> > + NULL_COMPOUND_DTOR,
> > + COMPOUND_PAGE_DTOR,
> > + HUGETLB_PAGE_DTOR,
> > + NR_COMPOUND_DTORS,
> > +};
> [...]
> > +static void free_compound_page(struct page *page);
> > +compound_page_dtor * const compound_page_dtors[] = {
> > + NULL,
> > + free_compound_page,
> > + free_huge_page,
> > +};
> > +
>
> Both need ifdef CONFIG_HUGETLB_PAGE as my compile test batter just found
> out.
I'll fix that.
--
Kirill A. Shutemov
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv2 3/4] mm: pack compound_dtor and compound_order into one word in struct page
On Tue, Aug 18, 2015 at 05:43:00PM +0200, Michal Hocko wrote:
> On Mon 17-08-15 18:09:04, Kirill A. Shutemov wrote:
> > The patch halves space occupied by compound_dtor and compound_order in
> > struct page.
> >
> > For compound_order, it's trivial long -> int/short conversion.
> >
> > For get_compound_page_dtor(), we now use hardcoded table for destructor
> > lookup and store its index in the struct page instead of direct pointer
> > to destructor. It shouldn't be a big trouble to maintain the table: we
> > have only two destructor and NULL currently.
> >
> > This patch free up one word in tail pages for reuse. This is preparation
> > for the next patch.
> >
> > Signed-off-by: Kirill A. Shutemov
>
> Reviewed-by: Michal Hocko
>
> [...]
> > @@ -145,8 +143,13 @@ struct page {
> > */
> > /* First tail page of compound page */
> > struct {
> > - compound_page_dtor *compound_dtor;
> > - unsigned long compound_order;
> > +#ifdef CONFIG_64BIT
> > + unsigned int compound_dtor;
> > + unsigned int compound_order;
> > +#else
> > + unsigned short int compound_dtor;
> > + unsigned short int compound_order;
> > +#endif
> > };
>
> Why do we need this ifdef? We can go with short for both 32b and 64b
> AFAICS.
My assumption was that operations on ints can be faster on some
[micro]arhictectures. I'm not sure if it's ever true.
> We do not use compound_order for anything else than the order, right?
Right.
> While I am looking at this, it seems we are jugling with type for order
> quite a lot - int, unsing int and even unsigned long.
Yeah. It's mess. I'll check if I can fix anything of it in v3.
--
Kirill A. Shutemov
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv2 3/4] mm: pack compound_dtor and compound_order into one word in struct page
On Mon 17-08-15 18:09:04, Kirill A. Shutemov wrote:
[...]
> +/* Keep the enum in sync with compound_page_dtors array in mm/page_alloc.c */
> +enum {
> + NULL_COMPOUND_DTOR,
> + COMPOUND_PAGE_DTOR,
> + HUGETLB_PAGE_DTOR,
> + NR_COMPOUND_DTORS,
> +};
[...]
> +static void free_compound_page(struct page *page);
> +compound_page_dtor * const compound_page_dtors[] = {
> + NULL,
> + free_compound_page,
> + free_huge_page,
> +};
> +
Both need ifdef CONFIG_HUGETLB_PAGE as my compile test batter just found
out.
--
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv2 3/4] mm: pack compound_dtor and compound_order into one word in struct page
On Mon 17-08-15 18:09:04, Kirill A. Shutemov wrote:
> The patch halves space occupied by compound_dtor and compound_order in
> struct page.
>
> For compound_order, it's trivial long -> int/short conversion.
>
> For get_compound_page_dtor(), we now use hardcoded table for destructor
> lookup and store its index in the struct page instead of direct pointer
> to destructor. It shouldn't be a big trouble to maintain the table: we
> have only two destructor and NULL currently.
>
> This patch free up one word in tail pages for reuse. This is preparation
> for the next patch.
>
> Signed-off-by: Kirill A. Shutemov
Reviewed-by: Michal Hocko
[...]
> @@ -145,8 +143,13 @@ struct page {
>*/
> /* First tail page of compound page */
> struct {
> - compound_page_dtor *compound_dtor;
> - unsigned long compound_order;
> +#ifdef CONFIG_64BIT
> + unsigned int compound_dtor;
> + unsigned int compound_order;
> +#else
> + unsigned short int compound_dtor;
> + unsigned short int compound_order;
> +#endif
> };
Why do we need this ifdef? We can go with short for both 32b and 64b
AFAICS. We do not use compound_order for anything else than the order,
right?
While I am looking at this, it seems we are jugling with type for order
quite a lot - int, unsing int and even unsigned long.
--
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv2 3/4] mm: pack compound_dtor and compound_order into one word in struct page
On Mon, 17 Aug 2015, Kirill A. Shutemov wrote:
> The patch halves space occupied by compound_dtor and compound_order in
> struct page.
>
> For compound_order, it's trivial long -> int/short conversion.
>
> For get_compound_page_dtor(), we now use hardcoded table for destructor
> lookup and store its index in the struct page instead of direct pointer
> to destructor. It shouldn't be a big trouble to maintain the table: we
> have only two destructor and NULL currently.
>
> This patch free up one word in tail pages for reuse. This is preparation
> for the next patch.
>
> Signed-off-by: Kirill A. Shutemov
Well, yes, that is one way of doing it. But I'd have thought the time
for complicating it, instead of simplifying it with direct calls,
would be when someone adds another destructor. Up to Andrew.
> ---
> include/linux/mm.h | 22 +-
> include/linux/mm_types.h | 11 +++
> mm/hugetlb.c | 8
> mm/page_alloc.c | 9 -
> 4 files changed, 36 insertions(+), 14 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 2e872f92dbac..9c21bbb8875a 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -575,18 +575,30 @@ int split_free_page(struct page *page);
> /*
> * Compound pages have a destructor function. Provide a
> * prototype for that function and accessor functions.
> - * These are _only_ valid on the head of a PG_compound page.
> + * These are _only_ valid on the head of a compound page.
> */
> +typedef void compound_page_dtor(struct page *);
> +
> +/* Keep the enum in sync with compound_page_dtors array in mm/page_alloc.c */
> +enum {
> + NULL_COMPOUND_DTOR,
> + COMPOUND_PAGE_DTOR,
> + HUGETLB_PAGE_DTOR,
> + NR_COMPOUND_DTORS,
> +};
> +extern compound_page_dtor * const compound_page_dtors[];
>
> static inline void set_compound_page_dtor(struct page *page,
> - compound_page_dtor *dtor)
> + unsigned int compound_dtor)
> {
> - page[1].compound_dtor = dtor;
> + VM_BUG_ON_PAGE(compound_dtor >= NR_COMPOUND_DTORS, page);
> + page[1].compound_dtor = compound_dtor;
> }
>
> static inline compound_page_dtor *get_compound_page_dtor(struct page *page)
> {
> - return page[1].compound_dtor;
> + VM_BUG_ON_PAGE(page[1].compound_dtor >= NR_COMPOUND_DTORS, page);
> + return compound_page_dtors[page[1].compound_dtor];
> }
>
> static inline int compound_order(struct page *page)
> @@ -596,7 +608,7 @@ static inline int compound_order(struct page *page)
> return page[1].compound_order;
> }
>
> -static inline void set_compound_order(struct page *page, unsigned long order)
> +static inline void set_compound_order(struct page *page, unsigned int order)
> {
> page[1].compound_order = order;
> }
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 58620ac7f15c..63cdfe7ec336 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -28,8 +28,6 @@ struct mem_cgroup;
> IS_ENABLED(CONFIG_ARCH_ENABLE_SPLIT_PMD_PTLOCK))
> #define ALLOC_SPLIT_PTLOCKS (SPINLOCK_SIZE > BITS_PER_LONG/8)
>
> -typedef void compound_page_dtor(struct page *);
> -
> /*
> * Each physical page in the system has a struct page associated with
> * it to keep track of whatever it is we are using the page for at the
> @@ -145,8 +143,13 @@ struct page {
>*/
> /* First tail page of compound page */
> struct {
> - compound_page_dtor *compound_dtor;
> - unsigned long compound_order;
> +#ifdef CONFIG_64BIT
> + unsigned int compound_dtor;
> + unsigned int compound_order;
> +#else
> + unsigned short int compound_dtor;
> + unsigned short int compound_order;
> +#endif
> };
>
> #if defined(CONFIG_TRANSPARENT_HUGEPAGE) && USE_SPLIT_PMD_PTLOCKS
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index a8c3087089d8..8ea74caa1fa8 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -969,7 +969,7 @@ static void update_and_free_page(struct hstate *h, struct
> page *page)
> 1 << PG_writeback);
> }
> VM_BUG_ON_PAGE(hugetlb_cgroup_from_page(page), page);
> - set_compound_page_dtor(page, NULL);
> + set_compound_page_dtor(page, NULL_COMPOUND_DTOR);
> set_page_refcounted(page);
> if (hstate_is_gigantic(h)) {
> destroy_compound_gigantic_page(page, huge_page_order(h));
> @@ -1065,7 +1065,7 @@ void free_huge_page(struct page *page)
> static void prep_new_huge_page(struct hstate *h, struct page *page, int nid)
> {
> INIT_LIST_HEAD(&page->lru);
> - set_compound_page_dtor(page, free_huge_page);
> + set_compound_page_dtor(page, HUGETLB_PAGE_DTOR);
> spin_lock(&hugetlb_lock);
> set_hugetlb_
[PATCHv2 3/4] mm: pack compound_dtor and compound_order into one word in struct page
The patch halves space occupied by compound_dtor and compound_order in
struct page.
For compound_order, it's trivial long -> int/short conversion.
For get_compound_page_dtor(), we now use hardcoded table for destructor
lookup and store its index in the struct page instead of direct pointer
to destructor. It shouldn't be a big trouble to maintain the table: we
have only two destructor and NULL currently.
This patch free up one word in tail pages for reuse. This is preparation
for the next patch.
Signed-off-by: Kirill A. Shutemov
---
include/linux/mm.h | 22 +-
include/linux/mm_types.h | 11 +++
mm/hugetlb.c | 8
mm/page_alloc.c | 9 -
4 files changed, 36 insertions(+), 14 deletions(-)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 2e872f92dbac..9c21bbb8875a 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -575,18 +575,30 @@ int split_free_page(struct page *page);
/*
* Compound pages have a destructor function. Provide a
* prototype for that function and accessor functions.
- * These are _only_ valid on the head of a PG_compound page.
+ * These are _only_ valid on the head of a compound page.
*/
+typedef void compound_page_dtor(struct page *);
+
+/* Keep the enum in sync with compound_page_dtors array in mm/page_alloc.c */
+enum {
+ NULL_COMPOUND_DTOR,
+ COMPOUND_PAGE_DTOR,
+ HUGETLB_PAGE_DTOR,
+ NR_COMPOUND_DTORS,
+};
+extern compound_page_dtor * const compound_page_dtors[];
static inline void set_compound_page_dtor(struct page *page,
- compound_page_dtor *dtor)
+ unsigned int compound_dtor)
{
- page[1].compound_dtor = dtor;
+ VM_BUG_ON_PAGE(compound_dtor >= NR_COMPOUND_DTORS, page);
+ page[1].compound_dtor = compound_dtor;
}
static inline compound_page_dtor *get_compound_page_dtor(struct page *page)
{
- return page[1].compound_dtor;
+ VM_BUG_ON_PAGE(page[1].compound_dtor >= NR_COMPOUND_DTORS, page);
+ return compound_page_dtors[page[1].compound_dtor];
}
static inline int compound_order(struct page *page)
@@ -596,7 +608,7 @@ static inline int compound_order(struct page *page)
return page[1].compound_order;
}
-static inline void set_compound_order(struct page *page, unsigned long order)
+static inline void set_compound_order(struct page *page, unsigned int order)
{
page[1].compound_order = order;
}
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 58620ac7f15c..63cdfe7ec336 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -28,8 +28,6 @@ struct mem_cgroup;
IS_ENABLED(CONFIG_ARCH_ENABLE_SPLIT_PMD_PTLOCK))
#define ALLOC_SPLIT_PTLOCKS(SPINLOCK_SIZE > BITS_PER_LONG/8)
-typedef void compound_page_dtor(struct page *);
-
/*
* Each physical page in the system has a struct page associated with
* it to keep track of whatever it is we are using the page for at the
@@ -145,8 +143,13 @@ struct page {
*/
/* First tail page of compound page */
struct {
- compound_page_dtor *compound_dtor;
- unsigned long compound_order;
+#ifdef CONFIG_64BIT
+ unsigned int compound_dtor;
+ unsigned int compound_order;
+#else
+ unsigned short int compound_dtor;
+ unsigned short int compound_order;
+#endif
};
#if defined(CONFIG_TRANSPARENT_HUGEPAGE) && USE_SPLIT_PMD_PTLOCKS
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index a8c3087089d8..8ea74caa1fa8 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -969,7 +969,7 @@ static void update_and_free_page(struct hstate *h, struct
page *page)
1 << PG_writeback);
}
VM_BUG_ON_PAGE(hugetlb_cgroup_from_page(page), page);
- set_compound_page_dtor(page, NULL);
+ set_compound_page_dtor(page, NULL_COMPOUND_DTOR);
set_page_refcounted(page);
if (hstate_is_gigantic(h)) {
destroy_compound_gigantic_page(page, huge_page_order(h));
@@ -1065,7 +1065,7 @@ void free_huge_page(struct page *page)
static void prep_new_huge_page(struct hstate *h, struct page *page, int nid)
{
INIT_LIST_HEAD(&page->lru);
- set_compound_page_dtor(page, free_huge_page);
+ set_compound_page_dtor(page, HUGETLB_PAGE_DTOR);
spin_lock(&hugetlb_lock);
set_hugetlb_cgroup(page, NULL);
h->nr_huge_pages++;
@@ -1117,7 +1117,7 @@ int PageHuge(struct page *page)
return 0;
page = compound_head(page);
- return get_compound_page_dtor(page) == free_huge_page;
+ return page[1].compound_dtor == HUGETLB_PAGE_DTOR;
}
EXPORT_SYMBOL_GPL(PageHuge);
@@ -1314,7 +1314,7 @@ static struct page *alloc_buddy_huge_page(struct hstate
*h, int n
