From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-9.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id DF339C433DF for ; Sat, 10 Oct 2020 21:41:44 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 69D97207CD for ; Sat, 10 Oct 2020 21:41:44 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=ffwll.ch header.i=@ffwll.ch header.b="cODuj2bz" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 69D97207CD Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=ffwll.ch Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id CC42D6B005C; Sat, 10 Oct 2020 17:41:43 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id C74D36B0062; Sat, 10 Oct 2020 17:41:43 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B616A900002; Sat, 10 Oct 2020 17:41:43 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0090.hostedemail.com [216.40.44.90]) by kanga.kvack.org (Postfix) with ESMTP id 879246B005C for ; Sat, 10 Oct 2020 17:41:43 -0400 (EDT) Received: from smtpin20.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 1B3F8180AD806 for ; Sat, 10 Oct 2020 21:41:43 +0000 (UTC) X-FDA: 77357338086.20.seat96_2918610271ec Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin20.hostedemail.com (Postfix) with ESMTP id E2E6D180C07A3 for ; Sat, 10 Oct 2020 21:41:42 +0000 (UTC) X-HE-Tag: seat96_2918610271ec X-Filterd-Recvd-Size: 13026 Received: from mail-ot1-f65.google.com (mail-ot1-f65.google.com [209.85.210.65]) by imf21.hostedemail.com (Postfix) with ESMTP for ; Sat, 10 Oct 2020 21:41:42 +0000 (UTC) Received: by mail-ot1-f65.google.com with SMTP id e20so11871147otj.11 for ; Sat, 10 Oct 2020 14:41:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=iNxwDH5u5TrRQqiTxTiC507fsoXAo+Ear9Qr1cSCDXM=; b=cODuj2bzo2AO+NtsSDmK+GM+IH6Wb2aQIdPJNZwsWPVwppiG83SSt1UXXIp6hTsgX4 TEoJk6lOuTqWZaWdYBoSJlxxzz9Lz3t/mbTiz7Ayr4xGvQyHkg9c04lcUmYMfoSQJIZQ sxhzwWcw4CIeNF13coxREsRH2lgZbzvzGnLIk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=iNxwDH5u5TrRQqiTxTiC507fsoXAo+Ear9Qr1cSCDXM=; b=VEnOnLRGL+Pjkr27XU3hTcZlcH5d9s9B4CvVV9I5SxBVEPS9pKISI5C0VhPH427Xgi zl9pZrsZPsbspOXUP8kNavCHE88WtryaFquACQpBuXCRbS/rCfLn8CEZfP9veABXJLZt QLvNiasADUtwDNxV12wIian9p3kzI5y79NoRaVAruUt0Vzn3K821UicOUiI+inSITQcw 1nUXrFTSWVIh93pwW6kmsAkmyhkrfdB55qMTMib7kgxzrV7m1cbslpbwslj4h0PDe/gb 0gZhPt0AyjtBAflPDtBaNfRZFigyEpxZFLOon7XzvMW0mJnzsO5YlbYwdD1QhwoxbzH4 ogRg== X-Gm-Message-State: AOAM532NTSHMxfql+hZIc/m2BwRl34b9+0iabZBENqZSUwd/EGnsJL1g ljgAIKKRZq/X4g3Gor4uPmNdZkJo6DojE1jQ8J7mNg== X-Google-Smtp-Source: ABdhPJysptysEsT0jSmsHNKPWkJOs+Pv1LX5DB7//lfpdyBsRnuaRszTMCaD3Je1nsfqA1Sk2kecuUszSEGAlMPDXc0= X-Received: by 2002:a05:6830:1c3c:: with SMTP id f28mr14253407ote.188.1602366101323; Sat, 10 Oct 2020 14:41:41 -0700 (PDT) MIME-Version: 1.0 References: <20201009075934.3509076-1-daniel.vetter@ffwll.ch> <20201009075934.3509076-4-daniel.vetter@ffwll.ch> In-Reply-To: From: Daniel Vetter Date: Sat, 10 Oct 2020 23:41:30 +0200 Message-ID: Subject: Re: [PATCH v2 03/17] misc/habana: Stop using frame_vector helpers To: Oded Gabbay Cc: DRI Development , LKML , KVM list , linux-mm , "list@263.net:IOMMU DRIVERS , Joerg Roedel ," , linux-samsung-soc , Linux Media Mailing List , linux-s390 , Daniel Vetter , Jason Gunthorpe , Andrew Morton , John Hubbard , =?UTF-8?B?SsOpcsO0bWUgR2xpc3Nl?= , Jan Kara , Dan Williams , Omer Shpigelman , Ofir Bitton , Tomer Tayar , Moti Haimovski , Greg Kroah-Hartman , Pawel Piskorski Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Sat, Oct 10, 2020 at 11:32 PM Daniel Vetter wro= te: > > On Sat, Oct 10, 2020 at 10:27 PM Oded Gabbay wrot= e: > > > > On Fri, Oct 9, 2020 at 10:59 AM Daniel Vetter = 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_frame= s. > > > > > Thanks for the patch Daniel. > > > > > Signed-off-by: Daniel Vetter > > > Cc: Jason Gunthorpe > > > Cc: Andrew Morton > > > Cc: John Hubbard > > > Cc: J=C3=A9r=C3=B4me Glisse > > > Cc: Jan Kara > > > Cc: Dan Williams > > > 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 > > > Cc: Omer Shpigelman > > > Cc: Ofir Bitton > > > Cc: Tomer Tayar > > > Cc: Moti Haimovski > > > Cc: Daniel Vetter > > > Cc: Greg Kroah-Hartman > > > Cc: Pawel Piskorski > > > -- > > > v2: Use unpin_user_pages_dirty_lock (John) > > > --- > > > drivers/misc/habanalabs/Kconfig | 1 - > > > drivers/misc/habanalabs/common/habanalabs.h | 3 +- > > > drivers/misc/habanalabs/common/memory.c | 49 ++++++++-----------= -- > > > 3 files changed, 20 insertions(+), 33 deletions(-) > > > > > > diff --git a/drivers/misc/habanalabs/Kconfig b/drivers/misc/habanalab= s/Kconfig > > > index 8eb5d38c618e..2f04187f7167 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 DMA_SHARED_BUFFER > > > select GENERIC_ALLOCATOR > > > select HWMON > > > diff --git a/drivers/misc/habanalabs/common/habanalabs.h b/drivers/mi= sc/habanalabs/common/habanalabs.h > > > index edbd627b29d2..c1b3ad613b15 100644 > > > --- a/drivers/misc/habanalabs/common/habanalabs.h > > > +++ b/drivers/misc/habanalabs/common/habanalabs.h > > > @@ -881,7 +881,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; > > Can you please update the kerneldoc comment section of this structure > > according to your changes ? > > Apologies I missed the nice kerneldoc. I'll fix that in the next round. > > > > > 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/h= abanalabs/common/memory.c > > > index 5ff4688683fd..327b64479f97 100644 > > > --- a/drivers/misc/habanalabs/common/memory.c > > > +++ b/drivers/misc/habanalabs/common/memory.c > > > @@ -1281,45 +1281,41 @@ static int get_user_memory(struct hl_device *= hdev, u64 addr, u64 size, > > > return -EFAULT; > > > } > > > > > > - userptr->vec =3D frame_vector_create(npages); > > > - if (!userptr->vec) { > > > + userptr->pages =3D kvmalloc_array(npages, sizeof(*userptr->pa= ges), > > > + GFP_KERNEL); > > > + if (!userptr->pages) { > > > dev_err(hdev->dev, "Failed to create frame vector\n")= ; > > > return -ENOMEM; > > > } > > > > > > - rc =3D get_vaddr_frames(start, npages, FOLL_FORCE | FOLL_WRIT= E, > > > - userptr->vec); > > > + rc =3D pin_user_pages_fast(start, npages, FOLL_FORCE | FOLL_W= RITE, > > > + userptr->pages); > > > > > > if (rc !=3D 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 =3D rc; > > > rc =3D -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 =3D -EFAULT; > > > - goto put_framevec; > > > + goto put_pages; > > > } > > > + userptr->npages =3D npages; > > > > > > rc =3D sg_alloc_table_from_pages(userptr->sgt, > > > - frame_vector_pages(userptr->v= ec), > > > - npages, offset, size, GFP_ATO= MIC); > > > + userptr->pages, > > > + npages, offset, size, GFP_ATOM= IC); > > I think that because the call to kvmalloc_array() is done with > > GFP_KERNEL, there is no point in using GFP_ATOMIC here. > > And actually, this path only needs to avoid yielding when using a > > special debug mode. > > So I suggest putting here GFP_KERNEL. > > Huh, I didn't even notice the GFP_ATOMIC here. This looks indeed > strange and GFP_KERNEL should be perfectly fine in a function that > also calls pin_user_pages (since that one can allocate and do worse > stuff like userspace pagefaults). > > But since that GFP_ATOMIC is there already I'll do that in a separate pat= ch. Ok I read up on your usage of GFP_ATOMIC in habanalabs, and I'm not going to touch this. But I'm pretty sure it's broken. You seem to have some requirement of not allocating memory with blocking (see hl_cb_alloc()), and that seems to be way you allocate tons of structures with GFP_ATOMIC. There's 2 pretty tough problems with that: - GFP_ATOMIC can fail, even when the system hasn't run out of memory yet. You _must_ have a fallback back to handle allocation failures for these. Quick survey shows you a ton of GFP_ATOMIC callsites, and very little fallback code - I've found none, but I didn't check the failure handlers all going up the possible callchains. - pin_user_pages can allocate memory, so you're breaking your own "no sleeping in these paths" rules. This isn't going to get fixed with a quick oneliner patch, depending what's needed you're looking at a driver rearchitecture here :-/ Hence I'm not going to touch this in the next patch, but leave it all as-is. Cheers, Daniel > > > In the meanwhile, I'll run this patch (coupled with the next patch) in > > our C/I to make sure there are no regressions. > > Excellent. I'll wait with v3 until that's done, just in case you hit a > snag I need to fix. > > Cheers, Daniel > > > Thanks, > > Oded > > > > > if (rc < 0) { > > > dev_err(hdev->dev, "failed to create SG table from pa= ges\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; > > > } > > > > > > @@ -1405,8 +1401,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) > > > @@ -1414,15 +1408,8 @@ void hl_unpin_host_memory(struct hl_device *hd= ev, struct hl_userptr *userptr) > > > userptr->sgt-= >nents, > > > userptr->dir)= ; > > > > > > - pages =3D frame_vector_pages(userptr->vec); > > > - if (!IS_ERR(pages)) { > > > - int i; > > > - > > > - for (i =3D 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.28.0 > > > > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch --=20 Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch