On Sat, Apr 17, 2021 at 01:47:51PM +0800, Aili Yao wrote:
> On Tue, 13 Apr 2021 07:43:17 +0900
> Naoya Horiguchi <nao.horigu...@gmail.com> wrote:
>
> > Hi,
> >
> > I wrote this patchset to materialize what I think is the current
> > allowable solution mentioned by the previous discussion [1].
> > I simply borrowed Tony's mutex patch and Aili's return code patch,
> > then I queued another one to find error virtual address in the best
> > effort manner. I know that this is not a perfect solution, but
> > should work for some typical case.
> >
> > My simple testing showed this patchset seems to work as intended,
> > but if you have the related testcases, could you please test and
> > let me have some feedback?
> >
> > Thanks,
> > Naoya Horiguchi
> >
> > [1]:
> > https://lore.kernel.org/linux-mm/20210331192540.2141052f@alex-virtual-machine/
> > ---
> > Summary:
> >
> > Aili Yao (1):
> > mm,hwpoison: return -EHWPOISON when page already
> >
> > Naoya Horiguchi (1):
> > mm,hwpoison: add kill_accessing_process() to find error virtual
> > address
> >
> > Tony Luck (1):
> > mm/memory-failure: Use a mutex to avoid memory_failure() races
> >
> > arch/x86/kernel/cpu/mce/core.c | 13 +++-
> > include/linux/swapops.h | 5 ++
> > mm/memory-failure.c | 166
> > ++++++++++++++++++++++++++++++++++++++++-
> > 3 files changed, 178 insertions(+), 6 deletions(-)
>
> Hi Naoya,
>
> Thanks for your patch and complete fix for this race issue.
>
> I test your patches, mainly it worked as expected, but in some cases it
> failed, I checked it
> and find some doubt places, could you help confirm it?
>
> 1. there is a compile warning:
> static int hwpoison_pte_range(pmd_t *pmdp, unsigned long addr,
> unsigned long end, struct mm_walk *walk)
> {
> struct hwp_walk *hwp = (struct hwp_walk *)walk->private;
> int ret; ---- here
>
> It seems this ret may not be initialized, and some time ret may be error
> retruned?
Yes, I'll initialize with 0.
>
> and for this:
> static int check_hwpoisoned_entry(pte_t pte, unsigned long addr, short shift,
> unsigned long poisoned_pfn, struct to_kill *tk)
> {
> unsigned long pfn;
>
> I think it better to be initialized too.
OK.
>
> 2. In the function hwpoison_pte_range():
> if (pfn <= hwp->pfn && hwp->pfn < pfn + PMD_SIZE) this check seem we should
> use PMD_SIZE/PAGE_SIZE or some macro like this?
Thanks, that's right. HPAGE_PMD_NR seems to fit here.
We also need "#ifdef CONFIG_TRANSPARENT_HUGEPAGE" to use it.
>
> 3. unsigned long hwpoison_vaddr = addr + (hwp->pfn << PAGE_SHIFT &
> ~PMD_MASK); this seems not exact accurate?
As operators precedence rule, we evaluate this in the following order:
1. ~ (bitwise NOT),
2. << (bitwise left shift), and
3. & (bitwise AND).
So this part are equivalent with ((hwp->pfn << PAGE_SHIFT) & (~PMD_MASK)),
which should properly calculate an address offset within a pmd.
>
> 4. static int set_to_kill(struct to_kill *tk, unsigned long addr, short shift)
> {
> if (tk->addr) { --- I am not sure about this check and if it will
> lead failure.
> return 1;
> }
> In my test, it seems sometimes it will hit this branch, I don't know it's
> multi entry issue or multi posion issue.
> when i get to this fail, there is not enough log for this, but i can't
> reproduce it after that.
As you commented above, this version is buggy, and that might
make you see the random failure. Sorry about that.
>
> wolud you help confirm this and if any changes, please post again and I will
> do the test again.
I'll send the fixed one for patch 3/3 soon later.
Thanks,
Naoya Horiguchi
