Re: + mm-thp-always-specify-disabled-vmas-as-nh-in-smaps.patch added to -mm tree

Re: + mm-thp-always-specify-disabled-vmas-as-nh-in-smaps.patch added to -mm tree

[Vlastimil Babka]

On 12/28/18 9:18 AM, Michal Hocko wrote:
> On Thu 27-12-18 21:31:00, Andrew Morton wrote:
>> On Thu, 27 Dec 2018 14:11:14 +0300 "Kirill A. Shutemov" <kirill@shutemov.name> wrote:
>>
>>> On Mon, Dec 24, 2018 at 10:17:31AM +0100, Michal Hocko wrote:
>>>> On Mon 24-12-18 01:05:57, David Rientjes wrote:
>>>> [...]
>>>>> I'm not interested in having a 100 email thread about this when a clear 
>>>>> and simple fix exists that actually doesn't break user code.
>>>>
>>>> You are breaking everybody who really wants to query MADV_NOHUGEPAGE
>>>> status by this flag. Is there anybody doing that?
>>>
>>> Yes.
>>>
>>> https://github.com/checkpoint-restore/criu/blob/v3.11/criu/proc_parse.c#L143
>>
>> Ugh.  So the regression fix causes a regression?
> 
> Yes. The patch from David will hardcode the nohugepage vm flag if the
> THP was disabled by the prctl at the time of the snapshot. And if the
> application later enables THP by the prctl then existing mappings would
> never get the THP enabled status back.
> 
> This is the kind of a potential regression I was poiting out earlier
> when explaining that the patch encodes the logic into the flag exporting
> and that means that whoever wants to get the raw value of the flag will
> not be able to do so. Please note that the raw value is exactly what
> this interface is documented and supposed to export. And as the
> documentation explains it is implementation specific and anybody to use
> it should be careful.

Let's add some CRIU guys in the loop (dunno if the right ones). We're
discussing David's patch [1] that makes 'nh' and 'hg' flags reported in
/proc/pid/smaps (and set by madvise) overridable by
prctl(PR_SET_THP_DISABLE). This was sort of accidental behavior (but
only for mappings created after the prctl call) before 4.13 commit
1860033237d4 ("mm: make PR_SET_THP_DISABLE immediately active").

For David's userspace that commit is a regression as there are false
positives when checking for vma's that are eligible for THP (=don't have
the 'nh' flag in smaps) but didn't really obtain THP's. The userspace
assumes it's due to fragmentation (=bad) and cannot know that it's due
to the prctl(). But we fear that making prctl() affect smaps vma flags
means that CRIU can't query them accurately anymore, and thus it's a
regression for CRIU. Can you comment on that?
Michal has a patch [2] that reports the prctl() status separately, but
that doesn't help David's existing userspace. For CRIU this also won't
help as long the smaps vma flags still silently included the prctl()
status. Do you see some solution that would work for everybody?

[1]
https://www.ozlabs.org/~akpm/mmots/broken-out/mm-thp-always-specify-disabled-vmas-as-nh-in-smaps.patch
[2]
https://www.ozlabs.org/~akpm/mmots/broken-out/mm-proc-report-pr_set_thp_disable-in-proc.patch

Re: + mm-thp-always-specify-disabled-vmas-as-nh-in-smaps.patch added to -mm tree

[Michal Hocko]

On Fri 28-12-18 11:54:17, Vlastimil Babka wrote:
[...]
> Michal has a patch [2] that reports the prctl() status separately, but
> that doesn't help David's existing userspace.

This is something that is not really clear to me. I have asked several
times already and never got any reply IIRC. The code is obviously very
specific for their setup. Why is it not viable to update the code
in question now that a long term, easily backportable and properly
supported API exists?

I do understand that a lack of such an API pushes people to use whatever
sounds usable (even when undocumented) but we do have that interface
now. So what is the roadblock?

Or do we really want to make vma flags properly supported and carved in
stone interface?
-- 
Michal Hocko
SUSE Labs

Re: + mm-thp-always-specify-disabled-vmas-as-nh-in-smaps.patch added to -mm tree

[Cyrill Gorcunov]

