Use new return type vm_fault_t for fault handler. For
now, this is just documenting that the function returns
a VM_FAULT value rather than an errno. Once all instances
are converted, vm_fault_t will become a distinct type.
Ref-> commit 1c8f422059ae ("mm: change return type to vm_fault_t")
Previously vm_insert_pfn() returns err which has to
mapped into VM_FAULT_* type. The new function
vmf_insert_pfn() will replace this inefficiency by
returning VM_FAULT_* type.
Signed-off-by: Souptick Joarder <[email protected]>
---
arch/x86/entry/vdso/vma.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/arch/x86/entry/vdso/vma.c b/arch/x86/entry/vdso/vma.c
index 5b8b556..3679885 100644
--- a/arch/x86/entry/vdso/vma.c
+++ b/arch/x86/entry/vdso/vma.c
@@ -39,7 +39,7 @@ void __init init_vdso_image(const struct vdso_image *image)
struct linux_binprm;
-static int vdso_fault(const struct vm_special_mapping *sm,
+static vm_fault_t vdso_fault(const struct vm_special_mapping *sm,
struct vm_area_struct *vma, struct vm_fault *vmf)
{
const struct vdso_image *image = vma->vm_mm->context.vdso_image;
@@ -84,15 +84,15 @@ static int vdso_mremap(const struct vm_special_mapping *sm,
return 0;
}
-static int vvar_fault(const struct vm_special_mapping *sm,
+static vm_fault_t vvar_fault(const struct vm_special_mapping *sm,
struct vm_area_struct *vma, struct vm_fault *vmf)
{
const struct vdso_image *image = vma->vm_mm->context.vdso_image;
long sym_offset;
- int ret = -EFAULT;
+ vm_fault_t ret = VM_FAULT_SIGBUS;
if (!image)
- return VM_FAULT_SIGBUS;
+ return ret;
sym_offset = (long)(vmf->pgoff << PAGE_SHIFT) +
image->sym_vvar_start;
@@ -105,10 +105,10 @@ static int vvar_fault(const struct vm_special_mapping *sm,
* the page past the end of the vvar mapping.
*/
if (sym_offset == 0)
- return VM_FAULT_SIGBUS;
+ return ret;
if (sym_offset == image->sym_vvar_page) {
- ret = vm_insert_pfn(vma, vmf->address,
+ ret = vmf_insert_pfn(vma, vmf->address,
__pa_symbol(&__vvar_page) >> PAGE_SHIFT);
} else if (sym_offset == image->sym_pvclock_page) {
struct pvclock_vsyscall_time_info *pvti =
@@ -124,14 +124,11 @@ static int vvar_fault(const struct vm_special_mapping *sm,
struct ms_hyperv_tsc_page *tsc_pg = hv_get_tsc_page();
if (tsc_pg && vclock_was_used(VCLOCK_HVCLOCK))
- ret = vm_insert_pfn(vma, vmf->address,
+ ret = vmf_insert_pfn(vma, vmf->address,
vmalloc_to_pfn(tsc_pg));
}
- if (ret == 0 || ret == -EBUSY)
- return VM_FAULT_NOPAGE;
-
- return VM_FAULT_SIGBUS;
+ return ret;
}
static const struct vm_special_mapping vdso_mapping = {
--
1.9.1
On Mon, Jun 25, 2018 at 8:55 AM Souptick Joarder <[email protected]> wrote:
>
> Use new return type vm_fault_t for fault handler. For
> now, this is just documenting that the function returns
> a VM_FAULT value rather than an errno. Once all instances
> are converted, vm_fault_t will become a distinct type.
Whoa there.. Your commit message makes it sound like you're just
changing the return type, but:
> if (tsc_pg && vclock_was_used(VCLOCK_HVCLOCK))
> - ret = vm_insert_pfn(vma, vmf->address,
> + ret = vmf_insert_pfn(vma, vmf->address,
> vmalloc_to_pfn(tsc_pg));
> }
>
> - if (ret == 0 || ret == -EBUSY)
> - return VM_FAULT_NOPAGE;
> -
> - return VM_FAULT_SIGBUS;
> + return ret;
You're refactoring the code, too.
Please fix your changelog.
On Mon, Jun 25, 2018 at 9:41 PM, Andy Lutomirski <[email protected]> wrote:
> On Mon, Jun 25, 2018 at 8:55 AM Souptick Joarder <[email protected]> wrote:
>>
>> Use new return type vm_fault_t for fault handler. For
>> now, this is just documenting that the function returns
>> a VM_FAULT value rather than an errno. Once all instances
>> are converted, vm_fault_t will become a distinct type.
>
> Whoa there.. Your commit message makes it sound like you're just
> changing the return type, but:
>
>> if (tsc_pg && vclock_was_used(VCLOCK_HVCLOCK))
>> - ret = vm_insert_pfn(vma, vmf->address,
>> + ret = vmf_insert_pfn(vma, vmf->address,
>> vmalloc_to_pfn(tsc_pg));
>> }
>>
>> - if (ret == 0 || ret == -EBUSY)
>> - return VM_FAULT_NOPAGE;
>> -
>> - return VM_FAULT_SIGBUS;
>> + return ret;
>
> You're refactoring the code, too.
>
> Please fix your changelog.
I have mentioned it.
********************
Ref-> commit 1c8f422059ae ("mm: change return type to vm_fault_t")
Previously vm_insert_pfn() returns err which has to
mapped into VM_FAULT_* type. The new function
vmf_insert_pfn() will replace this inefficiency by
returning VM_FAULT_* type.
*********************
On Mon, Jun 25, 2018 at 9:15 AM Souptick Joarder <[email protected]> wrote:
>
> On Mon, Jun 25, 2018 at 9:41 PM, Andy Lutomirski <[email protected]> wrote:
> > On Mon, Jun 25, 2018 at 8:55 AM Souptick Joarder <[email protected]> wrote:
> >>
> >> Use new return type vm_fault_t for fault handler. For
> >> now, this is just documenting that the function returns
> >> a VM_FAULT value rather than an errno. Once all instances
> >> are converted, vm_fault_t will become a distinct type.
> >
> > Whoa there.. Your commit message makes it sound like you're just
> > changing the return type, but:
> >
> >> if (tsc_pg && vclock_was_used(VCLOCK_HVCLOCK))
> >> - ret = vm_insert_pfn(vma, vmf->address,
> >> + ret = vmf_insert_pfn(vma, vmf->address,
> >> vmalloc_to_pfn(tsc_pg));
> >> }
> >>
> >> - if (ret == 0 || ret == -EBUSY)
> >> - return VM_FAULT_NOPAGE;
> >> -
> >> - return VM_FAULT_SIGBUS;
> >> + return ret;
> >
> > You're refactoring the code, too.
> >
> > Please fix your changelog.
>
>
> I have mentioned it.
>
> ********************
> Ref-> commit 1c8f422059ae ("mm: change return type to vm_fault_t")
>
> Previously vm_insert_pfn() returns err which has to
> mapped into VM_FAULT_* type. The new function
> vmf_insert_pfn() will replace this inefficiency by
> returning VM_FAULT_* type.
> *********************
The second line of your changelog says "For now, this is just
documenting...". Please don't make future readers read all the way
through the whole changelog and try to reconcile conflicting sentences
to figure out what the patch does.
I read your changelog, then I read through the diff to make sure that
you were really just changing the return type (as your changelog
seemed to claim), and I decided that your patch was incorrect. As the
maintainer of this code, this means that you've made my work harder
than it should be, so NAK. Please improve your changelog and
resubmit.
--Andy