Re: [PATCH 11/13] huge_memory: Remove dead vmf_insert_pXd code

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Peter Xu <peterx@xxxxxxxxxx> writes:

> On Tue, Jul 09, 2024 at 02:07:31PM +1000, Alistair Popple wrote:
>> 
>> Peter Xu <peterx@xxxxxxxxxx> writes:
>> 
>> > Hi, Alistair,
>> >
>> > On Thu, Jun 27, 2024 at 10:54:26AM +1000, Alistair Popple wrote:
>> >> Now that DAX is managing page reference counts the same as normal
>> >> pages there are no callers for vmf_insert_pXd functions so remove
>> >> them.
>> >> 
>> >> Signed-off-by: Alistair Popple <apopple@xxxxxxxxxx>
>> >> ---
>> >>  include/linux/huge_mm.h |   2 +-
>> >>  mm/huge_memory.c        | 165 +-----------------------------------------
>> >>  2 files changed, 167 deletions(-)
>> >> 
>> >> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
>> >> index 9207d8e..0fb6bff 100644
>> >> --- a/include/linux/huge_mm.h
>> >> +++ b/include/linux/huge_mm.h
>> >> @@ -37,8 +37,6 @@ int change_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
>> >>  		    pmd_t *pmd, unsigned long addr, pgprot_t newprot,
>> >>  		    unsigned long cp_flags);
>> >>  
>> >> -vm_fault_t vmf_insert_pfn_pmd(struct vm_fault *vmf, pfn_t pfn, bool write);
>> >> -vm_fault_t vmf_insert_pfn_pud(struct vm_fault *vmf, pfn_t pfn, bool write);
>> >>  vm_fault_t dax_insert_pfn_pmd(struct vm_fault *vmf, pfn_t pfn, bool write);
>> >>  vm_fault_t dax_insert_pfn_pud(struct vm_fault *vmf, pfn_t pfn, bool write);
>> >
>> > There's a plan to support huge pfnmaps in VFIO, which may still make good
>> > use of these functions.  I think it's fine to remove them but it may mean
>> > we'll need to add them back when supporting pfnmaps with no memmap.
>> 
>> I'm ok with that. If we need them back in future it shouldn't be too
>> hard to add them back again. I just couldn't find any callers of them
>> once DAX stopped using them and the usual policy is to remove unused
>> functions.
>
> True.  Currently the pmd/pud helpers are only used in dax.
>
>> 
>> > Is it still possible to make the old API generic to both service the new
>> > dax refcount plan, but at the meantime working for pfn injections when
>> > there's no page struct?
>> 
>> I don't think so - this new dax refcount plan relies on having a struct
>> page to take references on so I don't think it makes much sense to
>> combine it with something that doesn't have a struct page. It sounds
>> like the situation is the analogue of vm_insert_page()
>> vs. vmf_insert_pfn() - it's possible for both to exist but there's not
>> really anything that can be shared between the two APIs as one has a
>> page and the other is just a raw PFN.
>
> I still think most of the codes should be shared on e.g. most of sanity
> checks, pgtable injections, pgtable deposits (for pmd) and so on.

Yeah, it was mostly the BUG_ON's that weren't applicable once pXd_devmap
went away.

> To be explicit, I wonder whether something like below diff would be
> applicable on top of the patch "huge_memory: Allow mappings of PMD sized
> pages" in this series, which introduced dax_insert_pfn_pmd() for dax:
>
> $ diff origin new
> 1c1
> < vm_fault_t dax_insert_pfn_pmd(struct vm_fault *vmf, pfn_t pfn, bool write)
> ---
>> vm_fault_t vmf_insert_pfn_pmd(struct vm_fault *vmf, pfn_t pfn, bool write)
> 55,58c55,60
> <       folio = page_folio(page);
> <       folio_get(folio);
> <       folio_add_file_rmap_pmd(folio, page, vma);
> <       add_mm_counter(mm, mm_counter_file(folio), HPAGE_PMD_NR);
> ---
>>         if (page) {
>>                 folio = page_folio(page);
>>                 folio_get(folio);
>>                 folio_add_file_rmap_pmd(folio, page, vma);
>>                 add_mm_counter(mm, mm_counter_file(folio), HPAGE_PMD_NR);
>>         }

We get the page from calling pfn_t_to_page(pfn). This is safe for the
DAX case but is it safe to use a page returned by this more generally?


[Index of Archives]     [Reiser Filesystem Development]     [Ceph FS]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite National Park]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Device Mapper]     [Linux Media]

  Powered by Linux