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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id BCC0BC54E5D for ; Mon, 18 Mar 2024 15:28:28 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 0AEA26B0085; Mon, 18 Mar 2024 11:28:28 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 060356B0087; Mon, 18 Mar 2024 11:28:28 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E41786B0088; Mon, 18 Mar 2024 11:28:27 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id D2F296B0085 for ; Mon, 18 Mar 2024 11:28:27 -0400 (EDT) Received: from smtpin03.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 7EF80A08FC for ; Mon, 18 Mar 2024 15:28:27 +0000 (UTC) X-FDA: 81910541454.03.2F1B2DE Received: from fanzine2.igalia.com (fanzine2.igalia.com [213.97.179.56]) by imf03.hostedemail.com (Postfix) with ESMTP id AFAD820005 for ; Mon, 18 Mar 2024 15:28:23 +0000 (UTC) Authentication-Results: imf03.hostedemail.com; dkim=pass header.d=igalia.com header.s=20170329 header.b=gQrgSVIm; dmarc=none; spf=pass (imf03.hostedemail.com: domain of tvrtko.ursulin@igalia.com designates 213.97.179.56 as permitted sender) smtp.mailfrom=tvrtko.ursulin@igalia.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1710775705; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=g/yrJhHpWEK8h2yuVA/h39Mrnwl0MZC8CF+WSJwnrAo=; b=Fzntmt1OvcE20fZYIO3tY9vly3VwBLlRNBw9l8hkoaeoZdFJQxABX8Op1VwN5A5VbU9CmB TJRVH4mKe+3YwzJwkfNRS3XSziYscKeowrTtFfdWTxgcNuI0AWzrPOkn3//9z1eWUeGYpn DAL6upoUNKnLtZs0T9zlvXNlZ+xafBs= ARC-Authentication-Results: i=1; imf03.hostedemail.com; dkim=pass header.d=igalia.com header.s=20170329 header.b=gQrgSVIm; dmarc=none; spf=pass (imf03.hostedemail.com: domain of tvrtko.ursulin@igalia.com designates 213.97.179.56 as permitted sender) smtp.mailfrom=tvrtko.ursulin@igalia.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1710775705; a=rsa-sha256; cv=none; b=ZKrrs/vfcsRcHoBdspAQFW3GAsrvkqJKVhTFT85l3zGs4uTBOZaVv/T2ztg+WDbsJDYtDT C9pmEgOvQxABBSbItKysQOmmJE2bJKkuhMqZMhIPBcrt/1x6SP3lWBuHhsSTSXGmViINCc ezMkI6W2sOVFPbdE6yVYMOLcZ9UxK4I= DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=igalia.com; s=20170329; h=Content-Transfer-Encoding:Content-Type:In-Reply-To:From: References:Cc:To:Subject:MIME-Version:Date:Message-ID:Sender:Reply-To: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=g/yrJhHpWEK8h2yuVA/h39Mrnwl0MZC8CF+WSJwnrAo=; b=gQrgSVImaqa79bUBUU1GbA76yI fqSxdrZL183aMFcE2FtXB9YZEHas7WjNLkmqOB3nO2tzgspI4csQlzOpgrdXlY6ux7k/QwDLlWkP8 6u7gPQnvOCNZCReuXhaMsdSivSaweb05nDCxm0G/CGOv4Wk5qZoWJKqeNEa6OQlclruDn8Ed4weJm f5Egd+iQmzd5HUR/Kw6lSMYvJOxplvgBJDrNSfJ4lNxvvnkujaLnFzy/bOmuM61KWC/6Aj4Jjsikr J/y4kSKwr0LNhRP811wYZiJt67iWgxVgvg2bh6Kab07Z+VUu0w7j8/S8GuP5dTncJ98l9qu2xVjM/ igtWg7Pg==; Received: from [84.65.0.132] (helo=[192.168.0.101]) by fanzine2.igalia.com with esmtpsa (Cipher TLS1.3:ECDHE_X25519__RSA_PSS_RSAE_SHA256__AES_128_GCM:128) (Exim) id 1rmEtR-00C05E-4A; Mon, 18 Mar 2024 16:27:17 +0100 Message-ID: <0aeba332-73f9-46a7-a6d4-dc1822216b67@igalia.com> Date: Mon, 18 Mar 2024 15:27:14 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 2/5] drm/gem: Add a mountpoint parameter to drm_gem_object_init() Content-Language: en-GB To: =?UTF-8?Q?Christian_K=C3=B6nig?= , =?UTF-8?Q?Ma=C3=ADra_Canal?= , Melissa Wen , Iago Toral , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Daniel Vetter Cc: dri-devel@lists.freedesktop.org, kernel-dev@igalia.com, Russell King , Lucas Stach , Christian Gmeiner , Inki Dae , Seung-Woo Kim , Kyungmin Park , Krzysztof Kozlowski , Alim Akhtar , Patrik Jakobsson , Sui Jingfeng , Chun-Kuang Hu , Philipp Zabel , Matthias Brugger , AngeloGioacchino Del Regno , Rob Clark , Abhinav Kumar , Dmitry Baryshkov , Sean Paul , Marijn Suijten , Karol Herbst , Lyude Paul , Danilo Krummrich , Tomi Valkeinen , Gerd Hoffmann , Sandy Huang , =?UTF-8?Q?Heiko_St=C3=BCbner?= , Andy Yan , Thierry Reding , Mikko Perttunen , Jonathan Hunter , Huang Rui , Oleksandr Andrushchenko , Karolina Stolarek , Andi Shyti , Hugh Dickins , linux-mm@kvack.org References: <20240311100959.205545-1-mcanal@igalia.com> <20240311100959.205545-3-mcanal@igalia.com> <30a7f20b-1f2c-41cb-b193-03429c160b63@igalia.com> <3a5d07c0-120f-47f9-a35a-31f3bfcc9330@amd.com> <07885e3b-ee7b-456b-9fad-17d9009a4cb7@igalia.com> <69576e6d-9704-42b9-905f-289f9f9017b9@amd.com> <9cfb7f83-2d76-4e8d-9052-5975da71e0dc@amd.com> <2118492c-f223-41e9-8c1e-3c03d976301e@igalia.com> <68452c6c-12d7-4e60-9598-369acc9fc360@amd.com> <5f1a758b-b4f6-4fc3-b16b-29d731415636@amd.com> <00a4aa2e-e279-48ff-bc1f-8a72ef5b4491@igalia.com> <28ee9d07-059b-4ac1-97f7-21a74b532b12@amd.com> From: Tvrtko Ursulin In-Reply-To: <28ee9d07-059b-4ac1-97f7-21a74b532b12@amd.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Rspam-User: X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: AFAD820005 X-Stat-Signature: 13i6k777ncehnti3nuetsaq1aqxna3pf X-HE-Tag: 1710775703-818616 X-HE-Meta: U2FsdGVkX18cG6vPM2kxFDZKZZblxClMw1jLZr5wEkzo6+ara2MkaBLEEJv60WVci+q1W0Ug5U4VxmWpjyAi1U2MZqjrP9W6NoCMYuVBWzYG6XiiRvUfu/03LYuTujkXZx81xksBeQV2yCPmcAZkLh9DJrrmLr/6l0ShHRudGKarur/PB2GvAAr7otn0rQ3fHNaiGo/utNCIqXuqox+aPc+0CntLyMeQEciIdCuuoDqUsM3CbE31G8TWTqf9XR/6hmnlu2Q5XOIHkfm4jGQzV/KQGvZeyyxMwd91N8Otnhjnozcvhadg9Ss0AgocQPE9HGKeYM0fHGX+GvbrvFkrr8kx6X4uEVHoXiLXOrWOor0TxuJMlkEJ6DByexhHjsrSTYLnWRGiWocYG1NyBacUR0kbyziVrKeEU5wNWajT1x/PLq++zDiZLfBqp6OH5kENB97KFuBU1JrfM5PcYzHunlXZ/ZHX20RnB5P5YUmZi17QFnu1jOwK+u1bgQ9ZQ+WKIerUofYZXzO2SY8CL42313mx8xO4pF1oWapDHKgKKheE+3P5W/H6MAx8gotWGe3yINsG8dpdCDGXsCLaBMBOFfJgTTET+Aq5YtILFas11bs961TIzl7ly2X5hsyFgrOXi33Sj+VIXjZpN5cjnUk7a1D/PHKXu7YsIlWUrWGgH/5vZimDONyOJAZLb/0WRa5AA2/2EHZnWv9NRxSE7UmwcoWng78JPHzCcv3RrpVrrH6d7o/gYCwq66P/81NkKeD9PvlPTh9FZkC+KfRwj/RsMNnhQW/Ieaxz6VzdVp7Vcykwt3xAXA9APWiFLCEYopWTupNeBRX+vbQ8IvI8U847MSFcKHLjGWDsx7Vw42RosyOBxz7lFEN02fupBRoqgngzzWv5GR1szBpmVKt9iHj7chzYXOBFigKM+Bu4VMQu0Z0dvYHkj6zDzcXpt2atJj0YbDygFTUyiY77qePy/KE 9ZXim39G ClSvRCbq9xq66CZJcBTv/nlnILiBnCWHtJrtI/rVFbC79Y/hnKcqET67WRC9bDTZZYSS2cI2oaCh28iwJqVSpuoYTkByC1C/05ZpNQMuOXgjGMZOnc4o0/5QIVk9IhvN2jZZvyG9LkBAJREFgfubvGnO+X/IQ6zQa61iKkPRb568WHZ2YKobEXBJ5/DvEkFtEEnJe3F9t1wr4RgTk5dYZMLxDfdKBl1zDG8/fF/fY/XbiiUq99sicF0LFhlkwT1rsLH5NmINuqmCQrjaW3GOLhmZNtWrmmpy+gzFYYYjWOp43zFlEmq3O3HgXnVNolCAv2DMVD5ieljcyGvE= 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: List-Subscribe: List-Unsubscribe: On 18/03/2024 15:05, Christian König wrote: > Am 18.03.24 um 15:24 schrieb Maíra Canal: >> Not that the CC list wasn't big enough, but I'm adding MM folks >> in the CC list. >> >> On 3/18/24 11:04, Christian König wrote: >>> Am 18.03.24 um 14:28 schrieb Maíra Canal: >>>> Hi Christian, >>>> >>>> On 3/18/24 10:10, Christian König wrote: >>>>> Am 18.03.24 um 13:42 schrieb Maíra Canal: >>>>>> Hi Christian, >>>>>> >>>>>> On 3/12/24 10:48, Christian König wrote: >>>>>>> Am 12.03.24 um 14:09 schrieb Tvrtko Ursulin: >>>>>>>> >>>>>>>> On 12/03/2024 10:37, Christian König wrote: >>>>>>>>> Am 12.03.24 um 11:31 schrieb Tvrtko Ursulin: >>>>>>>>>> >>>>>>>>>> On 12/03/2024 10:23, Christian König wrote: >>>>>>>>>>> Am 12.03.24 um 10:30 schrieb Tvrtko Ursulin: >>>>>>>>>>>> >>>>>>>>>>>> On 12/03/2024 08:59, Christian König wrote: >>>>>>>>>>>>> Am 12.03.24 um 09:51 schrieb Tvrtko Ursulin: >>>>>>>>>>>>>> >>>>>>>>>>>>>> Hi Maira, >>>>>>>>>>>>>> >>>>>>>>>>>>>> On 11/03/2024 10:05, Maíra Canal wrote: >>>>>>>>>>>>>>> For some applications, such as using huge pages, we might >>>>>>>>>>>>>>> want to have a >>>>>>>>>>>>>>> different mountpoint, for which we pass in mount flags >>>>>>>>>>>>>>> that better match >>>>>>>>>>>>>>> our usecase. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Therefore, add a new parameter to drm_gem_object_init() >>>>>>>>>>>>>>> that allow us to >>>>>>>>>>>>>>> define the tmpfs mountpoint where the GEM object will be >>>>>>>>>>>>>>> created. If >>>>>>>>>>>>>>> this parameter is NULL, then we fallback to >>>>>>>>>>>>>>> shmem_file_setup(). >>>>>>>>>>>>>> >>>>>>>>>>>>>> One strategy for reducing churn, and so the number of >>>>>>>>>>>>>> drivers this patch touches, could be to add a lower level >>>>>>>>>>>>>> drm_gem_object_init() (which takes vfsmount, call it >>>>>>>>>>>>>> __drm_gem_object_init(), or drm__gem_object_init_mnt(), >>>>>>>>>>>>>> and make drm_gem_object_init() call that one with a NULL >>>>>>>>>>>>>> argument. >>>>>>>>>>>>> >>>>>>>>>>>>> I would even go a step further into the other direction. >>>>>>>>>>>>> The shmem backed GEM object is just some special handling >>>>>>>>>>>>> as far as I can see. >>>>>>>>>>>>> >>>>>>>>>>>>> So I would rather suggest to rename all drm_gem_* function >>>>>>>>>>>>> which only deal with the shmem backed GEM object into >>>>>>>>>>>>> drm_gem_shmem_*. >>>>>>>>>>>> >>>>>>>>>>>> That makes sense although it would be very churny. I at >>>>>>>>>>>> least would be on the fence regarding the cost vs benefit. >>>>>>>>>>> >>>>>>>>>>> Yeah, it should clearly not be part of this patch here. >>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>>> Also the explanation why a different mount point helps with >>>>>>>>>>>>> something isn't very satisfying. >>>>>>>>>>>> >>>>>>>>>>>> Not satisfying as you think it is not detailed enough to say >>>>>>>>>>>> driver wants to use huge pages for performance? Or not >>>>>>>>>>>> satisying as you question why huge pages would help? >>>>>>>>>>> >>>>>>>>>>> That huge pages are beneficial is clear to me, but I'm >>>>>>>>>>> missing the connection why a different mount point helps with >>>>>>>>>>> using huge pages. >>>>>>>>>> >>>>>>>>>> Ah right, same as in i915, one needs to mount a tmpfs instance >>>>>>>>>> passing huge=within_size or huge=always option. Default is >>>>>>>>>> 'never', see man 5 tmpfs. >>>>>>>>> >>>>>>>>> Thanks for the explanation, I wasn't aware of that. >>>>>>>>> >>>>>>>>> Mhm, shouldn't we always use huge pages? Is there a reason for >>>>>>>>> a DRM device to not use huge pages with the shmem backend? >>>>>>>> >>>>>>>> AFAIU, according to b901bb89324a ("drm/i915/gemfs: enable THP"), >>>>>>>> back then the understanding was within_size may overallocate, >>>>>>>> meaning there would be some space wastage, until the memory >>>>>>>> pressure makes the thp code split the trailing huge page. I >>>>>>>> haven't checked if that still applies. >>>>>>>> >>>>>>>> Other than that I don't know if some drivers/platforms could >>>>>>>> have problems if they have some limitations or hardcoded >>>>>>>> assumptions when they iterate the sg list. >>>>>>> >>>>>>> Yeah, that was the whole point behind my question. As far as I >>>>>>> can see this isn't driver specific, but platform specific. >>>>>>> >>>>>>> I might be wrong here, but I think we should then probably not >>>>>>> have that handling in each individual driver, but rather >>>>>>> centralized in the DRM code. >>>>>> >>>>>> I don't see a point in enabling THP for all shmem drivers. A huge >>>>>> page >>>>>> is only useful if the driver is going to use it. On V3D, for example, >>>>>> I only need huge pages because I need the memory contiguously >>>>>> allocated >>>>>> to implement Super Pages. Otherwise, if we don't have the Super Pages >>>>>> support implemented in the driver, I would be creating memory >>>>>> pressure >>>>>> without any performance gain. >>>>> >>>>> Well that's the point I'm disagreeing with. THP doesn't seem to >>>>> create much extra memory pressure for this use case. >>>>> >>>>> As far as I can see background for the option is that files in >>>>> tmpfs usually have a varying size, so it usually isn't beneficial >>>>> to allocate a huge page just to find that the shmem file is much >>>>> smaller than what's needed. >>>>> >>>>> But GEM objects have a fixed size. So we of hand knew if we need >>>>> 4KiB or 1GiB and can therefore directly allocate huge pages if they >>>>> are available and object large enough to back them with. >>>>> >>>>> If the memory pressure is so high that we don't have huge pages >>>>> available the shmem code falls back to standard pages anyway. >>>> >>>> The matter is: how do we define the point where the memory pressure >>>> is high? >>> >>> Well as driver developers/maintainers we simply don't do that. This >>> is the job of the shmem code. >>> >>>> For example, notice that in this implementation of Super Pages >>>> for the V3D driver, I only use a Super Page if the BO is bigger than >>>> 2MB. I'm doing that because the Raspberry Pi only has 4GB of RAM >>>> available for the GPU. If I created huge pages for every BO >>>> allocation (and initially, I tried that), I would end up with hangs >>>> in some applications. >>> >>> Yeah, that is what I meant with the trivial optimisation to the shmem >>> code. Essentially when you have 2MiB+4KiB as BO size then the shmem >>> code should use a 2MiB and a 4KiB page to back them, but what it >>> currently does is to use two 2MiB pages and then split up the second >>> page when it find that it isn't needed. >>> >>> That is wasteful and leads to fragmentation, but as soon as we stop >>> doing that we should be fine to enable it unconditionally for all >>> drivers. >> >> I see your point, but I believe that it would be tangent to the goal of >> this series. As you mentioned, currently, we have a lot of memory >> fragmentation when using THP and while we don't solve that (at the tmpfs >> level), I believe we could move on with the current implementation (with >> improvements proposed by Tvrtko). > > Oh, I seriously don't want to block this patch set here. Just asking if > it's the right approach. > > Point is we might need to revert the driver changes again when THP is > further optimized and the options aren't needed any more. > > But if and how that should happen is perfectly up to Tvrtko. Seem I got some un-intended voting powers here. :) What I think would work best is, if Maíra solves the v3d part first and then she can propose/float the wider DRM/GEM change to always use THP. Because that one will require some wider testing and acks. My concern there will be the behaviour of within_size mode. I was reading 465c403cb508 (drm/i915: introduce simple gemfs) the other day which made my think even then THP would initially over allocate. Or maybe the comment added in that commit wasn't fully accurate. If within_size is not high impact like that, then it GEM wide change might be feasible. With some opt-out facility or not I don't know. In any case, as long as the v3d work can be done with not too much churn to common code, I think separating that as follow up should not cause a lot of additional churn. Should all be hidden in the implementation details if all goes to plan. Regards, Tvrtko