Messages in this thread | | | Date | Sat, 15 Jul 2023 09:54:38 +0800 | Subject | Re: [PATCH rfc -next 01/10] mm: add a generic VMA lock-based page fault handler | From | Kefeng Wang <> |
| |
On 2023/7/14 9:52, Kefeng Wang wrote: > > > On 2023/7/14 4:12, Suren Baghdasaryan wrote: >> On Thu, Jul 13, 2023 at 9:15 AM Matthew Wilcox <willy@infradead.org> >> wrote: >>> >>>> +int try_vma_locked_page_fault(struct vm_locked_fault *vmlf, >>>> vm_fault_t *ret) >>>> +{ >>>> + struct vm_area_struct *vma; >>>> + vm_fault_t fault; >>> >>> >>> On Thu, Jul 13, 2023 at 05:53:29PM +0800, Kefeng Wang wrote: >>>> +#define VM_LOCKED_FAULT_INIT(_name, _mm, _address, _fault_flags, >>>> _vm_flags, _regs, _fault_code) \ >>>> + _name.mm = _mm; \ >>>> + _name.address = _address; \ >>>> + _name.fault_flags = _fault_flags; \ >>>> + _name.vm_flags = _vm_flags; \ >>>> + _name.regs = _regs; \ >>>> + _name.fault_code = _fault_code >>> >>> More consolidated code is a good idea; no question. But I don't think >>> this is the right way to do it. > > I agree it is not good enough, but the arch's vma check acess has > different implementation, some use vm flags, some need fault code and > regs, and some use both :( > >>> >>>> +int __weak arch_vma_check_access(struct vm_area_struct *vma, >>>> + struct vm_locked_fault *vmlf); >>> >>> This should be: >>> >>> #ifndef vma_check_access >>> bool vma_check_access(struct vm_area_struct *vma, ) >>> { >>> return (vma->vm_flags & vm_flags) == 0; >>> } >>> #endif >>> >>> and then arches which want to do something different can just define >>> vma_check_access. > > Ok, I could convert to use this way. > >>> >>>> +int try_vma_locked_page_fault(struct vm_locked_fault *vmlf, >>>> vm_fault_t *ret) >>>> +{ >>>> + struct vm_area_struct *vma; >>>> + vm_fault_t fault; >>> >>> Declaring the vmf in this function and then copying it back is just >>> wrong. >>> We need to declare vm_fault_t earlier (in the arch fault handler) and >>> pass it in. > > Actually I passed the vm_fault_t *ret(in the arch fault handler), we > could directly use *ret instead of a new local variable, and no copy. >> >> Did you mean to say "we need to declare vmf (struct vm_fault) earlier >> (in the arch fault handler) and pass it in." ?
After recheck the code, I think Matthew' idea is 'declare vmf (struct vm_fault) earlier' like Suren said, not vm_fault_t, right? will try this, thanks.
>> >>> I don't think that creating struct vm_locked_fault is the >>> right idea either. > > As mentioned above for vma check access, we need many arguments for a > function, a new struct looks possible better, is there better solution > or any suggestion? > > Thanks. >
| |