Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
From:   David Hildenbrand <david@redhat.com>
Mime-Version: 1.0 (1.0)
Subject: Re: [PATCH -next v4] mm/hotplug: silence a lockdep splat with printk()
Date:   Fri, 17 Jan 2020 20:15:34 +0100
Message-Id: <06AE045D-F167-406B-A78B-CAE246058C9D@redhat.com>
References: <00155F33-17C6-4051-A8F9-CCD9414F400D@lca.pw>
Cc:     Michal Hocko <mhocko@kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>,
        pmladek@suse.com, rostedt@goodmis.org, peterz@infradead.org,
        david@redhat.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org
In-Reply-To: <00155F33-17C6-4051-A8F9-CCD9414F400D@lca.pw>
To:     Qian Cai <cai@lca.pw>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


> Am 17.01.2020 um 19:49 schrieb Qian Cai <cai@lca.pw>:
>=20
> =EF=BB=BF
>=20
>> On Jan 17, 2020, at 10:46 AM, Michal Hocko <mhocko@kernel.org> wrote:
>>=20
>>> On Fri 17-01-20 10:05:12, Qian Cai wrote:
>>>=20
>>>=20
>>>> On Jan 17, 2020, at 9:39 AM, Michal Hocko <mhocko@kernel.org> wrote:
>>>>=20
>>>> Thanks a lot. Having it in a separate patch would be great.
>>>=20
>>> I was thinking about removing that WARN together in this v5 patch,
>>> so there is less churn to touch the same function again. However, I
>>> am fine either way, so just shout out if you feel strongly towards a
>>> separate patch.
>>=20
>> I hope you meant moving rather than removing ;). The warning is useful
>> because we shouldn't see unmovable pages in the movable zone. And a
>> separate patch makes more sense because the justification is slightly
>> different. We do not want to have a way for userspace to trigger the
>> warning from userspace - even though it shouldn't be possible, but
>> still. Only the offlining path should complain.
>=20
> Something like this?
>=20
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 621716a25639..32c854851e1f 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -8307,7 +8307,6 @@ struct page *has_unmovable_pages(struct zone *zone, s=
truct page *page,
>        }
>        return NULL;
> unmovable:
> -       WARN_ON_ONCE(zone_idx(zone) =3D=3D ZONE_MOVABLE);
>        return pfn_to_page(pfn + iter);
> }
>=20
> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
> index e70586523ca3..08571b515d9f 100644
> --- a/mm/page_isolation.c
> +++ b/mm/page_isolation.c
> @@ -54,9 +54,11 @@ static int set_migratetype_isolate(struct page *page, i=
nt migratetype, int isol_
>=20
> out:
>        spin_unlock_irqrestore(&zone->lock, flags);
> +
>        if (!ret)
>                drain_all_pages(zone);
>        else if ((isol_flags & REPORT_FAILURE) && unmovable)

We have a dedicated flag for the offlining part.

> +               WARN_ON_ONCE(zone_idx(zone) =3D=3D ZONE_MOVABLE);
>                /*
>                 * printk() with zone->lock held will guarantee to trigger a=

>                 * lockdep splat, so defer it here.
>=20

So, are we fine with unmovable data ending up in ZONE_MOVABLE as long as we c=
an offline it?=20

This might make my life in virtio-mem a little easier (I can unplug chunks f=
alling into ZONE_MOVABLE).=