Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190722094426.18563-1-hch@lst.de> <20190722094426.18563-2-hch@lst.de>
In-Reply-To: <20190722094426.18563-2-hch@lst.de>
From:   Souptick Joarder <jrdr.linux@gmail.com>
Date:   Mon, 22 Jul 2019 20:07:33 +0530
Message-ID: <CAFqt6zY8zWAmc-VTrZ1KxQPBCdbTxmZy_tq2-OkUi3TVrfp7Og@mail.gmail.com>
Subject: Re: [PATCH 1/6] mm: always return EBUSY for invalid ranges in hmm_range_{fault,snapshot}
To:     Christoph Hellwig <hch@lst.de>
Cc:     =?UTF-8?B?SsOpcsO0bWUgR2xpc3Nl?= <jglisse@redhat.com>,
        Jason Gunthorpe <jgg@mellanox.com>,
        Ben Skeggs <bskeggs@redhat.com>,
        Ralph Campbell <rcampbell@nvidia.com>,
        Linux-MM <linux-mm@kvack.org>, nouveau@lists.freedesktop.org,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
        Felix Kuehling <Felix.Kuehling@amd.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Jul 22, 2019 at 3:14 PM Christoph Hellwig <hch@lst.de> wrote:
>
> We should not have two different error codes for the same condition.  In
> addition this really complicates the code due to the special handling of
> EAGAIN that drops the mmap_sem due to the FAULT_FLAG_ALLOW_RETRY logic
> in the core vm.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Ralph Campbell <rcampbell@nvidia.com>
> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>  Documentation/vm/hmm.rst |  2 +-
>  mm/hmm.c                 | 10 ++++------
>  2 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/vm/hmm.rst b/Documentation/vm/hmm.rst
> index 7d90964abbb0..710ce1c701bf 100644
> --- a/Documentation/vm/hmm.rst
> +++ b/Documentation/vm/hmm.rst
> @@ -237,7 +237,7 @@ The usage pattern is::
>        ret = hmm_range_snapshot(&range);
>        if (ret) {
>            up_read(&mm->mmap_sem);
> -          if (ret == -EAGAIN) {
> +          if (ret == -EBUSY) {
>              /*
>               * No need to check hmm_range_wait_until_valid() return value
>               * on retry we will get proper error with hmm_range_snapshot()
> diff --git a/mm/hmm.c b/mm/hmm.c
> index e1eedef129cf..16b6731a34db 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -946,7 +946,7 @@ EXPORT_SYMBOL(hmm_range_unregister);
>   * @range: range
>   * Return: -EINVAL if invalid argument, -ENOMEM out of memory, -EPERM invalid
>   *          permission (for instance asking for write and range is read only),
> - *          -EAGAIN if you need to retry, -EFAULT invalid (ie either no valid
> + *          -EBUSY if you need to retry, -EFAULT invalid (ie either no valid
>   *          vma or it is illegal to access that range), number of valid pages
>   *          in range->pfns[] (from range start address).
>   *
> @@ -967,7 +967,7 @@ long hmm_range_snapshot(struct hmm_range *range)
>         do {
>                 /* If range is no longer valid force retry. */
>                 if (!range->valid)
> -                       return -EAGAIN;
> +                       return -EBUSY;
>
>                 vma = find_vma(hmm->mm, start);
>                 if (vma == NULL || (vma->vm_flags & device_vma))
> @@ -1062,10 +1062,8 @@ long hmm_range_fault(struct hmm_range *range, bool block)
>
>         do {
>                 /* If range is no longer valid force retry. */
> -               if (!range->valid) {
> -                       up_read(&hmm->mm->mmap_sem);
> -                       return -EAGAIN;
> -               }
> +               if (!range->valid)
> +                       return -EBUSY;

Is it fine to remove  up_read(&hmm->mm->mmap_sem) ?

>
>                 vma = find_vma(hmm->mm, start);
>                 if (vma == NULL || (vma->vm_flags & device_vma))
> --
> 2.20.1
>