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=-13.6 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,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 82728C433DB for ; Mon, 22 Mar 2021 14:22:32 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id D71046196C for ; Mon, 22 Mar 2021 14:22:31 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org D71046196C 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 05BBA6B00D0; Mon, 22 Mar 2021 10:03:40 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id F34856B00D2; Mon, 22 Mar 2021 10:03:39 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 984EF6B00D3; Mon, 22 Mar 2021 10:03:39 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0107.hostedemail.com [216.40.44.107]) by kanga.kvack.org (Postfix) with ESMTP id 67DB56B00D0 for ; Mon, 22 Mar 2021 10:03:39 -0400 (EDT) Received: from smtpin30.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id C27308249980 for ; Mon, 22 Mar 2021 14:22:29 +0000 (UTC) X-FDA: 77947725618.30.1DE059B Received: from mail-wr1-f44.google.com (mail-wr1-f44.google.com [209.85.221.44]) by imf07.hostedemail.com (Postfix) with ESMTP id 77C7AA001ABF for ; Mon, 22 Mar 2021 14:22:17 +0000 (UTC) Received: by mail-wr1-f44.google.com with SMTP id v11so17117790wro.7 for ; Mon, 22 Mar 2021 07:22:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=P7kM0ec17kAxJ7OeBgoamSTjvACVf0yQ0nbW97EJ9+w=; b=Ark5NE1ZMAG2EM3A8upZSyFaUgGhOogaj80j93mQsKDyA0T4O5gv3pwCRzfY8MWJYq hUWcr1zL27mqm5omXJeGZQvV/Z7U14LOKA7YpJAewrhl3LhRRqF6hUDbvvVYtAkCNjll Eb4fn/xWs2hYgCJEwfnXI3vWkykjd4wlY9VS8= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-disposition :content-transfer-encoding:in-reply-to; bh=P7kM0ec17kAxJ7OeBgoamSTjvACVf0yQ0nbW97EJ9+w=; b=GuJxJeUwZ1MhwwRM/6wnj0id9AFbNobhYCWudjizBYT+a20DUZcYM+a1YuUEI6plJ7 N8PfRg/OBo882SCPDN+5eZMHnuzoSxtuY8inQ1MNVXQDxRtNwVFu/KR4lXqkCNorvjrl hlrXgq07N0g4Z/Llug69zamKRiagWAKBf3cQHTY7e3QeNdHXGbYRMXyHgClRM0GUNprc eMx4FZ61SIZPFuivs0MhjhDs3+p8DouD69nzCTKaIx033CTxyBj6E1YWJShbIM27Bj/v lhBJ5eyIY5jeEAYa8b+SvWbcRO+keHXUEpnerUDd/LZmQD+n/8+QBtrC87fotfdn9R+J ipiA== X-Gm-Message-State: AOAM530lJ2EiDb2n/31qxZNlESQwDQfq5N4njzhO3GFJ75wI3QSlF85q 2jNOzWg+cm/kaJMnzEcJVAakTg== X-Google-Smtp-Source: ABdhPJzGaDnzwHoePMLQPQyGI6MVZ+JS6XIpmJfbcwANUFMYpQEVnC7/ci8tI2vAgrEgXYf/LNSMAw== X-Received: by 2002:adf:ba87:: with SMTP id p7mr18868962wrg.298.1616422925721; Mon, 22 Mar 2021 07:22:05 -0700 (PDT) Received: from phenom.ffwll.local ([2a02:168:57f4:0:efd0:b9e5:5ae6:c2fa]) by smtp.gmail.com with ESMTPSA id m15sm18866008wrp.96.2021.03.22.07.22.04 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 22 Mar 2021 07:22:04 -0700 (PDT) Date: Mon, 22 Mar 2021 15:22:02 +0100 From: Daniel Vetter To: Matthew Wilcox Cc: Christian =?iso-8859-1?Q?K=F6nig?= , Dave Chinner , Leo Liu , amd-gfx list , dri-devel , Linux MM , mhocko@kernel.org Subject: Re: [PATCH] drm/ttm: stop warning on TT shrinker failure Message-ID: Mail-Followup-To: Matthew Wilcox , Christian =?iso-8859-1?Q?K=F6nig?= , Dave Chinner , Leo Liu , amd-gfx list , dri-devel , Linux MM , mhocko@kernel.org References: <20210319140857.2262-1-christian.koenig@amd.com> <2831bfcc-140e-dade-1f50-a6431e495e9d@gmail.com> <1ae415c4-8e49-5183-b44d-bc92088657d5@gmail.com> <20210322140548.GN1719932@casper.infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20210322140548.GN1719932@casper.infradead.org> X-Operating-System: Linux phenom 5.7.0-1-amd64 X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 77C7AA001ABF X-Stat-Signature: b9fghme4itkn6c6tnjeu75q3f157z8e1 Received-SPF: none (ffwll.ch>: No applicable sender policy available) receiver=imf07; identity=mailfrom; envelope-from=""; helo=mail-wr1-f44.google.com; client-ip=209.85.221.44 X-HE-DKIM-Result: pass/pass X-HE-Tag: 1616422937-601708 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 Mon, Mar 22, 2021 at 02:05:48PM +0000, Matthew Wilcox wrote: > On Mon, Mar 22, 2021 at 02:49:27PM +0100, Daniel Vetter wrote: > > On Sun, Mar 21, 2021 at 03:18:28PM +0100, Christian K=F6nig wrote: > > > Am 20.03.21 um 14:17 schrieb Daniel Vetter: > > > > On Sat, Mar 20, 2021 at 10:04 AM Christian K=F6nig > > > > wrote: > > > > > Am 19.03.21 um 20:06 schrieb Daniel Vetter: > > > > > > On Fri, Mar 19, 2021 at 07:53:48PM +0100, Christian K=F6nig w= rote: > > > > > > > Am 19.03.21 um 18:52 schrieb Daniel Vetter: > > > > > > > > On Fri, Mar 19, 2021 at 03:08:57PM +0100, Christian K=F6n= ig wrote: > > > > > > > > > Don't print a warning when we fail to allocate a page f= or swapping things out. > > > > > > > > >=20 > > > > > > > > > Also rely on memalloc_nofs_save/memalloc_nofs_restore i= nstead of GFP_NOFS. > > > > > > > > Uh this part doesn't make sense. Especially since you onl= y do it for the > > > > > > > > debugfs file, not in general. Which means you've just com= pletely broken > > > > > > > > the shrinker. > > > > > > > Are you sure? My impression is that GFP_NOFS should now wor= k much more out > > > > > > > of the box with the memalloc_nofs_save()/memalloc_nofs_rest= ore(). > > > > > > Yeah, if you'd put it in the right place :-) > > > > > >=20 > > > > > > But also -mm folks are very clear that memalloc_no*() family = is for dire > > > > > > situation where there's really no other way out. For anything= where you > > > > > > know what you're doing, you really should use explicit gfp fl= ags. > > > > > My impression is just the other way around. You should try to a= void the > > > > > NOFS/NOIO flags and use the memalloc_no* approach instead. > > > > Where did you get that idea? > > >=20 > > > Well from the kernel comment on GFP_NOFS: > > >=20 > > > =A0* %GFP_NOFS will use direct reclaim but will not use any filesys= tem > > > interfaces. > > > =A0* Please try to avoid using this flag directly and instead use > > > =A0* memalloc_nofs_{save,restore} to mark the whole scope which > > > cannot/shouldn't > > > =A0* recurse into the FS layer with a short explanation why. All al= location > > > =A0* requests will inherit GFP_NOFS implicitly. > >=20 > > Huh that's interesting, since iirc Willy or Dave told me the opposite= , and > > the memalloc_no* stuff is for e.g. nfs calling into network layer (ne= eds > > GFP_NOFS) or swap on top of a filesystems (even needs GFP_NOIO I thin= k). > >=20 > > Adding them, maybe I got confused. >=20 > My impression is that the scoped API is preferred these days. >=20 > https://www.kernel.org/doc/html/latest/core-api/gfp_mask-from-fs-io.htm= l >=20 > I'd probably need to spend a few months learning the DRM subsystem to > have a more detailed opinion on whether passing GFP flags around explic= itly > or using the scope API is the better approach for your situation. Atm it's a single allocation in the ttm shrinker that's already explicitl= y using GFP_NOFS that we're talking about here. The scoped api might make sense for gpu scheduler, where we really operat= e under GFP_NOWAIT for somewhat awkward reasons. But also I thought at leas= t for GFP_NOIO you generally need a mempool and think about how you guarantee forward progress anyway. Is that also a bit outdated thinking, and nowadays we could operate under the assumption that this Just Works? Given that GFP_NOFS seems to fall over already for us I'm not super sure about that ... > I usually defer to Michal on these kinds of questions. >=20 > > > > The kernel is full of explicit gfp_t flag > > > > passing to make this as explicit as possible. The memalloc_no* st= uff > > > > is just for when you go through entire subsystems and really can'= t > > > > wire it through. I can't find the discussion anymore, but that wa= s the > > > > advice I got from mm/fs people. > > > >=20 > > > > One reason is that generally a small GFP_KERNEL allocation never > > > > fails. But it absolutely can fail if it's in a memalloc_no* secti= on, > > > > and these kind of non-obvious non-local effects are a real pain i= n > > > > testing and review. Hence explicit gfp_flag passing as much as > > > > possible. >=20 > I agree with this; it's definitely a problem with the scope API. I wan= ted > to extend it to include GFP_NOWAIT, but if you do that, your chances of > memory allocation failure go way up, so you really want to set __GFP_NO= WARN > too, but now you need to audit all the places that you're calling to be > sure they really handle errors correctly. >=20 > So I think I'm giving up on that patch set. Yeah the auditing is what scares me, and why at least personally I prefer explicit gfp flags. It's much easier to debug a lockdep splat involving fs_reclaim than memory allocation failures leading to very strange bugs because we're not handling the allocation failure properly (or maybe not even at all). -Daniel >=20 > > > > > > > > If this is just to paper over the seq_printf doing the wr= ong allocations, > > > > > > > > then just move that out from under the fs_reclaim_acquire= /release part. > > > > > > > No, that wasn't the problem. > > > > > > >=20 > > > > > > > We have just seen to many failures to allocate pages for sw= apout and I think > > > > > > > that would improve this because in a lot of cases we can th= en immediately > > > > > > > swap things out instead of having to rely on upper layers. > > > > > > Yeah, you broke it. Now the real shrinker is running with GFP= _KERNEL, > > > > > > because your memalloc_no is only around the debugfs function.= And ofc it's > > > > > > much easier to allocate with GFP_KERNEL, right until you dead= lock :-) > > > > > The problem here is that for example kswapd calls the shrinker = without > > > > > holding a FS lock as far as I can see. > > > > >=20 > > > > > And it is rather sad that we can't optimize this case directly. > > > > I'm still not clear what you want to optimize? You can check for = "is > > > > this kswapd" in pf flags, but that sounds very hairy and fragile. > > >=20 > > > Well we only need the NOFS flag when the shrinker callback really c= omes from > > > a memory shortage in the FS subsystem, and that is rather unlikely. > > >=20 > > > When we would allow all other cases to be able to directly IO the f= reed up > > > pages to swap it would certainly help. > >=20 > > tbh I'm not sure. i915-gem code has played tricks with special casing= the > > kswapd path, and they do kinda scare me at least. I'm not sure whethe= r > > there's not some hidden dependencies there that would make this a bad > > idea. Like afaik direct reclaim can sometimes stall for kswapd to cat= ch up > > a bit, or at least did in the past (I think, really not much clue abo= ut > > this) > >=20 > > The other thing is that the fs_reclaim_acquire/release annotation rea= lly > > only works well if you use it outside of the direct reclaim path too. > > Otherwise it's not much better than just lots of testing. That pretty= much > > means you have to annotate the kswapd path. > > -Daniel > >=20 > >=20 > >=20 > > >=20 > > > Christian. > > >=20 > > > > -Daniel > > > >=20 > > > > > Anyway you are right if some caller doesn't use the memalloc_no= *() > > > > > approach we are busted. > > > > >=20 > > > > > Going to change the patch to only not warn for the moment. > > > > >=20 > > > > > Regards, > > > > > Christian. > > > > >=20 > > > > > > Shrinking is hard, there's no easy way out here. > > > > > >=20 > > > > > > Cheers, Daniel > > > > > >=20 > > > > > > > Regards, > > > > > > > Christian. > > > > > > >=20 > > > > > > >=20 > > > > > > > > __GFP_NOWARN should be there indeed I think. > > > > > > > > -Daniel > > > > > > > >=20 > > > > > > > > > Signed-off-by: Christian K=F6nig > > > > > > > > > --- > > > > > > > > > drivers/gpu/drm/ttm/ttm_tt.c | 5 ++++- > > > > > > > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > > > >=20 > > > > > > > > > diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu= /drm/ttm/ttm_tt.c > > > > > > > > > index 2f0833c98d2c..86fa3e82dacc 100644 > > > > > > > > > --- a/drivers/gpu/drm/ttm/ttm_tt.c > > > > > > > > > +++ b/drivers/gpu/drm/ttm/ttm_tt.c > > > > > > > > > @@ -369,7 +369,7 @@ static unsigned long ttm_tt_shrinke= r_scan(struct shrinker *shrink, > > > > > > > > > }; > > > > > > > > > int ret; > > > > > > > > > - ret =3D ttm_bo_swapout(&ctx, GFP_NOFS); > > > > > > > > > + ret =3D ttm_bo_swapout(&ctx, GFP_KERNEL | __GFP_NOWA= RN); > > > > > > > > > return ret < 0 ? SHRINK_EMPTY : ret; > > > > > > > > > } > > > > > > > > > @@ -389,10 +389,13 @@ static unsigned long ttm_tt_shrin= ker_count(struct shrinker *shrink, > > > > > > > > > static int ttm_tt_debugfs_shrink_show(struct seq_fi= le *m, void *data) > > > > > > > > > { > > > > > > > > > struct shrink_control sc =3D { .gfp_mask =3D= GFP_KERNEL }; > > > > > > > > > + unsigned int flags; > > > > > > > > > fs_reclaim_acquire(GFP_KERNEL); > > > > > > > > > + flags =3D memalloc_nofs_save(); > > > > > > > > > seq_printf(m, "%lu/%lu\n", ttm_tt_shrinker_= count(&mm_shrinker, &sc), > > > > > > > > > ttm_tt_shrinker_scan(&mm_shrinke= r, &sc)); > > > > > > > > > + memalloc_nofs_restore(flags); > > > > > > > > > fs_reclaim_release(GFP_KERNEL); > > > > > > > > > return 0; > > > > > > > > > -- > > > > > > > > > 2.25.1 > > > > > > > > >=20 > > > > > > > > > _______________________________________________ > > > > > > > > > dri-devel mailing list > > > > > > > > > dri-devel@lists.freedesktop.org > > > > > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-deve= l > > > >=20 > > >=20 > >=20 > > --=20 > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel --=20 Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch