On 11/29/21 07:32, Christoph Hellwig wrote:
> On Fri, Nov 26, 2021 at 06:39:39PM +0000, Joao Martins wrote:
>> @@ -230,23 +235,18 @@ static vm_fault_t dev_dax_huge_fault(struct vm_fault *vmf,
>> id = dax_read_lock();
>> switch (pe_size) {
>> case PE_SIZE_PTE:
>> - fault_size = PAGE_SIZE;
>> rc = __dev_dax_pte_fault(dev_dax, vmf, &pfn);
>> break;
>> case PE_SIZE_PMD:
>> - fault_size = PMD_SIZE;
>> rc = __dev_dax_pmd_fault(dev_dax, vmf, &pfn);
>> break;
>> case PE_SIZE_PUD:
>> - fault_size = PUD_SIZE;
>> rc = __dev_dax_pud_fault(dev_dax, vmf, &pfn);
>> break;
>> default:
>> rc = VM_FAULT_SIGBUS;
>> }
>>
>> dax_read_unlock(id);
>
> I wonder if if would make sense to move dax_read_lock / dax_read_unlock
> іnto the individul helpers as well now. That way you could directly
> return from the switch.
Hmmm -- if by individual helpers moving to __dev_dax_{pte,pmd,pud}_fault()
it would be slightly less straighforward. Unless you might mean to move
to check_vma() (around the dax_alive() check) and that might actually
remove the opencoding of dax_read_lock in dax_mmap() even.
I would rather prefer that this cleanup around dax_read_{un,}lock is
a separate patch separate to this series, unless you feel strongly that
it needs to be part of this set.
> Aso it seems like pfn is only an input
> parameter now and doesn't need to be passed by reference.
>
It's actually just an output parameter (that dax_set_mapping would then use).
The fault handlers in device-dax use vmf->address to calculate pfn that they
insert in the page table entry. After this patch we can actually just remove
@pfn argument.
Joao
