| Message ID | 20230625113430.2310385-1-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: memory-failure: remove unneeded page state check in shake_page() | expand |
On Sun, Jun 25, 2023 at 07:34:30PM +0800, Miaohe Lin wrote: > Remove unneeded PageLRU(p) and is_free_buddy_page(p) check as slab caches > are not shrunk now. This check can be added back when a lightweight range > based shrinker is available. I think your logic is faulty. Based on the reasoning above, this patch should be: > - if (!PageSlab(p)) { > - lru_add_drain_all(); > - if (PageLRU(p) || is_free_buddy_page(p)) > - return; > - } > + if (PageSlab(p)) > + return; > + > + lru_add_drain_all(); + if (PageLRU(p) || is_free_buddy_page(p)) + return; but maybe it's the description that is wrong.
On Sun, Jun 25, 2023 at 07:34:30PM +0800, Miaohe Lin wrote: > Remove unneeded PageLRU(p) and is_free_buddy_page(p) check as slab caches > are not shrunk now. This check can be added back when a lightweight range > based shrinker is available. On further review, I think you've misunderstood the entire situation here and no patch should be applied. > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > --- > mm/memory-failure.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/mm/memory-failure.c b/mm/memory-failure.c > index 5b663eca1f29..92f951df3e87 100644 > --- a/mm/memory-failure.c > +++ b/mm/memory-failure.c > @@ -373,11 +373,10 @@ void shake_page(struct page *p) > if (PageHuge(p)) > return; > > - if (!PageSlab(p)) { > - lru_add_drain_all(); > - if (PageLRU(p) || is_free_buddy_page(p)) > - return; > - } > + if (PageSlab(p)) > + return; > + > + lru_add_drain_all(); > > /* > * TODO: Could shrink slab caches here if a lightweight range-based > -- > 2.27.0 > >
On Sun, Jun 25, 2023 at 07:34:30PM +0800, Miaohe Lin wrote: > Remove unneeded PageLRU(p) and is_free_buddy_page(p) check as slab caches > are not shrunk now. This check can be added back when a lightweight range > based shrinker is available. > > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> This looks to me a good cleanup because the result of "if (PageLRU(p) || is_free_buddy_page(p))" check is not used, so the check itself is unneeded. > --- > mm/memory-failure.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/mm/memory-failure.c b/mm/memory-failure.c > index 5b663eca1f29..92f951df3e87 100644 > --- a/mm/memory-failure.c > +++ b/mm/memory-failure.c > @@ -373,11 +373,10 @@ void shake_page(struct page *p) > if (PageHuge(p)) > return; > > - if (!PageSlab(p)) { > - lru_add_drain_all(); > - if (PageLRU(p) || is_free_buddy_page(p)) > - return; > - } > + if (PageSlab(p)) > + return; > + > + lru_add_drain_all(); > > /* > * TODO: Could shrink slab caches here if a lightweight range-based I think that this TODO comment can be put together with "if (PageSlab)" block. Thanks, Naoya Horiguchi
On 2023/6/26 3:50, Matthew Wilcox wrote: > On Sun, Jun 25, 2023 at 07:34:30PM +0800, Miaohe Lin wrote: >> Remove unneeded PageLRU(p) and is_free_buddy_page(p) check as slab caches >> are not shrunk now. This check can be added back when a lightweight range >> based shrinker is available. > > On further review, I think you've misunderstood the entire situation > here and no patch should be applied. Thanks for your review. I think you might misunderstood the patch. What I want to do here is remove the unneeded if block: if (PageLRU(p) || is_free_buddy_page(p)) return; Because nothing is done after the check. So this check is useless and should be removed. Thanks.
On 2023/6/26 8:52, Naoya Horiguchi wrote: > On Sun, Jun 25, 2023 at 07:34:30PM +0800, Miaohe Lin wrote: >> Remove unneeded PageLRU(p) and is_free_buddy_page(p) check as slab caches >> are not shrunk now. This check can be added back when a lightweight range >> based shrinker is available. >> >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > > This looks to me a good cleanup because the result of > "if (PageLRU(p) || is_free_buddy_page(p))" check is not used, so the check > itself is unneeded. > >> --- >> mm/memory-failure.c | 9 ++++----- >> 1 file changed, 4 insertions(+), 5 deletions(-) >> >> diff --git a/mm/memory-failure.c b/mm/memory-failure.c >> index 5b663eca1f29..92f951df3e87 100644 >> --- a/mm/memory-failure.c >> +++ b/mm/memory-failure.c >> @@ -373,11 +373,10 @@ void shake_page(struct page *p) >> if (PageHuge(p)) >> return; >> >> - if (!PageSlab(p)) { >> - lru_add_drain_all(); >> - if (PageLRU(p) || is_free_buddy_page(p)) >> - return; >> - } >> + if (PageSlab(p)) >> + return; >> + >> + lru_add_drain_all(); >> >> /* >> * TODO: Could shrink slab caches here if a lightweight range-based > > I think that this TODO comment can be put together with "if (PageSlab)" block. Thanks for your comment and advice. Do you mean something like below? diff --git a/mm/memory-failure.c b/mm/memory-failure.c index 5b663eca1f29..66e7b3ceaf2d 100644 --- a/mm/memory-failure.c +++ b/mm/memory-failure.c @@ -372,17 +372,14 @@ void shake_page(struct page *p) { if (PageHuge(p)) return; - - if (!PageSlab(p)) { - lru_add_drain_all(); - if (PageLRU(p) || is_free_buddy_page(p)) - return; - } - /* * TODO: Could shrink slab caches here if a lightweight range-based * shrinker will be available. */ + if (PageSlab(p)) + return; + + lru_add_drain_all(); } EXPORT_SYMBOL_GPL(shake_page); Thanks.
On Mon, Jun 26, 2023 at 09:43:44AM +0800, Miaohe Lin wrote: > On 2023/6/26 8:52, Naoya Horiguchi wrote: > > On Sun, Jun 25, 2023 at 07:34:30PM +0800, Miaohe Lin wrote: > >> Remove unneeded PageLRU(p) and is_free_buddy_page(p) check as slab caches > >> are not shrunk now. This check can be added back when a lightweight range > >> based shrinker is available. > >> > >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > > > > This looks to me a good cleanup because the result of > > "if (PageLRU(p) || is_free_buddy_page(p))" check is not used, so the check > > itself is unneeded. > > > >> --- > >> mm/memory-failure.c | 9 ++++----- > >> 1 file changed, 4 insertions(+), 5 deletions(-) > >> > >> diff --git a/mm/memory-failure.c b/mm/memory-failure.c > >> index 5b663eca1f29..92f951df3e87 100644 > >> --- a/mm/memory-failure.c > >> +++ b/mm/memory-failure.c > >> @@ -373,11 +373,10 @@ void shake_page(struct page *p) > >> if (PageHuge(p)) > >> return; > >> > >> - if (!PageSlab(p)) { > >> - lru_add_drain_all(); > >> - if (PageLRU(p) || is_free_buddy_page(p)) > >> - return; > >> - } > >> + if (PageSlab(p)) > >> + return; > >> + > >> + lru_add_drain_all(); > >> > >> /* > >> * TODO: Could shrink slab caches here if a lightweight range-based > > > > I think that this TODO comment can be put together with "if (PageSlab)" block. > > Thanks for your comment and advice. Do you mean something like below? Yes, this is what I mean. Feel free to add my ack when posting v2. Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com> Thank you. > > diff --git a/mm/memory-failure.c b/mm/memory-failure.c > index 5b663eca1f29..66e7b3ceaf2d 100644 > --- a/mm/memory-failure.c > +++ b/mm/memory-failure.c > @@ -372,17 +372,14 @@ void shake_page(struct page *p) > { > if (PageHuge(p)) > return; > - > - if (!PageSlab(p)) { > - lru_add_drain_all(); > - if (PageLRU(p) || is_free_buddy_page(p)) > - return; > - } > - > /* > * TODO: Could shrink slab caches here if a lightweight range-based > * shrinker will be available. > */ > + if (PageSlab(p)) > + return; > + > + lru_add_drain_all(); > } > EXPORT_SYMBOL_GPL(shake_page); > > Thanks. > >
diff --git a/mm/memory-failure.c b/mm/memory-failure.c index 5b663eca1f29..92f951df3e87 100644 --- a/mm/memory-failure.c +++ b/mm/memory-failure.c @@ -373,11 +373,10 @@ void shake_page(struct page *p) if (PageHuge(p)) return; - if (!PageSlab(p)) { - lru_add_drain_all(); - if (PageLRU(p) || is_free_buddy_page(p)) - return; - } + if (PageSlab(p)) + return; + + lru_add_drain_all(); /* * TODO: Could shrink slab caches here if a lightweight range-based
Remove unneeded PageLRU(p) and is_free_buddy_page(p) check as slab caches are not shrunk now. This check can be added back when a lightweight range based shrinker is available. Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> --- mm/memory-failure.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)