On Wed, Apr 15, 2009 at 09:38:50PM +0900, Ryo Tsuruta wrote:
> Hi Andrea and Kamezawa-san,
>
> > Ryo, it would be great if you can look at this and fix/integrate into
> > the mainstream bio-cgroup. Otherwise I can try to to schedule this in my
> > work.
>
> O.K. I'll apply those fixes and post patches as soon as I can.
>
Very good! I've just tested the bio_cgroup_id inclusion in
page_cgroup->flags. I'm posting the patch on-top-of my patchset.
If you're interested, it should apply cleanly to the original
bio-cgroup, except for the get/put_cgroup_from_page() part.
Thanks,
-Andrea
---
bio-cgroup: encode bio_cgroup_id in page_cgroup->flags
Encode the bio_cgroup_id into the flags argument of page_cgroup as
suggested by Kamezawa.
Lower 16-bits of the flags attribute are used for the actual page_cgroup
flags. The rest is reserved to store the bio-cgroup id.
This allows to save 4 bytes (in 32-bit architectures) or 8 bytes (in
64-bit) for each page_cgroup element.
Signed-off-by: Andrea Righi <righi.andrea@xxxxxxxxx>
---
include/linux/biotrack.h | 2 +-
include/linux/page_cgroup.h | 24 +++++++++++++++++++++---
mm/biotrack.c | 26 ++++++++++++--------------
3 files changed, 34 insertions(+), 18 deletions(-)
diff --git a/include/linux/biotrack.h b/include/linux/biotrack.h
index 25b8810..4bd0242 100644
--- a/include/linux/biotrack.h
+++ b/include/linux/biotrack.h
@@ -28,7 +28,7 @@ struct bio_cgroup {
static inline void __init_bio_page_cgroup(struct page_cgroup *pc)
{
- pc->bio_cgroup_id = 0;
+ page_cgroup_set_bio_id(pc, 0);
}
extern struct cgroup *get_cgroup_from_page(struct page *page);
diff --git a/include/linux/page_cgroup.h b/include/linux/page_cgroup.h
index 00a49c5..af780a4 100644
--- a/include/linux/page_cgroup.h
+++ b/include/linux/page_cgroup.h
@@ -16,12 +16,30 @@ struct page_cgroup {
#ifdef CONFIG_CGROUP_MEM_RES_CTLR
struct mem_cgroup *mem_cgroup;
#endif
-#ifdef CONFIG_CGROUP_BIO
- int bio_cgroup_id;
-#endif
struct list_head lru; /* per cgroup LRU list */
};
+#ifdef CONFIG_CGROUP_BIO
+/*
+ * use lower 16 bits for flags and reserve the rest for the bio-cgroup id
+ */
+#define BIO_CGROUP_ID_SHIFT (16)
+#define BIO_CGROUP_ID_BITS (8 * sizeof(unsigned long) - BIO_CGROUP_ID_SHIFT)
+
+static inline unsigned long page_cgroup_get_bio_id(struct page_cgroup *pc)
+{
+ return pc->flags >> BIO_CGROUP_ID_SHIFT;
+}
+
+static inline void page_cgroup_set_bio_id(struct page_cgroup *pc,
+ unsigned long id)
+{
+ WARN_ON(id >= (1UL << BIO_CGROUP_ID_BITS));
+ pc->flags &= (1UL << BIO_CGROUP_ID_SHIFT) - 1;
+ pc->flags |= (unsigned long)(id << BIO_CGROUP_ID_SHIFT);
+}
+#endif
+
void __meminit pgdat_page_cgroup_init(struct pglist_data *pgdat);
void __init page_cgroup_init(void);
struct page_cgroup *lookup_page_cgroup(struct page *page);
diff --git a/mm/biotrack.c b/mm/biotrack.c
index 431056c..4cca3bf 100644
--- a/mm/biotrack.c
+++ b/mm/biotrack.c
@@ -74,7 +74,7 @@ void bio_cgroup_set_owner(struct page *page, struct mm_struct *mm)
struct bio_cgroup *biog;
struct page_cgroup *pc;
- WARN_ON_ONCE(test_bit(PG_locked, &page->flags));
+ WARN_ON_ONCE(!test_bit(PG_locked, &page->flags));
if (bio_cgroup_disabled())
return;
@@ -82,7 +82,7 @@ void bio_cgroup_set_owner(struct page *page, struct mm_struct *mm)
if (unlikely(!pc))
return;
- pc->bio_cgroup_id = 0; /* 0: default bio_cgroup id */
+ __init_bio_page_cgroup(pc);
if (!mm)
return;
/*
@@ -95,11 +95,11 @@ void bio_cgroup_set_owner(struct page *page, struct mm_struct *mm)
goto out;
/*
* css_get(&bio->css) isn't called to increment the reference
- * count of this bio_cgroup "biog" so pc->bio_cgroup_id might turn
- * invalid even if this page is still active.
- * This approach is chosen to minimize the overhead.
+ * count of this bio_cgroup "biog" so the bio-cgroup id might turn
+ * invalid even if this page is still active. This approach is chosen
+ * to minimize the overhead.
*/
- pc->bio_cgroup_id = biog->id;
+ page_cgroup_set_bio_id(pc, biog->id);
out:
rcu_read_unlock();
}
@@ -112,7 +112,7 @@ void bio_cgroup_reset_owner(struct page *page, struct mm_struct *mm)
/*
* A little trick:
* Just call bio_cgroup_set_owner() for pages which are already
- * active since the bio_cgroup_id member of page_cgroup can be
+ * active since the bio-cgroup id member of page_cgroup can be
* updated without any locks. This is because an integer type of
* variable can be set a new value at once on modern cpus.
*/
@@ -148,12 +148,10 @@ void bio_cgroup_copy_owner(struct page *npage, struct page *opage)
opc = lookup_page_cgroup(opage);
if (unlikely(!opc))
return;
-
/*
- * Do this without any locks. The reason is the same as
- * bio_cgroup_reset_owner().
+ * XXX: is it safe to do this without locking?
*/
- npc->bio_cgroup_id = opc->bio_cgroup_id;
+ page_cgroup_set_bio_id(npc, page_cgroup_get_bio_id(opc));
}
/* Create a new bio-cgroup. */
@@ -250,7 +248,7 @@ struct cgroup *get_cgroup_from_page(struct page *page)
if (!pc)
return NULL;
lock_page_cgroup(pc);
- biog = find_bio_cgroup(pc->bio_cgroup_id);
+ biog = find_bio_cgroup(page_cgroup_get_bio_id(pc));
if (biog) {
css_get(&biog->css);
cgrp = biog->css.cgroup;
@@ -268,7 +266,7 @@ void put_cgroup_from_page(struct page *page)
if (!pc)
return;
lock_page_cgroup(pc);
- biog = find_bio_cgroup(pc->bio_cgroup_id);
+ biog = find_bio_cgroup(page_cgroup_get_bio_id(pc));
if (biog)
css_put(&biog->css);
unlock_page_cgroup(pc);
@@ -283,7 +281,7 @@ int get_bio_cgroup_id(struct bio *bio)
pc = lookup_page_cgroup(page);
if (pc)
- id = pc->bio_cgroup_id;
+ id = page_cgroup_get_bio_id(pc);
return id;
}
EXPORT_SYMBOL(get_bio_cgroup_id);
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/containers