On Fri, Dec 28, 2018 at 11:54:17AM +0100, Vlastimil Babka wrote:
...
> Let's add some CRIU guys in the loop (dunno if the right ones). We're
> discussing David's patch [1] that makes 'nh' and 'hg' flags reported in
> /proc/pid/smaps (and set by madvise) overridable by
> prctl(PR_SET_THP_DISABLE). This was sort of accidental behavior (but
> only for mappings created after the prctl call) before 4.13 commit
> 1860033237d4 ("mm: make PR_SET_THP_DISABLE immediately active").
> 
> For David's userspace that commit is a regression as there are false
> positives when checking for vma's that are eligible for THP (=don't have
> the 'nh' flag in smaps) but didn't really obtain THP's. The userspace
> assumes it's due to fragmentation (=bad) and cannot know that it's due
> to the prctl(). But we fear that making prctl() affect smaps vma flags
> means that CRIU can't query them accurately anymore, and thus it's a
> regression for CRIU. Can you comment on that?
> Michal has a patch [2] that reports the prctl() status separately, but
> that doesn't help David's existing userspace. For CRIU this also won't
> help as long the smaps vma flags still silently included the prctl()
> status. Do you see some solution that would work for everybody?
> 
> [1]
> https://www.ozlabs.org/~akpm/mmots/broken-out/mm-thp-always-specify-disabled-vmas-as-nh-in-smaps.patch
> [2]
> https://www.ozlabs.org/~akpm/mmots/broken-out/mm-proc-report-pr_set_thp_disable-in-proc.patch

First of all, thanks for CC'ing. Pavel and Mike should know more about
criu's part of THP. We'are using the smaps flags on later restore stage
only where we call madvise on particular vma area. But from general pov
it is fishy to hardcode nh flag into smaps report depedning on prctl flag
-- prctl and madvise are two different interfaces. Is it possible for
David to update their userspace tools instead? I know it shounds as
we're violating 'don't-break-user-space' term but with ability to
parse /proc/pid/status to figure out if thp is enabled/disabled should
not be that hard.

Re: + mm-thp-always-specify-disabled-vmas-as-nh-in-smaps.patch added to -mm tree

[Mike Rapoport]

On Fri, Dec 28, 2018 at 11:54:17AM +0100, Vlastimil Babka wrote:
> On 12/28/18 9:18 AM, Michal Hocko wrote:
> > On Thu 27-12-18 21:31:00, Andrew Morton wrote:
> >> On Thu, 27 Dec 2018 14:11:14 +0300 "Kirill A. Shutemov" <kirill@shutemov.name> wrote:
> >>
> >>> On Mon, Dec 24, 2018 at 10:17:31AM +0100, Michal Hocko wrote:
> >>>> On Mon 24-12-18 01:05:57, David Rientjes wrote:
> >>>> [...]
> >>>>> I'm not interested in having a 100 email thread about this when a clear 
> >>>>> and simple fix exists that actually doesn't break user code.
> >>>>
> >>>> You are breaking everybody who really wants to query MADV_NOHUGEPAGE
> >>>> status by this flag. Is there anybody doing that?
> >>>
> >>> Yes.
> >>>
> >>> https://github.com/checkpoint-restore/criu/blob/v3.11/criu/proc_parse.c#L143
> >>
> >> Ugh.  So the regression fix causes a regression?
> > 
> > Yes. The patch from David will hardcode the nohugepage vm flag if the
> > THP was disabled by the prctl at the time of the snapshot. And if the
> > application later enables THP by the prctl then existing mappings would
> > never get the THP enabled status back.
> > 
> > This is the kind of a potential regression I was poiting out earlier
> > when explaining that the patch encodes the logic into the flag exporting
> > and that means that whoever wants to get the raw value of the flag will
> > not be able to do so. Please note that the raw value is exactly what
> > this interface is documented and supposed to export. And as the
> > documentation explains it is implementation specific and anybody to use
> > it should be careful.
> 
> Let's add some CRIU guys in the loop (dunno if the right ones). We're
> discussing David's patch [1] that makes 'nh' and 'hg' flags reported in
> /proc/pid/smaps (and set by madvise) overridable by
> prctl(PR_SET_THP_DISABLE). This was sort of accidental behavior (but
> only for mappings created after the prctl call) before 4.13 commit
> 1860033237d4 ("mm: make PR_SET_THP_DISABLE immediately active").
> 
> For David's userspace that commit is a regression as there are false
> positives when checking for vma's that are eligible for THP (=don't have
> the 'nh' flag in smaps) but didn't really obtain THP's. The userspace
> assumes it's due to fragmentation (=bad) and cannot know that it's due
> to the prctl(). But we fear that making prctl() affect smaps vma flags
> means that CRIU can't query them accurately anymore, and thus it's a
> regression for CRIU. Can you comment on that?

