Messages in this thread | | | Date | Thu, 27 Jul 2023 22:43:27 +0200 | Subject | Re: [PATCH] smaps: Fix the abnormal memory statistics obtained through /proc/pid/smaps | From | David Hildenbrand <> |
| |
On 27.07.23 22:30, Peter Xu wrote: > On Thu, Jul 27, 2023 at 09:17:45PM +0200, David Hildenbrand wrote: >> On 27.07.23 20:59, Peter Xu wrote: >>> On Thu, Jul 27, 2023 at 07:27:02PM +0200, David Hildenbrand wrote: >>>>>> >>>>>> This was wrong from the very start. If we're not in GUP, we shouldn't call >>>>>> GUP functions. >>>>> >>>>> My understanding is !GET && !PIN is also called gup.. otherwise we don't >>>>> need GET and it can just be always implied. >>>> >>>> That's not the point. The point is that _arbitrary_ code shouldn't call into >>>> GUP internal helper functions, where they bypass, for example, any sanity >>>> checks. >>> >>> What's the sanity checks that you're referring to? >>> >> >> For example in follow_page() >> >> if (vma_is_secretmem(vma)) >> return NULL; >> >> if (WARN_ON_ONCE(foll_flags & FOLL_PIN)) >> return NULL; >> >> >> Maybe you can elaborate why you think we should *not* be using >> vm_normal_page_pmd() and instead some arbitrary GUP internal helper? I don't >> get it. > > Because the old code was written like that?
And it's not 2014 anymore. Nowadays we do have the right helper in place.
[...]
>> FOLL_NUMA naming was nowadays wrong to begin with (not to mention, confusing >> a we learned). There are other reasons why we have PROT_NONE -- mprotect(), >> for example. > > It doesn't really violate with the name, IMHO - protnone can be either numa > hint or PROT_NONE for real. As long as we return NULL for a FOLL_NUMA > request we're achieving the goal we want - we guarantee a NUMA balancing to > trigger with when FOLL_NUMA provided. It doesn't need to guarantee > anything else, afaiu. The final check relies in vma_is_accessible() in the > fault paths anyway. So I don't blame the old name that much.
IMHO, the name FOLL_NUMA made sense when it still was called pte_numa.
> >> >> We could have a flag that goes the other way around: FOLL_IGNORE_PROTNONE >> ... which surprisingly then ends up being exactly what FOLL_FORCE means >> without FOLL_WRITE, and what this patch does. >> >> Does that make sense to you? >> >> >>> >>> The very least is if with above we should really document FOLL_FORCE - we >>> should mention NUMA effects. But that's ... really confusing. Thinking >>> about that I personally prefer a revival of FOLL_NUMA, then smaps issue all >>> go away. >> >> smaps needs to be changed in any case IMHO. And I'm absolutely not in favor >> of revicing FOLL_NUMA. > > As stated above, to me FOLL_NUMA is all fine and clear. If you think > having a flag back for protnone is worthwhile no matter as-is (FOLL_NUMA) > or with reverted meaning, then that sounds all fine to me. Maybe the old > name at least makes old developers know what's that. > > I don't have a strong opinion on names though; mostly never had.
I'll avoid new FOLL_ flags first and post my proposal. If many people are unhappy with that approach, we can revert the commit and call it a day.
Thanks!
-- Cheers,
David / dhildenb
| |