Re: [PATCH v6 09/10] device-dax: set mapping prior to vmf_insert_pfn{,_pmd,pud}()

[Joao Martins <joao.m.martins@oracle.com>]

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