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=-12.1 required=3.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED,DKIM_INVALID,DKIM_SIGNED,FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 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 D7E99C433C1 for ; Thu, 25 Mar 2021 08:07:55 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 36B8961A10 for ; Thu, 25 Mar 2021 08:07:55 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 36B8961A10 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id A9C876B006C; Thu, 25 Mar 2021 04:07:54 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id A723F6B006E; Thu, 25 Mar 2021 04:07:54 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 911256B0070; Thu, 25 Mar 2021 04:07:54 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0166.hostedemail.com [216.40.44.166]) by kanga.kvack.org (Postfix) with ESMTP id 70EAB6B006C for ; Thu, 25 Mar 2021 04:07:54 -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 37F4082499A8 for ; Thu, 25 Mar 2021 08:07:54 +0000 (UTC) X-FDA: 77957668068.30.464C3A6 Received: from mail-ej1-f49.google.com (mail-ej1-f49.google.com [209.85.218.49]) by imf26.hostedemail.com (Postfix) with ESMTP id 67065407F8FE for ; Thu, 25 Mar 2021 08:07:52 +0000 (UTC) Received: by mail-ej1-f49.google.com with SMTP id e14so1388219ejz.11 for ; Thu, 25 Mar 2021 01:07:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:references:from:message-id:date:user-agent:mime-version :in-reply-to:content-transfer-encoding:content-language; bh=d5i7kXxGp1eybtz9ayp4p92UOrN6erTJVkkr1QUx4ao=; b=HBZAfwJJii/aEobIKIEkCHsHiDp8oPanCnfhVYkAFznpgFBlQ4fF6IvNgfLmnePkjQ D6NIZv/vRNEsmG3FoI7JqMGkKDiDiB1ExuynEurpM1rx+L8CBvqkia2YUbA/mW2+sKs7 6HKMa8uhMEIfP4OpiKoMNw70Y5zGNKlZmaoq30+BrQzZ42t7lc+o2PHAMQKIfxsJ/L9m Qx1UCUDkkZTWU2XiMIZofnBmE1oEeB4GxmM4vKj6xG3gPcLEoE4RQTJJy3iev3t+lT/l DSzOMYsquRSX8BynS+92dukjIq8T847/bBTQuT/JEmBM2N2xxXJIKtZ6FMqxOpMfy3Cv 1N+w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding :content-language; bh=d5i7kXxGp1eybtz9ayp4p92UOrN6erTJVkkr1QUx4ao=; b=aZaYamrq9mfXwpIfJVJ4jj2s+QSmykgq2+OpnyDfCB5eV7reZje+A98bepwAcN29JX dn/p5qHlTBnuGtz3iTq7YE6DB3XS5oCWJ/e3rQAQJQydLWZrPWYw5n8PBG6rQ6c2MWN1 i75yodLcc4vKbets6xyNlGvqZz5wCUz6Beu7q9zLk3mPY9ztZ3UIf64Jo+cGo7S4+O0S 7kw7QNuRz0xGkUrt5YPSuEerwXoI5xykgVJx2+6JuaXxq3Oqm/WDC3aWmySC2oU5DAd5 7VSfic9JFClEI4vOiVp+9SnAsw9Oco9W6QTCGeYtOLwclqA4Qvn+rD3tYM3R3r8VPgfs AwFw== X-Gm-Message-State: AOAM533u+SD4eF+df9U0B3RLF7El2uYO3TX11Pa+Z6LSfOqS7h+fxlj8 7YZ/BMujN4yXRAefhtLoPI0= X-Google-Smtp-Source: ABdhPJythe67rC5/cZGQh97IYBA0ZTyVWSV1Q5Gdf0YN2beV1droRekGYY38/fwjK8UkK5pK2nSjzQ== X-Received: by 2002:a17:906:c301:: with SMTP id s1mr7831510ejz.382.1616659672434; Thu, 25 Mar 2021 01:07:52 -0700 (PDT) Received: from ?IPv6:2a02:908:1252:fb60:a792:596e:3412:8626? ([2a02:908:1252:fb60:a792:596e:3412:8626]) by smtp.gmail.com with ESMTPSA id c19sm2308151edu.20.2021.03.25.01.07.51 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 25 Mar 2021 01:07:51 -0700 (PDT) Subject: Re: [PATCH] drm/ttm: switch back to static allocation limits for now To: dri-devel@lists.freedesktop.org, linux-mm@kvack.org, Liang.Liang@amd.com, thomas_os@shipmail.org References: <20210324134845.2338-1-christian.koenig@amd.com> From: =?UTF-8?Q?Christian_K=c3=b6nig?= Message-ID: <0f2308b4-f36e-ab02-c26d-4065e9972b50@gmail.com> Date: Thu, 25 Mar 2021 09:07:51 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 67065407F8FE X-Stat-Signature: zt8qcidb348bn7scfg5qcnjsh5urgt78 Received-SPF: none (gmail.com>: No applicable sender policy available) receiver=imf26; identity=mailfrom; envelope-from=""; helo=mail-ej1-f49.google.com; client-ip=209.85.218.49 X-HE-DKIM-Result: pass/pass X-HE-Tag: 1616659672-659714 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: Am 24.03.21 um 20:29 schrieb Daniel Vetter: > On Wed, Mar 24, 2021 at 02:48:45PM +0100, Christian K=C3=B6nig wrote: >> The shrinker based approach still has some flaws. Especially that we n= eed >> temporary pages to free up the pages allocated to the driver is proble= matic >> in a shrinker. >> >> Signed-off-by: Christian K=C3=B6nig >> --- >> drivers/gpu/drm/ttm/ttm_device.c | 14 ++-- >> drivers/gpu/drm/ttm/ttm_tt.c | 112 ++++++++++++-----------------= -- >> include/drm/ttm/ttm_tt.h | 3 +- >> 3 files changed, 53 insertions(+), 76 deletions(-) >> >> diff --git a/drivers/gpu/drm/ttm/ttm_device.c b/drivers/gpu/drm/ttm/tt= m_device.c >> index 95e1b7b1f2e6..388da2a7f0bb 100644 >> --- a/drivers/gpu/drm/ttm/ttm_device.c >> +++ b/drivers/gpu/drm/ttm/ttm_device.c >> @@ -53,7 +53,6 @@ static void ttm_global_release(void) >> goto out; >> =20 >> ttm_pool_mgr_fini(); >> - ttm_tt_mgr_fini(); >> =20 >> __free_page(glob->dummy_read_page); >> memset(glob, 0, sizeof(*glob)); >> @@ -64,7 +63,7 @@ static void ttm_global_release(void) >> static int ttm_global_init(void) >> { >> struct ttm_global *glob =3D &ttm_glob; >> - unsigned long num_pages; >> + unsigned long num_pages, num_dma32; >> struct sysinfo si; >> int ret =3D 0; >> unsigned i; >> @@ -79,8 +78,15 @@ static int ttm_global_init(void) >> * system memory. >> */ >> num_pages =3D ((u64)si.totalram * si.mem_unit) >> PAGE_SHIFT; >> - ttm_pool_mgr_init(num_pages * 50 / 100); >> - ttm_tt_mgr_init(); >> + num_pages /=3D 2; >> + >> + /* But for DMA32 we limit ourself to only use 2GiB maximum. */ >> + num_dma32 =3D (u64)(si.totalram - si.totalhigh) * si.mem_unit >> + >> PAGE_SHIFT; >> + num_dma32 =3D min(num_dma32, 2UL << (30 - PAGE_SHIFT)); >> + >> + ttm_pool_mgr_init(num_pages); >> + ttm_tt_mgr_init(num_pages, num_dma32); >> =20 >> spin_lock_init(&glob->lru_lock); >> glob->dummy_read_page =3D alloc_page(__GFP_ZERO | GFP_DMA32); >> diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt= .c >> index 2f0833c98d2c..5d8820725b75 100644 >> --- a/drivers/gpu/drm/ttm/ttm_tt.c >> +++ b/drivers/gpu/drm/ttm/ttm_tt.c >> @@ -40,8 +40,18 @@ >> =20 >> #include "ttm_module.h" >> =20 >> -static struct shrinker mm_shrinker; >> -static atomic_long_t swapable_pages; >> +static unsigned long ttm_pages_limit; >> + >> +MODULE_PARM_DESC(pages_limit, "Limit for the allocated pages"); >> +module_param_named(pages_limit, ttm_pages_limit, ulong, 0644); >> + >> +static unsigned long ttm_dma32_pages_limit; >> + >> +MODULE_PARM_DESC(dma32_pages_limit, "Limit for the allocated DMA32 pa= ges"); >> +module_param_named(dma32_pages_limit, ttm_dma32_pages_limit, ulong, 0= 644); >> + >> +static atomic_long_t ttm_pages_allocated; >> +static atomic_long_t ttm_dma32_pages_allocated; > Making this configurable looks an awful lot like "job done, move on". J= ust > the revert to hardcoded 50% (or I guess just revert the shrinker patch = at > that point) for -fixes is imo better. Well this is the revert to hardcoded 50%. We had that configurable=20 before and have it configurable now. Reverting everything back would mean that we also need to revert the=20 sysfs changes and move all the memory accounting code from VMWGFX back=20 into TTM. Christian. > > Then I guess retry again for 5.14 or so. > -Daniel > >> =20 >> /* >> * Allocates a ttm structure for the given BO. >> @@ -294,8 +304,6 @@ static void ttm_tt_add_mapping(struct ttm_device *= bdev, struct ttm_tt *ttm) >> =20 >> for (i =3D 0; i < ttm->num_pages; ++i) >> ttm->pages[i]->mapping =3D bdev->dev_mapping; >> - >> - atomic_long_add(ttm->num_pages, &swapable_pages); >> } >> =20 >> int ttm_tt_populate(struct ttm_device *bdev, >> @@ -309,12 +317,25 @@ int ttm_tt_populate(struct ttm_device *bdev, >> if (ttm_tt_is_populated(ttm)) >> return 0; >> =20 >> + atomic_long_add(ttm->num_pages, &ttm_pages_allocated); >> + if (bdev->pool.use_dma32) >> + atomic_long_add(ttm->num_pages, &ttm_dma32_pages_allocated); >> + >> + while (atomic_long_read(&ttm_pages_allocated) > ttm_pages_limit || >> + atomic_long_read(&ttm_dma32_pages_allocated) > >> + ttm_dma32_pages_limit) { >> + >> + ret =3D ttm_bo_swapout(ctx, GFP_KERNEL); >> + if (ret) >> + goto error; >> + } >> + >> if (bdev->funcs->ttm_tt_populate) >> ret =3D bdev->funcs->ttm_tt_populate(bdev, ttm, ctx); >> else >> ret =3D ttm_pool_alloc(&bdev->pool, ttm, ctx); >> if (ret) >> - return ret; >> + goto error; >> =20 >> ttm_tt_add_mapping(bdev, ttm); >> ttm->page_flags |=3D TTM_PAGE_FLAG_PRIV_POPULATED; >> @@ -327,6 +348,12 @@ int ttm_tt_populate(struct ttm_device *bdev, >> } >> =20 >> return 0; >> + >> +error: >> + atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); >> + if (bdev->pool.use_dma32) >> + atomic_long_sub(ttm->num_pages, &ttm_dma32_pages_allocated); >> + return ret; >> } >> EXPORT_SYMBOL(ttm_tt_populate); >> =20 >> @@ -342,12 +369,9 @@ static void ttm_tt_clear_mapping(struct ttm_tt *t= tm) >> (*page)->mapping =3D NULL; >> (*page++)->index =3D 0; >> } >> - >> - atomic_long_sub(ttm->num_pages, &swapable_pages); >> } >> =20 >> -void ttm_tt_unpopulate(struct ttm_device *bdev, >> - struct ttm_tt *ttm) >> +void ttm_tt_unpopulate(struct ttm_device *bdev, struct ttm_tt *ttm) >> { >> if (!ttm_tt_is_populated(ttm)) >> return; >> @@ -357,76 +381,24 @@ void ttm_tt_unpopulate(struct ttm_device *bdev, >> bdev->funcs->ttm_tt_unpopulate(bdev, ttm); >> else >> ttm_pool_free(&bdev->pool, ttm); >> - ttm->page_flags &=3D ~TTM_PAGE_FLAG_PRIV_POPULATED; >> -} >> - >> -/* As long as pages are available make sure to release at least one *= / >> -static unsigned long ttm_tt_shrinker_scan(struct shrinker *shrink, >> - struct shrink_control *sc) >> -{ >> - struct ttm_operation_ctx ctx =3D { >> - .no_wait_gpu =3D false >> - }; >> - int ret; >> - >> - ret =3D ttm_bo_swapout(&ctx, GFP_NOFS); >> - return ret < 0 ? SHRINK_EMPTY : ret; >> -} >> - >> -/* Return the number of pages available or SHRINK_EMPTY if we have no= ne */ >> -static unsigned long ttm_tt_shrinker_count(struct shrinker *shrink, >> - struct shrink_control *sc) >> -{ >> - unsigned long num_pages; >> - >> - num_pages =3D atomic_long_read(&swapable_pages); >> - return num_pages ? num_pages : SHRINK_EMPTY; >> -} >> =20 >> -#ifdef CONFIG_DEBUG_FS >> + atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); >> + if (bdev->pool.use_dma32) >> + atomic_long_sub(ttm->num_pages, &ttm_dma32_pages_allocated); >> =20 >> -/* Test the shrinker functions and dump the result */ >> -static int ttm_tt_debugfs_shrink_show(struct seq_file *m, void *data) >> -{ >> - struct shrink_control sc =3D { .gfp_mask =3D GFP_KERNEL }; >> - >> - fs_reclaim_acquire(GFP_KERNEL); >> - seq_printf(m, "%lu/%lu\n", ttm_tt_shrinker_count(&mm_shrinker, &sc), >> - ttm_tt_shrinker_scan(&mm_shrinker, &sc)); >> - fs_reclaim_release(GFP_KERNEL); >> - >> - return 0; >> + ttm->page_flags &=3D ~TTM_PAGE_FLAG_PRIV_POPULATED; >> } >> -DEFINE_SHOW_ATTRIBUTE(ttm_tt_debugfs_shrink); >> - >> -#endif >> - >> - >> =20 >> /** >> * ttm_tt_mgr_init - register with the MM shrinker >> * >> * Register with the MM shrinker for swapping out BOs. >> */ >> -int ttm_tt_mgr_init(void) >> +void ttm_tt_mgr_init(unsigned long num_pages, unsigned long num_dma32= _pages) >> { >> -#ifdef CONFIG_DEBUG_FS >> - debugfs_create_file("tt_shrink", 0400, ttm_debugfs_root, NULL, >> - &ttm_tt_debugfs_shrink_fops); >> -#endif >> - >> - mm_shrinker.count_objects =3D ttm_tt_shrinker_count; >> - mm_shrinker.scan_objects =3D ttm_tt_shrinker_scan; >> - mm_shrinker.seeks =3D 1; >> - return register_shrinker(&mm_shrinker); >> -} >> + if (!ttm_pages_limit) >> + ttm_pages_limit =3D num_pages; >> =20 >> -/** >> - * ttm_tt_mgr_fini - unregister our MM shrinker >> - * >> - * Unregisters the MM shrinker. >> - */ >> -void ttm_tt_mgr_fini(void) >> -{ >> - unregister_shrinker(&mm_shrinker); >> + if (!ttm_dma32_pages_limit) >> + ttm_dma32_pages_limit =3D num_dma32_pages; >> } >> diff --git a/include/drm/ttm/ttm_tt.h b/include/drm/ttm/ttm_tt.h >> index 069f8130241a..134d09ef7766 100644 >> --- a/include/drm/ttm/ttm_tt.h >> +++ b/include/drm/ttm/ttm_tt.h >> @@ -157,8 +157,7 @@ int ttm_tt_populate(struct ttm_device *bdev, struc= t ttm_tt *ttm, struct ttm_oper >> */ >> void ttm_tt_unpopulate(struct ttm_device *bdev, struct ttm_tt *ttm); >> =20 >> -int ttm_tt_mgr_init(void); >> -void ttm_tt_mgr_fini(void); >> +void ttm_tt_mgr_init(unsigned long num_pages, unsigned long num_dma32= _pages); >> =20 >> #if IS_ENABLED(CONFIG_AGP) >> #include >> --=20 >> 2.25.1 >>