On 2023/12/15 16:12, Qiuxu Zhuo wrote: > During the process of splitting a hw-poisoned huge page, it is possible > for the reference count of the huge page to be increased by the threads > within the affected process, leading to a failure in splitting the > hw-poisoned huge page with an error code of -EAGAIN. > > This issue can be reproduced when doing memory error injection to a > multiple-thread process, and the error occurs within a huge page. > The call path with the returned -EAGAIN during the testing is shown below: > > memory_failure() > try_to_split_thp_page() > split_huge_page() > split_huge_page_to_list() { > ... > Step A: can_split_folio() - Checked that the thp can be split. > Step B: unmap_folio() > Step C: folio_ref_freeze() - Failed and returned -EAGAIN. > ... > } > > The testing logs indicated that some huge pages were split successfully > via the call path above (Step C was successful for these huge pages). > However, some huge pages failed to split due to a failure at Step C, and > it was observed that the reference count of the huge page increased between > Step A and Step C. > > Testing has shown that after receiving -EAGAIN, simply re-splitting the > hw-poisoned huge page within memory_failure() always results in the same > -EAGAIN. This is possible because memory_failure() is executed in the > currently affected process. Before this process exits memory_failure() and > is terminated, its threads could increase the reference count of the > hw-poisoned page. > > To address this issue, employ the kernel worker to re-split the hw-poisoned > huge page. By the time this worker begins re-splitting the hw-poisoned huge > page, the affected process has already been terminated, preventing its > threads from increasing the reference count. Experimental results have > consistently shown that this worker successfully re-splits these > hw-poisoned huge pages on its first attempt. > > The kernel log (before): > [ 1116.862895] Memory failure: 0x4097fa7: recovery action for unsplit thp: Ignored > > The kernel log (after): > [ 793.573536] Memory failure: 0x2100dda: recovery action for unsplit thp: Delayed > [ 793.574666] Memory failure: 0x2100dda: split unsplit thp successfully. > > Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@xxxxxxxxx> Thanks for your patch. Except for the comment from Naoya, I have some questions about the code itself. > --- > mm/memory-failure.c | 73 +++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 71 insertions(+), 2 deletions(-) > > diff --git a/mm/memory-failure.c b/mm/memory-failure.c > index 660c21859118..0db4cf712a78 100644 > --- a/mm/memory-failure.c > +++ b/mm/memory-failure.c > @@ -72,6 +72,60 @@ atomic_long_t num_poisoned_pages __read_mostly = ATOMIC_LONG_INIT(0); > > static bool hw_memory_failure __read_mostly = false; > > +#define SPLIT_THP_MAX_RETRY_CNT 10 > +#define SPLIT_THP_INIT_DELAYED_MS 1 > + > +static bool split_thp_pending; > + > +struct split_thp_req { > + struct delayed_work work; > + struct page *thp; > + int retries; > +}; > + > +static void split_thp_work_fn(struct work_struct *work) > +{ > + struct split_thp_req *req = container_of(work, typeof(*req), work.work); > + int ret; > + > + /* Split the thp. */ > + get_page(req->thp); Can req->thp be freed when split_thp_work_fn is scheduled ? > + lock_page(req->thp); > + ret = split_huge_page(req->thp); > + unlock_page(req->thp); > + put_page(req->thp); > + > + /* Retry with an exponential backoff. */ > + if (ret && ++req->retries < SPLIT_THP_MAX_RETRY_CNT) { > + schedule_delayed_work(to_delayed_work(work), > + msecs_to_jiffies(SPLIT_THP_INIT_DELAYED_MS << req->retries)); > + return; > + } > + > + pr_err("%#lx: split unsplit thp %ssuccessfully.\n", page_to_pfn(req->thp), ret ? "un" : ""); > + kfree(req); > + split_thp_pending = false; split_thp_pending is not protected against split_thp_delayed? Though this race should be benign. Thanks.