| Message ID | 20240325235018.2028408-6-yosryahmed@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | zswap: store zero-filled pages more efficiently | expand |
On Mon, Mar 25, 2024 at 4:50 PM Yosry Ahmed <yosryahmed@google.com> wrote: > > There is no logical reason to refuse storing same-filled pages more > efficiently and opt for compression. Remove the userspace knob. > Agree. Absolutely no idea why we have this knob in the first place - if you have cycles to compress, you probably have some extra cycles to check same-filled page state? And that is the only cost I can think of - it wins on probably all other aspects: memory usage, "decompression", no need to write back these pages etc. Any actual zswap user who has data or counter-use case should chime in, but FWIW, my vote is: Reviewed-by: Nhat Pham <nphamcs@gmail.com>
On 2024/3/26 07:50, Yosry Ahmed wrote: > There is no logical reason to refuse storing same-filled pages more > efficiently and opt for compression. Remove the userspace knob. > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> LGTM, should we also remove zswap_non_same_filled_pages_enabled? Not sure if it has real usage... Reviewed-by: Chengming Zhou <chengming.zhou@linux.dev> > --- > mm/zswap.c | 11 ----------- > 1 file changed, 11 deletions(-) > > diff --git a/mm/zswap.c b/mm/zswap.c > index 498a6c5839bef..0fc27ae950c74 100644 > --- a/mm/zswap.c > +++ b/mm/zswap.c > @@ -123,14 +123,6 @@ static unsigned int zswap_accept_thr_percent = 90; /* of max pool size */ > module_param_named(accept_threshold_percent, zswap_accept_thr_percent, > uint, 0644); > > -/* > - * Enable/disable handling same-value filled pages (enabled by default). > - * If disabled every page is considered non-same-value filled. > - */ > -static bool zswap_same_filled_pages_enabled = true; > -module_param_named(same_filled_pages_enabled, zswap_same_filled_pages_enabled, > - bool, 0644); > - > /* Enable/disable handling non-same-value filled pages (enabled by default) */ > static bool zswap_non_same_filled_pages_enabled = true; > module_param_named(non_same_filled_pages_enabled, zswap_non_same_filled_pages_enabled, > @@ -1392,9 +1384,6 @@ static bool zswap_is_folio_same_filled(struct folio *folio, unsigned long *value > unsigned int pos, last_pos = PAGE_SIZE / sizeof(*page) - 1; > bool ret; > > - if (!zswap_same_filled_pages_enabled) > - return false; > - > page = kmap_local_folio(folio, 0); > val = page[0]; >
On Tue, Mar 26, 2024 at 7:44 PM Chengming Zhou <chengming.zhou@linux.dev> wrote: > > On 2024/3/26 07:50, Yosry Ahmed wrote: > > There is no logical reason to refuse storing same-filled pages more > > efficiently and opt for compression. Remove the userspace knob. > > > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > > LGTM, should we also remove zswap_non_same_filled_pages_enabled? > Not sure if it has real usage... I am not aware of usages, but in theory you can use it if you exclusively use zswap to optimize swapping zero-filled pages. Not sure if anyone actually does that though. We can remove it if everyone else agrees. > > Reviewed-by: Chengming Zhou <chengming.zhou@linux.dev> Thanks!
On Mon, Mar 25, 2024 at 11:50:13PM +0000, Yosry Ahmed wrote: > There is no logical reason to refuse storing same-filled pages more > efficiently and opt for compression. Remove the userspace knob. > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> Acked-by: Johannes Weiner <hannes@cmpxchg.org> I also think the non_same_filled_pages_enabled option should go away. Both of these tunables are pretty bizarre.
On Thu, Mar 28, 2024 at 12:11 PM Johannes Weiner <hannes@cmpxchg.org> wrote: > > On Mon, Mar 25, 2024 at 11:50:13PM +0000, Yosry Ahmed wrote: > > There is no logical reason to refuse storing same-filled pages more > > efficiently and opt for compression. Remove the userspace knob. > > > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > > Acked-by: Johannes Weiner <hannes@cmpxchg.org> > > I also think the non_same_filled_pages_enabled option should go > away. Both of these tunables are pretty bizarre. Happy to remove both in the next version :) Thanks!
On Thu, Mar 28, 2024 at 1:06 PM Yosry Ahmed <yosryahmed@google.com> wrote: > > On Thu, Mar 28, 2024 at 12:11 PM Johannes Weiner <hannes@cmpxchg.org> wrote: > > > > On Mon, Mar 25, 2024 at 11:50:13PM +0000, Yosry Ahmed wrote: > > > There is no logical reason to refuse storing same-filled pages more > > > efficiently and opt for compression. Remove the userspace knob. > > > > > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > > > > Acked-by: Johannes Weiner <hannes@cmpxchg.org> > > > > I also think the non_same_filled_pages_enabled option should go > > away. Both of these tunables are pretty bizarre. > > Happy to remove both in the next version :) I thought non_same_filled_pages_enabled was introduced with the initial support for same-filled pages, but it was introduced separately (and much more recently): https://lore.kernel.org/all/7dbafa963e8bab43608189abbe2067f4b9287831.1641247624.git.maciej.szmigiero@oracle.com/ I am CCing Maciej to hear more about the use case for this. > > Thanks!
On 29.03.2024 03:14, Yosry Ahmed wrote: > On Thu, Mar 28, 2024 at 1:06 PM Yosry Ahmed <yosryahmed@google.com> wrote: >> >> On Thu, Mar 28, 2024 at 12:11 PM Johannes Weiner <hannes@cmpxchg.org> wrote: >>> >>> On Mon, Mar 25, 2024 at 11:50:13PM +0000, Yosry Ahmed wrote: >>>> There is no logical reason to refuse storing same-filled pages more >>>> efficiently and opt for compression. Remove the userspace knob. >>>> >>>> Signed-off-by: Yosry Ahmed <yosryahmed@google.com> >>> >>> Acked-by: Johannes Weiner <hannes@cmpxchg.org> >>> >>> I also think the non_same_filled_pages_enabled option should go >>> away. Both of these tunables are pretty bizarre. >> >> Happy to remove both in the next version :) > > I thought non_same_filled_pages_enabled was introduced with the > initial support for same-filled pages, but it was introduced > separately (and much more recently): > https://lore.kernel.org/all/7dbafa963e8bab43608189abbe2067f4b9287831.1641247624.git.maciej.szmigiero@oracle.com/ > > I am CCing Maciej to hear more about the use case for this. Thanks for CCing me. I introduced "non_same_filled_pages_enabled" a few years ago to enable using zswap in a lightweight mode where it is only used for its ability to store same-filled pages effectively. As far as I remember, there were some interactions between full zswap and the cgroup memory controller - like, it made it easier for an aggressive workload to exceed its cgroup memory.high limits. On the other hand, "same_filled_pages_enabled" sounds like some kind of a debugging option since I don't see any real reason to disable it either. Thanks, Maciej
On Fri, Mar 29, 2024 at 03:02:10PM +0100, Maciej S. Szmigiero wrote: > On 29.03.2024 03:14, Yosry Ahmed wrote: > > On Thu, Mar 28, 2024 at 1:06 PM Yosry Ahmed <yosryahmed@google.com> wrote: > >> > >> On Thu, Mar 28, 2024 at 12:11 PM Johannes Weiner <hannes@cmpxchg.org> wrote: > >>> > >>> On Mon, Mar 25, 2024 at 11:50:13PM +0000, Yosry Ahmed wrote: > >>>> There is no logical reason to refuse storing same-filled pages more > >>>> efficiently and opt for compression. Remove the userspace knob. > >>>> > >>>> Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > >>> > >>> Acked-by: Johannes Weiner <hannes@cmpxchg.org> > >>> > >>> I also think the non_same_filled_pages_enabled option should go > >>> away. Both of these tunables are pretty bizarre. > >> > >> Happy to remove both in the next version :) > > > > I thought non_same_filled_pages_enabled was introduced with the > > initial support for same-filled pages, but it was introduced > > separately (and much more recently): > > https://lore.kernel.org/all/7dbafa963e8bab43608189abbe2067f4b9287831.1641247624.git.maciej.szmigiero@oracle.com/ > > > > I am CCing Maciej to hear more about the use case for this. > > Thanks for CCing me. > > I introduced "non_same_filled_pages_enabled" a few years ago to > enable using zswap in a lightweight mode where it is only used for > its ability to store same-filled pages effectively. But all the pages it rejects go to disk swap instead, which is much slower than compression... > As far as I remember, there were some interactions between full > zswap and the cgroup memory controller - like, it made it easier > for an aggressive workload to exceed its cgroup memory.high limits. Ok, that makes sense! A container fairness measure, rather than a performance optimization. Fair enough, but that's moot then with cgroup accounting of the backing memory, f4840ccfca25 ("zswap: memcg accounting"). Thanks for prodiving context.
On Fri, Mar 29, 2024 at 10:45 AM Johannes Weiner <hannes@cmpxchg.org> wrote: > > On Fri, Mar 29, 2024 at 03:02:10PM +0100, Maciej S. Szmigiero wrote: > > On 29.03.2024 03:14, Yosry Ahmed wrote: > > > On Thu, Mar 28, 2024 at 1:06 PM Yosry Ahmed <yosryahmed@google.com> wrote: > > >> > > >> On Thu, Mar 28, 2024 at 12:11 PM Johannes Weiner <hannes@cmpxchg.org> wrote: > > >>> > > >>> On Mon, Mar 25, 2024 at 11:50:13PM +0000, Yosry Ahmed wrote: > > >>>> There is no logical reason to refuse storing same-filled pages more > > >>>> efficiently and opt for compression. Remove the userspace knob. > > >>>> > > >>>> Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > > >>> > > >>> Acked-by: Johannes Weiner <hannes@cmpxchg.org> > > >>> > > >>> I also think the non_same_filled_pages_enabled option should go > > >>> away. Both of these tunables are pretty bizarre. > > >> > > >> Happy to remove both in the next version :) > > > > > > I thought non_same_filled_pages_enabled was introduced with the > > > initial support for same-filled pages, but it was introduced > > > separately (and much more recently): > > > https://lore.kernel.org/all/7dbafa963e8bab43608189abbe2067f4b9287831.1641247624.git.maciej.szmigiero@oracle.com/ > > > > > > I am CCing Maciej to hear more about the use case for this. > > > > Thanks for CCing me. > > > > I introduced "non_same_filled_pages_enabled" a few years ago to > > enable using zswap in a lightweight mode where it is only used for > > its ability to store same-filled pages effectively. > > But all the pages it rejects go to disk swap instead, which is much > slower than compression... > > > As far as I remember, there were some interactions between full > > zswap and the cgroup memory controller - like, it made it easier > > for an aggressive workload to exceed its cgroup memory.high limits. > > Ok, that makes sense! A container fairness measure, rather than a > performance optimization. > > Fair enough, but that's moot then with cgroup accounting of the > backing memory, f4840ccfca25 ("zswap: memcg accounting"). Right, this should no longer be needed with the zswap charging. Maciej, is this still being used on kernels with f4840ccfca25 (5.19+)? Any objections to removing it now?
On 29.03.2024 19:22, Yosry Ahmed wrote: > On Fri, Mar 29, 2024 at 10:45 AM Johannes Weiner <hannes@cmpxchg.org> wrote: >> >> On Fri, Mar 29, 2024 at 03:02:10PM +0100, Maciej S. Szmigiero wrote: >>> On 29.03.2024 03:14, Yosry Ahmed wrote: >>>> On Thu, Mar 28, 2024 at 1:06 PM Yosry Ahmed <yosryahmed@google.com> wrote: >>>>> >>>>> On Thu, Mar 28, 2024 at 12:11 PM Johannes Weiner <hannes@cmpxchg.org> wrote: >>>>>> >>>>>> On Mon, Mar 25, 2024 at 11:50:13PM +0000, Yosry Ahmed wrote: >>>>>>> There is no logical reason to refuse storing same-filled pages more >>>>>>> efficiently and opt for compression. Remove the userspace knob. >>>>>>> >>>>>>> Signed-off-by: Yosry Ahmed <yosryahmed@google.com> >>>>>> >>>>>> Acked-by: Johannes Weiner <hannes@cmpxchg.org> >>>>>> >>>>>> I also think the non_same_filled_pages_enabled option should go >>>>>> away. Both of these tunables are pretty bizarre. >>>>> >>>>> Happy to remove both in the next version :) >>>> >>>> I thought non_same_filled_pages_enabled was introduced with the >>>> initial support for same-filled pages, but it was introduced >>>> separately (and much more recently): >>>> https://lore.kernel.org/all/7dbafa963e8bab43608189abbe2067f4b9287831.1641247624.git.maciej.szmigiero@oracle.com/ >>>> >>>> I am CCing Maciej to hear more about the use case for this. >>> >>> Thanks for CCing me. >>> >>> I introduced "non_same_filled_pages_enabled" a few years ago to >>> enable using zswap in a lightweight mode where it is only used for >>> its ability to store same-filled pages effectively. >> >> But all the pages it rejects go to disk swap instead, which is much >> slower than compression... >> >>> As far as I remember, there were some interactions between full >>> zswap and the cgroup memory controller - like, it made it easier >>> for an aggressive workload to exceed its cgroup memory.high limits. >> >> Ok, that makes sense! A container fairness measure, rather than a >> performance optimization. >> >> Fair enough, but that's moot then with cgroup accounting of the >> backing memory, f4840ccfca25 ("zswap: memcg accounting"). > > Right, this should no longer be needed with the zswap charging. > > Maciej, is this still being used on kernels with f4840ccfca25 (5.19+)? > Any objections to removing it now? I don't object to its removal as long as stable kernel trees aren't affected. Thanks, Maciej
On Mon, Apr 1, 2024 at 3:38 AM Maciej S. Szmigiero <mail@maciej.szmigiero.name> wrote: > > On 29.03.2024 19:22, Yosry Ahmed wrote: > > On Fri, Mar 29, 2024 at 10:45 AM Johannes Weiner <hannes@cmpxchg.org> wrote: > >> > >> On Fri, Mar 29, 2024 at 03:02:10PM +0100, Maciej S. Szmigiero wrote: > >>> On 29.03.2024 03:14, Yosry Ahmed wrote: > >>>> On Thu, Mar 28, 2024 at 1:06 PM Yosry Ahmed <yosryahmed@google.com> wrote: > >>>>> > >>>>> On Thu, Mar 28, 2024 at 12:11 PM Johannes Weiner <hannes@cmpxchg.org> wrote: > >>>>>> > >>>>>> On Mon, Mar 25, 2024 at 11:50:13PM +0000, Yosry Ahmed wrote: > >>>>>>> There is no logical reason to refuse storing same-filled pages more > >>>>>>> efficiently and opt for compression. Remove the userspace knob. > >>>>>>> > >>>>>>> Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > >>>>>> > >>>>>> Acked-by: Johannes Weiner <hannes@cmpxchg.org> > >>>>>> > >>>>>> I also think the non_same_filled_pages_enabled option should go > >>>>>> away. Both of these tunables are pretty bizarre. > >>>>> > >>>>> Happy to remove both in the next version :) > >>>> > >>>> I thought non_same_filled_pages_enabled was introduced with the > >>>> initial support for same-filled pages, but it was introduced > >>>> separately (and much more recently): > >>>> https://lore.kernel.org/all/7dbafa963e8bab43608189abbe2067f4b9287831.1641247624.git.maciej.szmigiero@oracle.com/ > >>>> > >>>> I am CCing Maciej to hear more about the use case for this. > >>> > >>> Thanks for CCing me. > >>> > >>> I introduced "non_same_filled_pages_enabled" a few years ago to > >>> enable using zswap in a lightweight mode where it is only used for > >>> its ability to store same-filled pages effectively. > >> > >> But all the pages it rejects go to disk swap instead, which is much > >> slower than compression... > >> > >>> As far as I remember, there were some interactions between full > >>> zswap and the cgroup memory controller - like, it made it easier > >>> for an aggressive workload to exceed its cgroup memory.high limits. > >> > >> Ok, that makes sense! A container fairness measure, rather than a > >> performance optimization. > >> > >> Fair enough, but that's moot then with cgroup accounting of the > >> backing memory, f4840ccfca25 ("zswap: memcg accounting"). > > > > Right, this should no longer be needed with the zswap charging. > > > > Maciej, is this still being used on kernels with f4840ccfca25 (5.19+)? > > Any objections to removing it now? > > I don't object to its removal as long as stable kernel trees aren't > affected. Yeah this isn't something that would be backported to stable kernels. Thanks for confirming.
diff --git a/mm/zswap.c b/mm/zswap.c index 498a6c5839bef..0fc27ae950c74 100644 --- a/mm/zswap.c +++ b/mm/zswap.c @@ -123,14 +123,6 @@ static unsigned int zswap_accept_thr_percent = 90; /* of max pool size */ module_param_named(accept_threshold_percent, zswap_accept_thr_percent, uint, 0644); -/* - * Enable/disable handling same-value filled pages (enabled by default). - * If disabled every page is considered non-same-value filled. - */ -static bool zswap_same_filled_pages_enabled = true; -module_param_named(same_filled_pages_enabled, zswap_same_filled_pages_enabled, - bool, 0644); - /* Enable/disable handling non-same-value filled pages (enabled by default) */ static bool zswap_non_same_filled_pages_enabled = true; module_param_named(non_same_filled_pages_enabled, zswap_non_same_filled_pages_enabled, @@ -1392,9 +1384,6 @@ static bool zswap_is_folio_same_filled(struct folio *folio, unsigned long *value unsigned int pos, last_pos = PAGE_SIZE / sizeof(*page) - 1; bool ret; - if (!zswap_same_filled_pages_enabled) - return false; - page = kmap_local_folio(folio, 0); val = page[0];
There is no logical reason to refuse storing same-filled pages more efficiently and opt for compression. Remove the userspace knob. Signed-off-by: Yosry Ahmed <yosryahmed@google.com> --- mm/zswap.c | 11 ----------- 1 file changed, 11 deletions(-)