Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230707201904.953262-1-jiaqiyan@google.com> <20230707201904.953262-3-jiaqiyan@google.com>
 <6682284d-7ad3-9b59-687d-899f4d08d911@huawei.com> <CACw3F50k9WJr7WgHS-dRxJRfuXPbq2adUBLeFcKRjmm2D6qf-g@mail.gmail.com>
 <CACw3F52Pj+SeB+dD2Cjkr-bX-OZkmCpL1s6SO1aHDvaD37YZBg@mail.gmail.com> <20230711180159.GA3887@monkey>
In-Reply-To: <20230711180159.GA3887@monkey>
From:   Jiaqi Yan <jiaqiyan@google.com>
Date:   Tue, 11 Jul 2023 15:22:52 -0700
Message-ID: <CACw3F53s6pX50TV3Lq79=THsxff0T1eS2wUdLZSp1JL8gFHQyg@mail.gmail.com>
Subject: Re: [PATCH v3 2/4] mm/hwpoison: check if a subpage of a hugetlb folio
 is raw HWPOISON
To:     Mike Kravetz <mike.kravetz@oracle.com>
Cc:     Miaohe Lin <linmiaohe@huawei.com>, akpm@linux-foundation.org,
        naoya.horiguchi@nec.com, songmuchun@bytedance.com,
        shy828301@gmail.com, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, duenwen@google.com,
        axelrasmussen@google.com, jthoughton@google.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Jul 11, 2023 at 11:02=E2=80=AFAM Mike Kravetz <mike.kravetz@oracle.=
com> wrote:
>
> On 07/11/23 10:05, Jiaqi Yan wrote:
> > On Mon, Jul 10, 2023 at 8:16=E2=80=AFAM Jiaqi Yan <jiaqiyan@google.com>=
 wrote:
> > > On Fri, Jul 7, 2023 at 7:57=E2=80=AFPM Miaohe Lin <linmiaohe@huawei.c=
om> wrote:
> > > > On 2023/7/8 4:19, Jiaqi Yan wrote:
> > > >
> > > > > +             if (subpage =3D=3D p->page) {
> > > > > +                     ret =3D true;
> > > > > +                     break;
> > > > > +             }
> > > > > +     }
> > > > > +
> > > > > +     return ret;
> > > > >  }
> > > >
> > > > It seems there's a race between __is_raw_hwp_subpage and unpoison_m=
emory:
> > > >   unpoison_memory               __is_raw_hwp_subpage
> > > >                                   if (!folio_test_hwpoison(folio)) =
-- hwpoison is set
> > > >     folio_free_raw_hwp            llist_for_each_entry_safe raw_hwp=
_list
> > > >       llist_del_all                 ..
> > > >     folio_test_clear_hwpoison
> > > >
> > >
> > > Thanks Miaohe for raising this concern.
> > >
> > > > But __is_raw_hwp_subpage is used in hugetlbfs, unpoison_memory coul=
dn't reach here because there's a
> > > > folio_mapping =3D=3D NULL check before folio_free_raw_hwp.
> > >
> > > I agree. But in near future I do want to make __is_raw_hwp_subpage
> > > work for shared-mapping hugetlb, so it would be nice to work with
> > > unpoison_memory. It doesn't seem to me that holding mf_mutex in
> > > __is_raw_hwp_subpage is nice or even absolutely correct. Let me think
> > > if I can come up with something in v4.
> >
> > At my 2nd thought, if __is_raw_hwp_subpage simply takes mf_mutex
> > before llist_for_each_entry, it will introduce a deadlock:
> >
> > unpoison_memory                       __is_raw_hwp_subpage
> >   held mf_mutex                         held hugetlb_lock
> >   get_hwpoison_hugetlb_folio            attempts mf_mutex
> >     attempts hugetlb lock
> >
> > Not for this patch series, but for future, is it a good idea to make
> > mf_mutex available to hugetlb code? Then enforce the order of locking
> > to be mf_mutex first, hugetlb_lock second? I believe this is the
> > current locking pattern / order for try_memory_failure_hugetlb.
>
> I think only holding mf_mutex in __is_raw_hwp_subpage would be sufficient
> to prevent races with unpoison_memory.  memory failure code needs to take
> both mf_mutex and hugetlb_lock.  The hugetlb lock is to prevent hugetlb
> page state changes.  IIUC, __is_raw_hwp_subpage is only taking hugetlb_lo=
ck
> to prevent races with memory failure code.

Thanks Mike, I think mf_mutex is a simple and correct solution. I will
drop hugetlb_lock from __is_raw_hwp_subpage in v4.

>
> Of course, I could be missing something as there are subtle issues with
> locking in the memory failure code.
> --
> Mike Kravetz