| Message ID | 20231106231158.380730-1-nphamcs@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] zswap: memcontrol: implement zswap writeback disabling | expand |
Hi Nhat, Acked-by: Chris Li <chrisl@kernel.org> On Mon, Nov 6, 2023 at 3:12 PM Nhat Pham <nphamcs@gmail.com> wrote: --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -219,6 +219,12 @@ struct mem_cgroup { #if defined(CONFIG_MEMCG_KMEM) && defined(CONFIG_ZSWAP) unsigned long zswap_max; + + /* + * Prevent pages from this memcg from being written back from zswap to + * swap, and from being swapped out on zswap store failures. + */ + bool zswap_writeback; #endif I notice the bool is between two integers. mem_cgroup structure has a few bool sprinkle in different locations. Arrange them together might save a few padding bytes. We can also consider using bit fields. It is a very minor point, the condition also exists before your change. > #endif /* _LINUX_ZSWAP_H */ > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index e43b5aba8efc..9cb3ea912cbe 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -5545,6 +5545,11 @@ mem_cgroup_css_alloc(struct cgroup_subsys_state *parent_css) > WRITE_ONCE(memcg->soft_limit, PAGE_COUNTER_MAX); > #if defined(CONFIG_MEMCG_KMEM) && defined(CONFIG_ZSWAP) > memcg->zswap_max = PAGE_COUNTER_MAX; > + > + if (parent) > + WRITE_ONCE(memcg->zswap_writeback, READ_ONCE(parent->zswap_writeback)); > + else > + WRITE_ONCE(memcg->zswap_writeback, true); You can combine this two WRITE_ONCE to one bool writeback = !parent || READ_ONCE(parent->zswap_writeback); WRITE_ONCE(memcg->zswap_writeback, writeback); Chris
On Fri, Nov 10, 2023 at 3:10 PM Chris Li <chrisl@kernel.org> wrote: > > Hi Nhat, > > Acked-by: Chris Li <chrisl@kernel.org> Thanks, Chris! > > On Mon, Nov 6, 2023 at 3:12 PM Nhat Pham <nphamcs@gmail.com> wrote: > --- a/include/linux/memcontrol.h > +++ b/include/linux/memcontrol.h > @@ -219,6 +219,12 @@ struct mem_cgroup { > > #if defined(CONFIG_MEMCG_KMEM) && defined(CONFIG_ZSWAP) > unsigned long zswap_max; > + > + /* > + * Prevent pages from this memcg from being written back from zswap to > + * swap, and from being swapped out on zswap store failures. > + */ > + bool zswap_writeback; > #endif > > I notice the bool is between two integers. > mem_cgroup structure has a few bool sprinkle in different locations. > Arrange them together might save a few padding bytes. We can also > consider using bit fields. > It is a very minor point, the condition also exists before your change. This sounds like an optimization worthy of its own patch. Two random thoughts however: a) Can this be done at the compiler level? I believe you can reduce the padding required by sorting the fields of a struct by its size, correct? That sounds like a job that a compiler should do for us... b) Re: the bitfield idea, some of the fields are CONFIG-dependent (well like this one). Might be a bit hairier to do it... > > > #endif /* _LINUX_ZSWAP_H */ > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > index e43b5aba8efc..9cb3ea912cbe 100644 > > --- a/mm/memcontrol.c > > +++ b/mm/memcontrol.c > > @@ -5545,6 +5545,11 @@ mem_cgroup_css_alloc(struct cgroup_subsys_state *parent_css) > > WRITE_ONCE(memcg->soft_limit, PAGE_COUNTER_MAX); > > #if defined(CONFIG_MEMCG_KMEM) && defined(CONFIG_ZSWAP) > > memcg->zswap_max = PAGE_COUNTER_MAX; > > + > > + if (parent) > > + WRITE_ONCE(memcg->zswap_writeback, READ_ONCE(parent->zswap_writeback)); > > + else > > + WRITE_ONCE(memcg->zswap_writeback, true); > > You can combine this two WRITE_ONCE to one > > bool writeback = !parent || READ_ONCE(parent->zswap_writeback); > WRITE_ONCE(memcg->zswap_writeback, writeback); > Yeah I originally did something similar, but then decided to do the if-else instead. Honest no strong preference here - just felt that the if-else was cleaner at that moment. > Chris
On Fri, Nov 10, 2023 at 4:10 PM Nhat Pham <nphamcs@gmail.com> wrote: > > I notice the bool is between two integers. > > mem_cgroup structure has a few bool sprinkle in different locations. > > Arrange them together might save a few padding bytes. We can also > > consider using bit fields. > > It is a very minor point, the condition also exists before your change. > > This sounds like an optimization worthy of its own patch. Two random > thoughts however: Sure. I consider this a very minor point as well. > > a) Can this be done at the compiler level? I believe you can reduce > the padding required by sorting the fields of a struct by its size, correct? > That sounds like a job that a compiler should do for us... According to the C standard, the struct member should be layered out in the order it was declared. There are too many codes that assume the first member has the same address of the struct. Consider we use struct for DMA descriptor as well, where the memory layout needs to match the underlying hardware. Re-ordering the members will be really bad there. There are gcc extensions to do structure member randomization. But the randomization layout is determined by the randomization seed. The compiler actually doesn't have the flexibility to rearrange the member orders to reduce the padding either. > > b) Re: the bitfield idea, some of the fields are CONFIG-dependent (well > like this one). Might be a bit hairier to do it... You can declare the bit field under preprocessor condition as well, just like a normal declare. Can you clarify why it is more hairier? The bitfield does not have a pointer address associated with it, the compiler can actually move the bit field bits around. You get the compiler to do it for you in this case. > > > > > > #endif /* _LINUX_ZSWAP_H */ > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > > index e43b5aba8efc..9cb3ea912cbe 100644 > > > --- a/mm/memcontrol.c > > > +++ b/mm/memcontrol.c > > > @@ -5545,6 +5545,11 @@ mem_cgroup_css_alloc(struct cgroup_subsys_state *parent_css) > > > WRITE_ONCE(memcg->soft_limit, PAGE_COUNTER_MAX); > > > #if defined(CONFIG_MEMCG_KMEM) && defined(CONFIG_ZSWAP) > > > memcg->zswap_max = PAGE_COUNTER_MAX; > > > + > > > + if (parent) > > > + WRITE_ONCE(memcg->zswap_writeback, READ_ONCE(parent->zswap_writeback)); > > > + else > > > + WRITE_ONCE(memcg->zswap_writeback, true); > > > > You can combine this two WRITE_ONCE to one > > > > bool writeback = !parent || READ_ONCE(parent->zswap_writeback); > > WRITE_ONCE(memcg->zswap_writeback, writeback); > > > > Yeah I originally did something similar, but then decided to do the if-else > instead. Honest no strong preference here - just felt that the if-else was > cleaner at that moment. One WRITE_ONCE will produce slightly better machine code as less memory store instructions. Normally the compiler is allowed to do the common expression elimination to merge the write. However here it has explicite WRITE_ONCE, so the compiler has to place two memory stores instructions, because you have two WRITE_ONCE. My suggestion will only have one memory store instruction. I agree it is micro optimization. Chris
On Sat, Nov 11, 2023 at 10:22 AM Chris Li <chrisl@kernel.org> wrote: > > On Fri, Nov 10, 2023 at 4:10 PM Nhat Pham <nphamcs@gmail.com> wrote: > > > I notice the bool is between two integers. > > > mem_cgroup structure has a few bool sprinkle in different locations. > > > Arrange them together might save a few padding bytes. We can also > > > consider using bit fields. > > > It is a very minor point, the condition also exists before your change. > > > > This sounds like an optimization worthy of its own patch. Two random > > thoughts however: > > Sure. I consider this a very minor point as well. > > > > > a) Can this be done at the compiler level? I believe you can reduce > > the padding required by sorting the fields of a struct by its size, correct? > > That sounds like a job that a compiler should do for us... > > According to the C standard, the struct member should be layered out > in the order it was declared. There are too many codes that assume the > first member has the same address of the struct. Consider we use > struct for DMA descriptor as well, where the memory layout needs to > match the underlying hardware. Re-ordering the members will be really > bad there. There are gcc extensions to do structure member > randomization. But the randomization layout is determined by the > randomization seed. The compiler actually doesn't have the flexibility > to rearrange the member orders to reduce the padding either. > Ah I see. Yeah then it might be worth tweaking around manually. But yeah, we should do this separately from this patch. > > > > b) Re: the bitfield idea, some of the fields are CONFIG-dependent (well > > like this one). Might be a bit hairier to do it... > > You can declare the bit field under preprocessor condition as well, > just like a normal declare. Can you clarify why it is more hairier? > The bitfield does not have a pointer address associated with it, the > compiler can actually move the bit field bits around. You get the > compiler to do it for you in this case. I see hmmm. > > > > > > > > > > #endif /* _LINUX_ZSWAP_H */ > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > > > index e43b5aba8efc..9cb3ea912cbe 100644 > > > > --- a/mm/memcontrol.c > > > > +++ b/mm/memcontrol.c > > > > @@ -5545,6 +5545,11 @@ mem_cgroup_css_alloc(struct cgroup_subsys_state *parent_css) > > > > WRITE_ONCE(memcg->soft_limit, PAGE_COUNTER_MAX); > > > > #if defined(CONFIG_MEMCG_KMEM) && defined(CONFIG_ZSWAP) > > > > memcg->zswap_max = PAGE_COUNTER_MAX; > > > > + > > > > + if (parent) > > > > + WRITE_ONCE(memcg->zswap_writeback, READ_ONCE(parent->zswap_writeback)); > > > > + else > > > > + WRITE_ONCE(memcg->zswap_writeback, true); > > > > > > You can combine this two WRITE_ONCE to one > > > > > > bool writeback = !parent || READ_ONCE(parent->zswap_writeback); > > > WRITE_ONCE(memcg->zswap_writeback, writeback); > > > > > > > Yeah I originally did something similar, but then decided to do the if-else > > instead. Honest no strong preference here - just felt that the if-else was > > cleaner at that moment. > > One WRITE_ONCE will produce slightly better machine code as less > memory store instructions. Normally the compiler is allowed to do the > common expression elimination to merge the write. However here it has > explicite WRITE_ONCE, so the compiler has to place two memory stores > instructions, because you have two WRITE_ONCE. My suggestion will only > have one memory store instruction. I agree it is micro optimization. > Ohh I did not think about this. Seems like my original version was more than just a clever one-liner haha. It's a bit of a micro-optimization indeed. But if for some reason I need to send v5 or a fixlet, I'll keep this in mind! Thanks for the explanation, Chris! > Chris
On Mon, Nov 6, 2023 at 3:12 PM Nhat Pham <nphamcs@gmail.com> wrote: > > During our experiment with zswap, we sometimes observe swap IOs due to > occasional zswap store failures and writebacks-to-swap. These swapping > IOs prevent many users who cannot tolerate swapping from adopting zswap > to save memory and improve performance where possible. > > This patch adds the option to disable this behavior entirely: do not > writeback to backing swapping device when a zswap store attempt fail, > and do not write pages in the zswap pool back to the backing swap > device (both when the pool is full, and when the new zswap shrinker is > called). > > This new behavior can be opted-in/out on a per-cgroup basis via a new > cgroup file. By default, writebacks to swap device is enabled, which is > the previous behavior. Initially, writeback is enabled for the root > cgroup, and a newly created cgroup will inherit the current setting of > its parent. > > Note that this is subtly different from setting memory.swap.max to 0, as > it still allows for pages to be stored in the zswap pool (which itself > consumes swap space in its current form). > > Suggested-by: Johannes Weiner <hannes@cmpxchg.org> > Signed-off-by: Nhat Pham <nphamcs@gmail.com> LGTM, Reviewed-by: Yosry Ahmed <yosryahmed@google.com>
On Sat, 11 Nov 2023 16:06:58 -0800 Nhat Pham <nphamcs@gmail.com> wrote: > It's a bit of a micro-optimization indeed. But if for some reason I need > to send v5 or a fixlet, I'll keep this in mind! I'm seeing a lot of rejects against current mainline, so yes please.
On Tue, Nov 14, 2023 at 5:50 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Sat, 11 Nov 2023 16:06:58 -0800 Nhat Pham <nphamcs@gmail.com> wrote: > > > It's a bit of a micro-optimization indeed. But if for some reason I need > > to send v5 or a fixlet, I'll keep this in mind! > > I'm seeing a lot of rejects against current mainline, so yes please. I should have been clearer - this patch is to be applied on top of the shrinker series (which is still under review). I originally included a comment in the patch log, but it must have been accidentally dropped when I reworked the patch according to Chris' and Johannes' suggestions. Nevertheless, I have rebased it on top of current mm-unstable (with the shrinker series on top), then sent out as v5 (with the second suggestion from Chris Li) just in case. Let me know if there are still issues! My apologies for the inconvenience and lack of clarity.
diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst index 606b2e0eac4b..eefccfffdff7 100644 --- a/Documentation/admin-guide/cgroup-v2.rst +++ b/Documentation/admin-guide/cgroup-v2.rst @@ -1672,6 +1672,18 @@ PAGE_SIZE multiple when read back. limit, it will refuse to take any more stores before existing entries fault back in or are written out to disk. + memory.zswap.writeback + A read-write single value file. The default value is "1". The + initial value of the root cgroup is 1, and when a new cgroup is + created, it inherits the current value of its parent. + + When this is set to 0, all swapping attempts to swapping devices + are disabled. This included both zswap writebacks, and swapping due + to zswap store failure. + + Note that this is subtly different from setting memory.swap.max to + 0, as it still allows for pages to be written to the zswap pool. + memory.pressure A read-only nested-keyed file. diff --git a/Documentation/admin-guide/mm/zswap.rst b/Documentation/admin-guide/mm/zswap.rst index 522ae22ccb84..b987e58edb70 100644 --- a/Documentation/admin-guide/mm/zswap.rst +++ b/Documentation/admin-guide/mm/zswap.rst @@ -153,6 +153,12 @@ attribute, e. g.:: Setting this parameter to 100 will disable the hysteresis. +Some users cannot tolerate the swapping that comes with zswap store failures +and zswap writebacks. Swapping can be disabled entirely (without disabling +zswap itself) on a cgroup-basis as follows: + + echo 0 > /sys/fs/cgroup/<cgroup-name>/memory.zswap.writeback + When there is a sizable amount of cold memory residing in the zswap pool, it can be advantageous to proactively write these cold pages to swap and reclaim the memory for other use cases. By default, the zswap shrinker is disabled. diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 95f6c9e60ed1..e51eafdf2a15 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -219,6 +219,12 @@ struct mem_cgroup { #if defined(CONFIG_MEMCG_KMEM) && defined(CONFIG_ZSWAP) unsigned long zswap_max; + + /* + * Prevent pages from this memcg from being written back from zswap to + * swap, and from being swapped out on zswap store failures. + */ + bool zswap_writeback; #endif unsigned long soft_limit; @@ -1931,6 +1937,7 @@ static inline void count_objcg_event(struct obj_cgroup *objcg, bool obj_cgroup_may_zswap(struct obj_cgroup *objcg); void obj_cgroup_charge_zswap(struct obj_cgroup *objcg, size_t size); void obj_cgroup_uncharge_zswap(struct obj_cgroup *objcg, size_t size); +bool mem_cgroup_zswap_writeback_enabled(struct mem_cgroup *memcg); #else static inline bool obj_cgroup_may_zswap(struct obj_cgroup *objcg) { @@ -1944,6 +1951,11 @@ static inline void obj_cgroup_uncharge_zswap(struct obj_cgroup *objcg, size_t size) { } +static inline bool mem_cgroup_zswap_writeback_enabled(struct mem_cgroup *memcg) +{ + /* if zswap is disabled, do not block pages going to the swapping device */ + return true; +} #endif #endif /* _LINUX_MEMCONTROL_H */ diff --git a/include/linux/zswap.h b/include/linux/zswap.h index cbd373ba88d2..b4997e27a74b 100644 --- a/include/linux/zswap.h +++ b/include/linux/zswap.h @@ -35,6 +35,7 @@ void zswap_swapoff(int type); void zswap_memcg_offline_cleanup(struct mem_cgroup *memcg); void zswap_lruvec_state_init(struct lruvec *lruvec); void zswap_lruvec_swapin(struct page *page); +bool is_zswap_enabled(void); #else struct zswap_lruvec_state {}; @@ -55,6 +56,11 @@ static inline void zswap_swapoff(int type) {} static inline void zswap_memcg_offline_cleanup(struct mem_cgroup *memcg) {} static inline void zswap_lruvec_init(struct lruvec *lruvec) {} static inline void zswap_lruvec_swapin(struct page *page) {} + +static inline bool is_zswap_enabled(void) +{ + return false; +} #endif #endif /* _LINUX_ZSWAP_H */ diff --git a/mm/memcontrol.c b/mm/memcontrol.c index e43b5aba8efc..9cb3ea912cbe 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -5545,6 +5545,11 @@ mem_cgroup_css_alloc(struct cgroup_subsys_state *parent_css) WRITE_ONCE(memcg->soft_limit, PAGE_COUNTER_MAX); #if defined(CONFIG_MEMCG_KMEM) && defined(CONFIG_ZSWAP) memcg->zswap_max = PAGE_COUNTER_MAX; + + if (parent) + WRITE_ONCE(memcg->zswap_writeback, READ_ONCE(parent->zswap_writeback)); + else + WRITE_ONCE(memcg->zswap_writeback, true); #endif page_counter_set_high(&memcg->swap, PAGE_COUNTER_MAX); if (parent) { @@ -8177,6 +8182,12 @@ void obj_cgroup_uncharge_zswap(struct obj_cgroup *objcg, size_t size) rcu_read_unlock(); } +bool mem_cgroup_zswap_writeback_enabled(struct mem_cgroup *memcg) +{ + /* if zswap is disabled, do not block pages going to the swapping device */ + return !is_zswap_enabled() || !memcg || READ_ONCE(memcg->zswap_writeback); +} + static u64 zswap_current_read(struct cgroup_subsys_state *css, struct cftype *cft) { @@ -8209,6 +8220,31 @@ static ssize_t zswap_max_write(struct kernfs_open_file *of, return nbytes; } +static int zswap_writeback_show(struct seq_file *m, void *v) +{ + struct mem_cgroup *memcg = mem_cgroup_from_seq(m); + + seq_printf(m, "%d\n", READ_ONCE(memcg->zswap_writeback)); + return 0; +} + +static ssize_t zswap_writeback_write(struct kernfs_open_file *of, + char *buf, size_t nbytes, loff_t off) +{ + struct mem_cgroup *memcg = mem_cgroup_from_css(of_css(of)); + int zswap_writeback; + ssize_t parse_ret = kstrtoint(strstrip(buf), 0, &zswap_writeback); + + if (parse_ret) + return parse_ret; + + if (zswap_writeback != 0 && zswap_writeback != 1) + return -EINVAL; + + WRITE_ONCE(memcg->zswap_writeback, zswap_writeback); + return nbytes; +} + static struct cftype zswap_files[] = { { .name = "zswap.current", @@ -8221,6 +8257,11 @@ static struct cftype zswap_files[] = { .seq_show = zswap_max_show, .write = zswap_max_write, }, + { + .name = "zswap.writeback", + .seq_show = zswap_writeback_show, + .write = zswap_writeback_write, + }, { } /* terminate */ }; #endif /* CONFIG_MEMCG_KMEM && CONFIG_ZSWAP */ diff --git a/mm/page_io.c b/mm/page_io.c index cb559ae324c6..5e606f1aa2f6 100644 --- a/mm/page_io.c +++ b/mm/page_io.c @@ -201,6 +201,12 @@ int swap_writepage(struct page *page, struct writeback_control *wbc) folio_end_writeback(folio); return 0; } + + if (!mem_cgroup_zswap_writeback_enabled(folio_memcg(folio))) { + folio_mark_dirty(folio); + return AOP_WRITEPAGE_ACTIVATE; + } + __swap_writepage(&folio->page, wbc); return 0; } diff --git a/mm/shmem.c b/mm/shmem.c index cab053831fea..e5044678de8b 100644 --- a/mm/shmem.c +++ b/mm/shmem.c @@ -1514,8 +1514,7 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc) mutex_unlock(&shmem_swaplist_mutex); BUG_ON(folio_mapped(folio)); - swap_writepage(&folio->page, wbc); - return 0; + return swap_writepage(&folio->page, wbc); } mutex_unlock(&shmem_swaplist_mutex); diff --git a/mm/zswap.c b/mm/zswap.c index 260e01180ee0..051f4487c9ab 100644 --- a/mm/zswap.c +++ b/mm/zswap.c @@ -150,6 +150,11 @@ module_param_named(exclusive_loads, zswap_exclusive_loads_enabled, bool, 0644); static bool zswap_shrinker_enabled; module_param_named(shrinker_enabled, zswap_shrinker_enabled, bool, 0644); +bool is_zswap_enabled(void) +{ + return zswap_enabled; +} + /********************************* * data structures **********************************/ @@ -590,6 +595,9 @@ static unsigned long zswap_shrinker_scan(struct shrinker *shrinker, struct zswap_pool *pool = shrinker->private_data; bool encountered_page_in_swapcache = false; + if (!mem_cgroup_zswap_writeback_enabled(sc->memcg)) + return SHRINK_STOP; + nr_protected = atomic_long_read(&lruvec->zswap_lruvec_state.nr_zswap_protected); lru_size = list_lru_shrink_count(&pool->list_lru, sc); @@ -620,6 +628,9 @@ static unsigned long zswap_shrinker_count(struct shrinker *shrinker, struct lruvec *lruvec = mem_cgroup_lruvec(memcg, NODE_DATA(sc->nid)); unsigned long nr_backing, nr_stored, nr_freeable, nr_protected; + if (!mem_cgroup_zswap_writeback_enabled(memcg)) + return 0; + #ifdef CONFIG_MEMCG_KMEM cgroup_rstat_flush(memcg->css.cgroup); nr_backing = memcg_page_state(memcg, MEMCG_ZSWAP_B) >> PAGE_SHIFT; @@ -935,6 +946,9 @@ static int shrink_memcg(struct mem_cgroup *memcg) struct zswap_pool *pool; int nid, shrunk = 0; + if (!mem_cgroup_zswap_writeback_enabled(memcg)) + return -EINVAL; + /* * Skip zombies because their LRUs are reparented and we would be * reclaiming from the parent instead of the dead memcg.
During our experiment with zswap, we sometimes observe swap IOs due to occasional zswap store failures and writebacks-to-swap. These swapping IOs prevent many users who cannot tolerate swapping from adopting zswap to save memory and improve performance where possible. This patch adds the option to disable this behavior entirely: do not writeback to backing swapping device when a zswap store attempt fail, and do not write pages in the zswap pool back to the backing swap device (both when the pool is full, and when the new zswap shrinker is called). This new behavior can be opted-in/out on a per-cgroup basis via a new cgroup file. By default, writebacks to swap device is enabled, which is the previous behavior. Initially, writeback is enabled for the root cgroup, and a newly created cgroup will inherit the current setting of its parent. Note that this is subtly different from setting memory.swap.max to 0, as it still allows for pages to be stored in the zswap pool (which itself consumes swap space in its current form). Suggested-by: Johannes Weiner <hannes@cmpxchg.org> Signed-off-by: Nhat Pham <nphamcs@gmail.com> --- Documentation/admin-guide/cgroup-v2.rst | 12 ++++++++ Documentation/admin-guide/mm/zswap.rst | 6 ++++ include/linux/memcontrol.h | 12 ++++++++ include/linux/zswap.h | 6 ++++ mm/memcontrol.c | 41 +++++++++++++++++++++++++ mm/page_io.c | 6 ++++ mm/shmem.c | 3 +- mm/zswap.c | 14 +++++++++ 8 files changed, 98 insertions(+), 2 deletions(-)