| Message ID | 20240126083015.3557006-1-chengming.zhou@linux.dev (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] mm/zswap: don't return LRU_SKIP if we have dropped lru lock | expand |
On Fri, Jan 26, 2024 at 08:30:14AM +0000, chengming.zhou@linux.dev wrote: > From: Chengming Zhou <zhouchengming@bytedance.com> > > LRU_SKIP can only be returned if we don't ever dropped lru lock, or > we need to return LRU_RETRY to restart from the head of lru list. Good catch. Can you mention the possible consequences in the log? "Otherwise, the iteration might continue from a cursor position that was freed while the locks were dropped."? > Actually we may need to introduce another LRU_STOP to really terminate > the ongoing shrinking scan process, when we encounter a warm page > already in the swap cache. The current list_lru implementation > doesn't have this function to early break from __list_lru_walk_one. > > Fixes: b5ba474f3f51 ("zswap: shrink zswap pool based on memory pressure") > Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com> Acked-by: Johannes Weiner <hannes@cmpxchg.org>
On Fri, Jan 26, 2024 at 12:31 AM <chengming.zhou@linux.dev> wrote: > > From: Chengming Zhou <zhouchengming@bytedance.com> > > LRU_SKIP can only be returned if we don't ever dropped lru lock, or > we need to return LRU_RETRY to restart from the head of lru list. Ooops. You're right! I just double checked and only LRU_REMOVED_RETRY and LRU_RETRY indicate we might have dropped the lock. My bad. > > Actually we may need to introduce another LRU_STOP to really terminate > the ongoing shrinking scan process, when we encounter a warm page Yup. This is what I was trying (and failing) to do. To be honest, this needs to be even stronger: short-circuit ALL concurrent/ongoing zswap shrinker scan processes that are touching this memcg (as they will also shrink into warmer regions going forward). But that's a bit more engineering to do. LRU_STOP, which stops this scan process, would be a good place to start. > already in the swap cache. The current list_lru implementation > doesn't have this function to early break from __list_lru_walk_one. > > Fixes: b5ba474f3f51 ("zswap: shrink zswap pool based on memory pressure") > Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com> Reviewed-by: Nhat Pham <nphamcs@gmail.com> > --- > mm/zswap.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/mm/zswap.c b/mm/zswap.c > index 00e90b9b5417..81cb3790e0dd 100644 > --- a/mm/zswap.c > +++ b/mm/zswap.c > @@ -901,10 +901,8 @@ static enum lru_status shrink_memcg_cb(struct list_head *item, struct list_lru_o > * into the warmer region. We should terminate shrinking (if we're in the dynamic > * shrinker context). > */ > - if (writeback_result == -EEXIST && encountered_page_in_swapcache) { > - ret = LRU_SKIP; > + if (writeback_result == -EEXIST && encountered_page_in_swapcache) > *encountered_page_in_swapcache = true; > - } > > goto put_unlock; > } > -- > 2.40.1 >
On 2024/1/26 22:28, Johannes Weiner wrote: > On Fri, Jan 26, 2024 at 08:30:14AM +0000, chengming.zhou@linux.dev wrote: >> From: Chengming Zhou <zhouchengming@bytedance.com> >> >> LRU_SKIP can only be returned if we don't ever dropped lru lock, or >> we need to return LRU_RETRY to restart from the head of lru list. > > Good catch. Can you mention the possible consequences in the log? > > "Otherwise, the iteration might continue from a cursor position that > was freed while the locks were dropped."? Good, will do. > >> Actually we may need to introduce another LRU_STOP to really terminate >> the ongoing shrinking scan process, when we encounter a warm page >> already in the swap cache. The current list_lru implementation >> doesn't have this function to early break from __list_lru_walk_one. >> >> Fixes: b5ba474f3f51 ("zswap: shrink zswap pool based on memory pressure") >> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com> > > Acked-by: Johannes Weiner <hannes@cmpxchg.org> Thanks.
On 2024/1/27 02:01, Nhat Pham wrote: > On Fri, Jan 26, 2024 at 12:31 AM <chengming.zhou@linux.dev> wrote: >> >> From: Chengming Zhou <zhouchengming@bytedance.com> >> >> LRU_SKIP can only be returned if we don't ever dropped lru lock, or >> we need to return LRU_RETRY to restart from the head of lru list. > > Ooops. You're right! I just double checked and only LRU_REMOVED_RETRY > and LRU_RETRY indicate we might have dropped the lock. My bad. > >> >> Actually we may need to introduce another LRU_STOP to really terminate >> the ongoing shrinking scan process, when we encounter a warm page > > Yup. This is what I was trying (and failing) to do. To be honest, this > needs to be even stronger: short-circuit ALL concurrent/ongoing zswap > shrinker scan processes that are touching this memcg (as they will > also shrink into warmer regions going forward). But that's a bit more > engineering to do. LRU_STOP, which stops this scan process, would be a > good place to start. Good suggestion, will look into that more later. > >> already in the swap cache. The current list_lru implementation >> doesn't have this function to early break from __list_lru_walk_one. >> >> Fixes: b5ba474f3f51 ("zswap: shrink zswap pool based on memory pressure") >> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com> > > Reviewed-by: Nhat Pham <nphamcs@gmail.com> Thanks. > >> --- >> mm/zswap.c | 4 +--- >> 1 file changed, 1 insertion(+), 3 deletions(-) >> >> diff --git a/mm/zswap.c b/mm/zswap.c >> index 00e90b9b5417..81cb3790e0dd 100644 >> --- a/mm/zswap.c >> +++ b/mm/zswap.c >> @@ -901,10 +901,8 @@ static enum lru_status shrink_memcg_cb(struct list_head *item, struct list_lru_o >> * into the warmer region. We should terminate shrinking (if we're in the dynamic >> * shrinker context). >> */ >> - if (writeback_result == -EEXIST && encountered_page_in_swapcache) { >> - ret = LRU_SKIP; >> + if (writeback_result == -EEXIST && encountered_page_in_swapcache) >> *encountered_page_in_swapcache = true; >> - } >> >> goto put_unlock; >> } >> -- >> 2.40.1 >>
diff --git a/mm/zswap.c b/mm/zswap.c index 00e90b9b5417..81cb3790e0dd 100644 --- a/mm/zswap.c +++ b/mm/zswap.c @@ -901,10 +901,8 @@ static enum lru_status shrink_memcg_cb(struct list_head *item, struct list_lru_o * into the warmer region. We should terminate shrinking (if we're in the dynamic * shrinker context). */ - if (writeback_result == -EEXIST && encountered_page_in_swapcache) { - ret = LRU_SKIP; + if (writeback_result == -EEXIST && encountered_page_in_swapcache) *encountered_page_in_swapcache = true; - } goto put_unlock; }