CRIU parses VmFlags from /proc/pid/smaps during the checkpoint and then
uses their raw values to recreate exactly the same mappings during restore.
We use appropriate madvise() calls to re-establish VMA flags.

Implicitly setting the 'nh' flag in /proc/pid/smaps when THP usage was
restricted with prctl() will make CRIU to call madvise(MADV_NOHUPEPAGE) for
all the mappings with 'nh' set as CRIU cannot detect the actual reason for
VMA being marked as VM_NOHUGEPAGE.

As Andrew pointed out, if the restored process will re-enable THP with
prtcl(), the VMAs will retail VM_NOHUGEPAGE flag. And, I don't see how can
we work around this in CRIU.

> Michal has a patch [2] that reports the prctl() status separately, but
> that doesn't help David's existing userspace. For CRIU this also won't
> help as long the smaps vma flags still silently included the prctl()
> status. Do you see some solution that would work for everybody?

Nothing comes to mind at the moment, maybe next year ;-)

> [1]
> https://www.ozlabs.org/~akpm/mmots/broken-out/mm-thp-always-specify-disabled-vmas-as-nh-in-smaps.patch
> [2]
> https://www.ozlabs.org/~akpm/mmots/broken-out/mm-proc-report-pr_set_thp_disable-in-proc.patch
> 

-- 
Sincerely yours,
Mike.

Re: + mm-thp-always-specify-disabled-vmas-as-nh-in-smaps.patch added to -mm tree

[Mike Rapoport]

Hi,

The holidays are over and I think it's time to resurrect this thread.

On Fri, Dec 28, 2018 at 11:54:17AM +0100, Vlastimil Babka wrote:
> On 12/28/18 9:18 AM, Michal Hocko wrote:
> > On Thu 27-12-18 21:31:00, Andrew Morton wrote:
> >> On Thu, 27 Dec 2018 14:11:14 +0300 "Kirill A. Shutemov" <kirill@shutemov.name> wrote:
> >>
> >>> On Mon, Dec 24, 2018 at 10:17:31AM +0100, Michal Hocko wrote:
> >>>> On Mon 24-12-18 01:05:57, David Rientjes wrote:
> >>>> [...]
> >>>>> I'm not interested in having a 100 email thread about this when a clear 
> >>>>> and simple fix exists that actually doesn't break user code.
> >>>>
> >>>> You are breaking everybody who really wants to query MADV_NOHUGEPAGE
> >>>> status by this flag. Is there anybody doing that?
> >>>
> >>> Yes.
> >>>
> >>> https://github.com/checkpoint-restore/criu/blob/v3.11/criu/proc_parse.c#L143
> >>
> >> Ugh.  So the regression fix causes a regression?
> > 
> > Yes. The patch from David will hardcode the nohugepage vm flag if the
> > THP was disabled by the prctl at the time of the snapshot. And if the
> > application later enables THP by the prctl then existing mappings would
> > never get the THP enabled status back.
> > 
> > This is the kind of a potential regression I was poiting out earlier
> > when explaining that the patch encodes the logic into the flag exporting
> > and that means that whoever wants to get the raw value of the flag will
> > not be able to do so. Please note that the raw value is exactly what
> > this interface is documented and supposed to export. And as the
> > documentation explains it is implementation specific and anybody to use
> > it should be careful.
> 
> Let's add some CRIU guys in the loop (dunno if the right ones). We're
> discussing David's patch [1] that makes 'nh' and 'hg' flags reported in
> /proc/pid/smaps (and set by madvise) overridable by
> prctl(PR_SET_THP_DISABLE). This was sort of accidental behavior (but
> only for mappings created after the prctl call) before 4.13 commit
> 1860033237d4 ("mm: make PR_SET_THP_DISABLE immediately active").
> 
> For David's userspace that commit is a regression as there are false
> positives when checking for vma's that are eligible for THP (=don't have
> the 'nh' flag in smaps) but didn't really obtain THP's. The userspace
> assumes it's due to fragmentation (=bad) and cannot know that it's due
> to the prctl(). But we fear that making prctl() affect smaps vma flags
> means that CRIU can't query them accurately anymore, and thus it's a
> regression for CRIU. Can you comment on that?
> Michal has a patch [2] that reports the prctl() status separately, but
> that doesn't help David's existing userspace. For CRIU this also won't
> help as long the smaps vma flags still silently included the prctl()
> status. Do you see some solution that would work for everybody?

The patch from David obviously breaks CRIU, and I can't see a nice solution
that will work for everybody.

