dri-devel Archive on lore.kernel.org
 help / color / Atom feed
From: Oded Gabbay <oded.gabbay@gmail.com>
To: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: linux-samsung-soc <linux-samsung-soc@vger.kernel.org>,
	"Jan Kara" <jack@suse.cz>, "KVM list" <kvm@vger.kernel.org>,
	"Jason Gunthorpe" <jgg@ziepe.ca>,
	"Pawel Piskorski" <ppiskorski@habana.ai>,
	"John Hubbard" <jhubbard@nvidia.com>,
	LKML <linux-kernel@vger.kernel.org>,
	"DRI Development" <dri-devel@lists.freedesktop.org>,
	"Ofir Bitton" <obitton@habana.ai>,
	"Christoph Hellwig" <hch@infradead.org>,
	linux-mm <linux-mm@kvack.org>,
	"Jérôme Glisse" <jglisse@redhat.com>,
	"Tomer Tayar" <ttayar@habana.ai>,
	"Omer Shpigelman" <oshpigelman@habana.ai>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Daniel Vetter" <daniel.vetter@intel.com>,
	"Andrew Morton" <akpm@linux-foundation.org>,
	"Moti Haimovski" <mhaimovski@habana.ai>,
	"Dan Williams" <dan.j.williams@intel.com>,
	"list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>,
	Joerg Roedel <joro@8bytes.org>,
	" <linux-arm-kernel@lists.infradead.org>,
	"Linux Media Mailing List" <linux-media@vger.kernel.org>
Subject: Re: [PATCH v6 03/17] misc/habana: Stop using frame_vector helpers
Date: Sat, 21 Nov 2020 14:47:05 +0200
Message-ID: <CAFCwf12Hrjm1La_qgrpW_E0WryiBXYzOobMMGEYsxh9kg=3_AA@mail.gmail.com> (raw)
In-Reply-To: <20201119144146.1045202-4-daniel.vetter@ffwll.ch>

