On Tue, 16 Oct 2018, Michal Hocko wrote:
> > I don't understand the point of extending smaps with yet another line.
>
> Because abusing a vma flag part is just wrong. What are you going to do
> when a next bug report states that the flag is set even though no
> userspace has set it and that leads to some malfunctioning? Can you rule
> that out? Even your abuse of the flag is surprising so why others
> wouldn't be?
>
The flag has taken on the meaning of "thp disabled for this vma", how it
is set is not the scope of the flag. If a thp is explicitly disabled from
being eligible for thp, whether by madvise, prctl, or any future
mechanism, it should use VM_NOHUGEPAGE or show_smap_vma_flags() needs to
be modified.
I agree with you that this could have been done better if an interface was
defined earlier that userspace could have used. PR_SET_THP_DISABLE was
merged long after thp had already been merged so this can be a reminder
that defining clean, robust, and extensible APIs is important, but I'm
afraid we can't go back in time and change how userspace queries
information, especially in cases where there was only one way to do it.
> > The only way for a different process to determine if a single vma from
> > another process is thp disabled is by the "nh" flag, so it is reasonable
> > that userspace reads this. My patch fixes that. If smaps is extended
> > with another line per your patch, it doesn't change the fact that previous
> > binaries are built to check for "nh" so it does not deprecate that.
> > ("THP_Enabled" is also ambiguous since it only refers to prctl and not the
> > default thp setting or madvise.)
>
> As I've said there are two things. Exporting PR_SET_THP_DISABLE to
> userspace so that a 3rd party process can query it. I've already
> explained why that might be useful. If you really insist on having
> a per-vma field then let's do it properly now. Are you going to agree on
> that? If yes, I am willing to spend my time on that but I am not going
> to bother if this will lead to "I want my vma field abuse anyway".
I think what you and I want is largely irrelevant :) What's important is
that there are userspace implementations that query this today so
continuing to support it as the way to determine if a vma has been thp
disabled doesn't seem problematic and guarantees that userspace doesn't
break.