Of course we could add something like 'NH' to /proc/pid/smaps so that 'nh'
will work as David's userspace is expecting and 'NH' will represent the
state of VmFlags. This is hackish and ugly, though.

In any case, if David's patch is not reverted CRIU needs some way to know
if VMA has VM_NOHUGEPAGE set.

> [1]
> https://www.ozlabs.org/~akpm/mmots/broken-out/mm-thp-always-specify-disabled-vmas-as-nh-in-smaps.patch
> [2]
> https://www.ozlabs.org/~akpm/mmots/broken-out/mm-proc-report-pr_set_thp_disable-in-proc.patch
> 

-- 
Sincerely yours,
Mike.

Re: + mm-thp-always-specify-disabled-vmas-as-nh-in-smaps.patch added to -mm tree

[Michal Hocko]

On Tue 15-01-19 08:32:02, Mike Rapoport wrote:
> Hi,
> 
> The holidays are over and I think it's time to resurrect this thread.
> 
> On Fri, Dec 28, 2018 at 11:54:17AM +0100, Vlastimil Babka wrote:
> > On 12/28/18 9:18 AM, Michal Hocko wrote:
> > > On Thu 27-12-18 21:31:00, Andrew Morton wrote:
> > >> On Thu, 27 Dec 2018 14:11:14 +0300 "Kirill A. Shutemov" <kirill@shutemov.name> wrote:
> > >>
> > >>> On Mon, Dec 24, 2018 at 10:17:31AM +0100, Michal Hocko wrote:
> > >>>> On Mon 24-12-18 01:05:57, David Rientjes wrote:
> > >>>> [...]
> > >>>>> I'm not interested in having a 100 email thread about this when a clear 
> > >>>>> and simple fix exists that actually doesn't break user code.
> > >>>>
> > >>>> You are breaking everybody who really wants to query MADV_NOHUGEPAGE
> > >>>> status by this flag. Is there anybody doing that?
> > >>>
> > >>> Yes.
> > >>>
> > >>> https://github.com/checkpoint-restore/criu/blob/v3.11/criu/proc_parse.c#L143
> > >>
> > >> Ugh.  So the regression fix causes a regression?
> > > 
> > > Yes. The patch from David will hardcode the nohugepage vm flag if the
> > > THP was disabled by the prctl at the time of the snapshot. And if the
> > > application later enables THP by the prctl then existing mappings would
> > > never get the THP enabled status back.
> > > 
> > > This is the kind of a potential regression I was poiting out earlier
> > > when explaining that the patch encodes the logic into the flag exporting
> > > and that means that whoever wants to get the raw value of the flag will
> > > not be able to do so. Please note that the raw value is exactly what
> > > this interface is documented and supposed to export. And as the
> > > documentation explains it is implementation specific and anybody to use
> > > it should be careful.
> > 
> > Let's add some CRIU guys in the loop (dunno if the right ones). We're
> > discussing David's patch [1] that makes 'nh' and 'hg' flags reported in
> > /proc/pid/smaps (and set by madvise) overridable by
> > prctl(PR_SET_THP_DISABLE). This was sort of accidental behavior (but
> > only for mappings created after the prctl call) before 4.13 commit
> > 1860033237d4 ("mm: make PR_SET_THP_DISABLE immediately active").
> > 
> > For David's userspace that commit is a regression as there are false
> > positives when checking for vma's that are eligible for THP (=don't have
> > the 'nh' flag in smaps) but didn't really obtain THP's. The userspace
> > assumes it's due to fragmentation (=bad) and cannot know that it's due
> > to the prctl(). But we fear that making prctl() affect smaps vma flags
> > means that CRIU can't query them accurately anymore, and thus it's a
> > regression for CRIU. Can you comment on that?
> > Michal has a patch [2] that reports the prctl() status separately, but
> > that doesn't help David's existing userspace. For CRIU this also won't
> > help as long the smaps vma flags still silently included the prctl()
> > status. Do you see some solution that would work for everybody?
> 
> The patch from David obviously breaks CRIU, and I can't see a nice solution
> that will work for everybody.
> 
> Of course we could add something like 'NH' to /proc/pid/smaps so that 'nh'
> will work as David's userspace is expecting and 'NH' will represent the
> state of VmFlags. This is hackish and ugly, though.
> 
> In any case, if David's patch is not reverted CRIU needs some way to know
> if VMA has VM_NOHUGEPAGE set.

Hmm, there doesn't seem to be any follow up here and the patch is still
in the mmotm tree AFAICS in mainline-urgent section. I thought it was
clarified that the patch will break an existing userspace that relies on
the documented semantic.

