Re: [PATCH rfc v2 04/10] s390: mm: use try_vma_locked_page_fault()

[Kefeng Wang]

On 2023/8/24 16:32, Heiko Carstens wrote:
> On Thu, Aug 24, 2023 at 10:16:33AM +0200, Alexander Gordeev wrote:
> > On Mon, Aug 21, 2023 at 08:30:50PM +0800, Kefeng Wang wrote:
> > > Use new try_vma_locked_page_fault() helper to simplify code.
> > > No functional change intended.
> > >
> > > Signed-off-by: Kefeng Wang <wangkefeng.wang@xxxxxxxxxx>
> > > ---
> > >   arch/s390/mm/fault.c | 66 ++++++++++++++++++--------------------------
> > >   1 file changed, 27 insertions(+), 39 deletions(-)
> ...
> > > -	fault = handle_mm_fault(vma, address, flags | FAULT_FLAG_VMA_LOCK, regs);
> > > -	if (!(fault & (VM_FAULT_RETRY | VM_FAULT_COMPLETED)))
> > > -		vma_end_read(vma);
> > > -	if (!(fault & VM_FAULT_RETRY)) {
> > > -		count_vm_vma_lock_event(VMA_LOCK_SUCCESS);
> > > -		if (likely(!(fault & VM_FAULT_ERROR)))
> > > -			fault = 0;
> >
> > This fault fixup is removed in the new version.
> ...
>
> > > +		vmf.vm_flags = VM_WRITE;
> > > +	if (vmf.vm_flags == VM_WRITE)
> > > +		vmf.flags |= FAULT_FLAG_WRITE;
> > > +
> > > +	fault = try_vma_locked_page_fault(&vmf);
> > > +	if (fault == VM_FAULT_NONE)
> > > +		goto lock_mm;
> >
> > Because VM_FAULT_NONE is set to 0 it gets confused with
> > the success code of 0 returned by a fault handler. In the
> > former case we want to continue, while in the latter -
> > successfully return. I think it applies to all archs.
> ...
> > FWIW, this series ends up with kernel BUG at arch/s390/mm/fault.c:341!

I didn't test and only built, this is a RFC to want to know whether
the way to add three more numbers into vmf and using vmf in arch's page
fault is feasible or not.

> Without having looked in detail into this patch: all of this is likely
> because s390's fault handling is quite odd. Not only because fault is set
> to 0, but also because of the private VM_FAULT values like
> VM_FAULT_BADCONTEXT. I'm just cleaning up all of this, but it won't make it
> for the next merge window.

Sure, if re-post, will drop the s390's change, but as mentioned above, 
the abstract of the generic vma locked and changes may be not perfect,
let's wait for more response.

Thanks all.

> Therefore I'd like to ask to drop the s390 conversion of this series, and
> if this series is supposed to be merged the s390 conversion needs to be
> done later. Let's not waste more time on the current implementation,
> please.