On Thu, Nov 19, 2020 at 4:41 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> All we need are a pages array, pin_user_pages_fast can give us that
> directly. Plus this avoids the entire raw pfn side of get_vaddr_frames.
>
> Note that pin_user_pages_fast is a safe replacement despite the
> seeming lack of checking for vma->vm_flasg & (VM_IO | VM_PFNMAP). Such
> ptes are marked with pte_mkspecial (which pup_fast rejects in the
> fastpath), and only architectures supporting that support the
> pin_user_pages_fast fastpath.
>
> Reviewed-by: John Hubbard <jhubbard@nvidia.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: John Hubbard <jhubbard@nvidia.com>
> Cc: Jérôme Glisse <jglisse@redhat.com>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: linux-mm@kvack.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-samsung-soc@vger.kernel.org
> Cc: linux-media@vger.kernel.org
> Cc: Oded Gabbay <oded.gabbay@gmail.com>
> Cc: Omer Shpigelman <oshpigelman@habana.ai>
> Cc: Ofir Bitton <obitton@habana.ai>
> Cc: Tomer Tayar <ttayar@habana.ai>
> Cc: Moti Haimovski <mhaimovski@habana.ai>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Pawel Piskorski <ppiskorski@habana.ai>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> --
> v2: Use unpin_user_pages_dirty_lock (John)
> v3: Update kerneldoc (Oded)
> v6: Explain why pup_fast is safe, after discussions with John and
> Christoph.
> ---
>  drivers/misc/habanalabs/Kconfig             |  1 -
>  drivers/misc/habanalabs/common/habanalabs.h |  6 ++-
>  drivers/misc/habanalabs/common/memory.c     | 49 ++++++++-------------
>  3 files changed, 22 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/misc/habanalabs/Kconfig b/drivers/misc/habanalabs/Kconfig
> index 1640340d3e62..293d79811372 100644
> --- a/drivers/misc/habanalabs/Kconfig
> +++ b/drivers/misc/habanalabs/Kconfig
> @@ -6,7 +6,6 @@
>  config HABANA_AI
>         tristate "HabanaAI accelerators (habanalabs)"
>         depends on PCI && HAS_IOMEM
> -       select FRAME_VECTOR
>         select GENERIC_ALLOCATOR
>         select HWMON
>         help
> diff --git a/drivers/misc/habanalabs/common/habanalabs.h b/drivers/misc/habanalabs/common/habanalabs.h
> index 80d4d7385ffe..272aa3f29200 100644
> --- a/drivers/misc/habanalabs/common/habanalabs.h
> +++ b/drivers/misc/habanalabs/common/habanalabs.h
> @@ -921,7 +921,8 @@ struct hl_ctx_mgr {
>   * struct hl_userptr - memory mapping chunk information
>   * @vm_type: type of the VM.
>   * @job_node: linked-list node for hanging the object on the Job's list.
> - * @vec: pointer to the frame vector.
> + * @pages: pointer to struct page array
> + * @npages: size of @pages array
>   * @sgt: pointer to the scatter-gather table that holds the pages.
>   * @dir: for DMA unmapping, the direction must be supplied, so save it.
>   * @debugfs_list: node in debugfs list of command submissions.
> @@ -932,7 +933,8 @@ struct hl_ctx_mgr {
>  struct hl_userptr {
>         enum vm_type_t          vm_type; /* must be first */
>         struct list_head        job_node;
> -       struct frame_vector     *vec;
> +       struct page             **pages;
> +       unsigned int            npages;
>         struct sg_table         *sgt;
>         enum dma_data_direction dir;
>         struct list_head        debugfs_list;
> diff --git a/drivers/misc/habanalabs/common/memory.c b/drivers/misc/habanalabs/common/memory.c
> index 84227819e4d1..0b220221873d 100644
> --- a/drivers/misc/habanalabs/common/memory.c
> +++ b/drivers/misc/habanalabs/common/memory.c
> @@ -1291,45 +1291,41 @@ static int get_user_memory(struct hl_device *hdev, u64 addr, u64 size,
>                 return -EFAULT;
>         }
>
> -       userptr->vec = frame_vector_create(npages);
> -       if (!userptr->vec) {
> +       userptr->pages = kvmalloc_array(npages, sizeof(*userptr->pages),
> +                                       GFP_KERNEL);
> +       if (!userptr->pages) {
>                 dev_err(hdev->dev, "Failed to create frame vector\n");
>                 return -ENOMEM;
>         }
Hi Daniel, sorry but missed this in my initial review.
The error message no longer fits the code, and actually it isn't
needed as we don't print error messages on malloc failures.
With that fixed, this patch is:
Reviewed-by: Oded Gabbay <ogabbay@kernel.org>

>
> -       rc = get_vaddr_frames(start, npages, FOLL_FORCE | FOLL_WRITE,
> -                               userptr->vec);
> +       rc = pin_user_pages_fast(start, npages, FOLL_FORCE | FOLL_WRITE,
> +                                userptr->pages);
>
>         if (rc != npages) {
>                 dev_err(hdev->dev,
>                         "Failed to map host memory, user ptr probably wrong\n");
>                 if (rc < 0)
> -                       goto destroy_framevec;
> +                       goto destroy_pages;
> +               npages = rc;
>                 rc = -EFAULT;
> -               goto put_framevec;
> -       }
> -
> -       if (frame_vector_to_pages(userptr->vec) < 0) {
> -               dev_err(hdev->dev,
> -                       "Failed to translate frame vector to pages\n");
> -               rc = -EFAULT;
> -               goto put_framevec;
> +               goto put_pages;
>         }
> +       userptr->npages = npages;
>
>         rc = sg_alloc_table_from_pages(userptr->sgt,
> -                                       frame_vector_pages(userptr->vec),
> -                                       npages, offset, size, GFP_ATOMIC);
> +                                      userptr->pages,
> +                                      npages, offset, size, GFP_ATOMIC);
>         if (rc < 0) {
>                 dev_err(hdev->dev, "failed to create SG table from pages\n");
> -               goto put_framevec;
> +               goto put_pages;
>         }
>
>         return 0;
>
> -put_framevec:
> -       put_vaddr_frames(userptr->vec);
> -destroy_framevec:
> -       frame_vector_destroy(userptr->vec);
> +put_pages:
> +       unpin_user_pages(userptr->pages, npages);
> +destroy_pages:
> +       kvfree(userptr->pages);
>         return rc;
>  }
>
> @@ -1415,8 +1411,6 @@ int hl_pin_host_memory(struct hl_device *hdev, u64 addr, u64 size,
>   */
>  void hl_unpin_host_memory(struct hl_device *hdev, struct hl_userptr *userptr)
>  {
> -       struct page **pages;
> -
>         hl_debugfs_remove_userptr(hdev, userptr);
>
>         if (userptr->dma_mapped)
> @@ -1424,15 +1418,8 @@ void hl_unpin_host_memory(struct hl_device *hdev, struct hl_userptr *userptr)
>                                                         userptr->sgt->nents,
>                                                         userptr->dir);
>
> -       pages = frame_vector_pages(userptr->vec);
> -       if (!IS_ERR(pages)) {
> -               int i;
> -
> -               for (i = 0; i < frame_vector_count(userptr->vec); i++)
> -                       set_page_dirty_lock(pages[i]);
> -       }
> -       put_vaddr_frames(userptr->vec);
> -       frame_vector_destroy(userptr->vec);
> +       unpin_user_pages_dirty_lock(userptr->pages, userptr->npages, true);
> +       kvfree(userptr->pages);
>
>         list_del(&userptr->job_node);
>
> --
> 2.29.2
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

  reply index

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-19 14:41 [PATCH v6 00/17] follow_pfn and other iomap races Daniel Vetter
2020-11-19 14:41 ` [PATCH v6 01/17] drm/exynos: Stop using frame_vector helpers Daniel Vetter
2020-11-19 14:41 ` [PATCH v6 02/17] drm/exynos: Use FOLL_LONGTERM for g2d cmdlists Daniel Vetter
2020-11-19 14:41 ` [PATCH v6 03/17] misc/habana: Stop using frame_vector helpers Daniel Vetter
2020-11-21 12:47   ` Oded Gabbay [this message]
2020-11-19 14:41 ` [PATCH v6 04/17] misc/habana: Use FOLL_LONGTERM for userptr Daniel Vetter
2020-11-21 10:15   ` Oded Gabbay
2020-11-19 14:41 ` [PATCH v6 05/17] mm/frame-vector: Use FOLL_LONGTERM Daniel Vetter
2020-11-19 14:41 ` [PATCH v6 06/17] media: videobuf2: Move frame_vector into media subsystem Daniel Vetter
2020-11-20  8:07   ` Hans Verkuil
2020-11-19 14:41 ` [PATCH v6 07/17] mm: Close race in generic_access_phys Daniel Vetter
2020-11-19 14:41 ` [PATCH v6 08/17] mm: Add unsafe_follow_pfn Daniel Vetter
2020-11-19 14:41 ` [PATCH v6 09/17] media/videbuf1|2: Mark follow_pfn usage as unsafe Daniel Vetter
2020-11-20  8:06   ` Hans Verkuil
2020-11-20  8:28     ` Hans Verkuil
2020-11-20  8:32       ` Tomasz Figa
2020-11-20  9:18       ` Daniel Vetter
2020-11-20 10:38         ` Hans Verkuil
2020-11-20 10:51           ` Daniel Vetter
2020-11-20 12:08             ` Hans Verkuil
2020-11-20 12:23               ` Tomasz Figa
2020-11-24 14:16                 ` Daniel Vetter
2020-11-19 14:41 ` [PATCH v6 10/17] vfio/type1: Mark follow_pfn " Daniel Vetter
2020-11-19 14:41 ` [PATCH v6 11/17] PCI: Obey iomem restrictions for procfs mmap Daniel Vetter
2020-11-19 14:41 ` [PATCH v6 12/17] /dev/mem: Only set filp->f_mapping Daniel Vetter
2020-11-19 14:41 ` [PATCH v6 13/17] resource: Move devmem revoke code to resource framework Daniel Vetter
2020-11-19 14:41 ` [PATCH v6 14/17] sysfs: Support zapping of binary attr mmaps Daniel Vetter
2020-11-19 14:41 ` [PATCH v6 15/17] PCI: Revoke mappings like devmem Daniel Vetter
2020-11-19 14:41 ` [PATCH v6 16/17] RFC: kvm: pass kvm argument to follow_pfn callsites Daniel Vetter
2020-11-20 15:33   ` Paolo Bonzini
2020-11-20 15:44     ` Daniel Vetter
2020-11-20 15:55       ` Paolo Bonzini
2020-11-19 14:41 ` [PATCH v6 17/17] RFC: mm: add mmu_notifier argument to follow_pfn Daniel Vetter
2020-11-20 18:30   ` Jason Gunthorpe
2020-11-24 14:28     ` Daniel Vetter
2020-11-24 15:55       ` Jason Gunthorpe
2020-11-25  9:00         ` Daniel Vetter
2020-11-27 13:12 ` [PATCH v6 00/17] follow_pfn and other iomap races Jason Gunthorpe
2020-11-27 15:36   ` Daniel Vetter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAFCwf12Hrjm1La_qgrpW_E0WryiBXYzOobMMGEYsxh9kg=3_AA@mail.gmail.com' \
    --to=oded.gabbay@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=dan.j.williams@intel.com \
    --cc=daniel.vetter@ffwll.ch \
    --cc=daniel.vetter@intel.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=hch@infradead.org \
    --cc=jack@suse.cz \
    --cc=jgg@ziepe.ca \
    --cc=jglisse@redhat.com \
    --cc=jhubbard@nvidia.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-samsung-soc@vger.kernel.org \
    --cc=mhaimovski@habana.ai \
    --cc=obitton@habana.ai \
    --cc=oshpigelman@habana.ai \
    --cc=ppiskorski@habana.ai \
    --cc=ttayar@habana.ai \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

dri-devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dri-devel/0 dri-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dri-devel dri-devel/ https://lore.kernel.org/dri-devel \
		dri-devel@lists.freedesktop.org
	public-inbox-index dri-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.freedesktop.lists.dri-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git