While it is unfortunate that the use case mentioned by David got broken
we have provided a long term sustainable which is much better than
relying on an undocumented side effect of the prctl implementation at
the time.

So can we make a decision on this finally please?

> > [1]
> > https://www.ozlabs.org/~akpm/mmots/broken-out/mm-thp-always-specify-disabled-vmas-as-nh-in-smaps.patch
> > [2]
> > https://www.ozlabs.org/~akpm/mmots/broken-out/mm-proc-report-pr_set_thp_disable-in-proc.patch
> > 
> 
> -- 
> Sincerely yours,
> Mike.

-- 
Michal Hocko
SUSE Labs

Re: + mm-thp-always-specify-disabled-vmas-as-nh-in-smaps.patch added to -mm tree

[Cyrill Gorcunov]

On Mon, Jan 21, 2019 at 11:21:44AM +0100, Michal Hocko wrote:
...
> > 
> > The patch from David obviously breaks CRIU, and I can't see a nice solution
> > that will work for everybody.
> > 
> > Of course we could add something like 'NH' to /proc/pid/smaps so that 'nh'
> > will work as David's userspace is expecting and 'NH' will represent the
> > state of VmFlags. This is hackish and ugly, though.
> > 
> > In any case, if David's patch is not reverted CRIU needs some way to know
> > if VMA has VM_NOHUGEPAGE set.
> 
> Hmm, there doesn't seem to be any follow up here and the patch is still
> in the mmotm tree AFAICS in mainline-urgent section. I thought it was
> clarified that the patch will break an existing userspace that relies on
> the documented semantic.
> 
> While it is unfortunate that the use case mentioned by David got broken
> we have provided a long term sustainable which is much better than
> relying on an undocumented side effect of the prctl implementation at
> the time.
> 
> So can we make a decision on this finally please?

As to me David's userspace application could use /proc/$pid/status
to fetch precise THP state. And the patch in mm queue simply breaks
others userspace thus should be reverted.

Re: + mm-thp-always-specify-disabled-vmas-as-nh-in-smaps.patch added to -mm tree

[Michal Hocko]

On Mon 21-01-19 21:00:29, Cyrill Gorcunov wrote:
> On Mon, Jan 21, 2019 at 11:21:44AM +0100, Michal Hocko wrote:
> ...
> > > 
> > > The patch from David obviously breaks CRIU, and I can't see a nice solution
> > > that will work for everybody.
> > > 
> > > Of course we could add something like 'NH' to /proc/pid/smaps so that 'nh'
> > > will work as David's userspace is expecting and 'NH' will represent the
> > > state of VmFlags. This is hackish and ugly, though.
> > > 
> > > In any case, if David's patch is not reverted CRIU needs some way to know
> > > if VMA has VM_NOHUGEPAGE set.
> > 
> > Hmm, there doesn't seem to be any follow up here and the patch is still
> > in the mmotm tree AFAICS in mainline-urgent section. I thought it was
> > clarified that the patch will break an existing userspace that relies on
> > the documented semantic.
> > 
> > While it is unfortunate that the use case mentioned by David got broken
> > we have provided a long term sustainable which is much better than
> > relying on an undocumented side effect of the prctl implementation at
> > the time.
> > 
> > So can we make a decision on this finally please?
> 
> As to me David's userspace application could use /proc/$pid/status
> to fetch precise THP state. And the patch in mm queue simply breaks
> others userspace thus should be reverted.

7635d9cbe832 ("mm, thp, proc: report THP eligibility for each vma") will
provide even more detailed information because it displays THP
eligibility per VMA so you do not have to check all other conditions
that control THP.

-- 
Michal Hocko
SUSE Labs

Re: + mm-thp-always-specify-disabled-vmas-as-nh-in-smaps.patch added to -mm tree

[Cyrill Gorcunov]

On Mon, Jan 21, 2019 at 07:18:24PM +0100, Michal Hocko wrote:
> > > 
> > > So can we make a decision on this finally please?
> > 
> > As to me David's userspace application could use /proc/$pid/status
> > to fetch precise THP state. And the patch in mm queue simply breaks
> > others userspace thus should be reverted.
> 
> 7635d9cbe832 ("mm, thp, proc: report THP eligibility for each vma") will
> provide even more detailed information because it displays THP
> eligibility per VMA so you do not have to check all other conditions
> that control THP.

Thus the only thing we need is to wait for David's reply if he can
update his application to use the THPeligible flag and drop the
patch from mm queue