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 844A3C4829B for ; Mon, 12 Feb 2024 11:22:01 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 1595C6B0074; Mon, 12 Feb 2024 06:22:01 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 109A96B0078; Mon, 12 Feb 2024 06:22:01 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id F131C6B007B; Mon, 12 Feb 2024 06:22:00 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id E27246B0074 for ; Mon, 12 Feb 2024 06:22:00 -0500 (EST) Received: from smtpin04.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id A7484401BD for ; Mon, 12 Feb 2024 11:22:00 +0000 (UTC) X-FDA: 81782912400.04.200A99A Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf21.hostedemail.com (Postfix) with ESMTP id 64B521C0005 for ; Mon, 12 Feb 2024 11:21:58 +0000 (UTC) Authentication-Results: imf21.hostedemail.com; dkim=none; spf=pass (imf21.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com; dmarc=pass (policy=none) header.from=arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1707736919; 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; bh=A4ICoNITsJCxxmS/Acu3X9Vq8EB4weeb/vpETTBT6kI=; b=FCJgkXZy9mBlKb/nusVV1ovsQUkWmoyqwHIUIQLv/s3YjbdK2eaBfrYX2sC596UfKBfPp6 FV4EHja/rKcGaJHoh8OyblidaggowF1Ayr34l/tfL1pcUDroE1HaEPzKKaO6qXeZe9Eg0q 74brQLzkHUaF+lS88oyC7f189rn/v9o= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1707736919; a=rsa-sha256; cv=none; b=6OE+NhQS7lPmqDyXGqhDaVyyyvWR+HiALjh+EGfEpMigI9Go0bG3zLMUcY+eUtmUkTqJSb trs+IKTRUSwNAdhjCGG2ynEznWEdmzzwVZIn985RStuHaehkLk6uQfMskFa1ITZhiyNEi+ f4LYWKKAU3x5ZwHQMzAh6kjFF9oc+KY= ARC-Authentication-Results: i=1; imf21.hostedemail.com; dkim=none; spf=pass (imf21.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com; dmarc=pass (policy=none) header.from=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 9C9AFDA7; Mon, 12 Feb 2024 03:22:38 -0800 (PST) Received: from [10.57.78.115] (unknown [10.57.78.115]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id E5E683F762; Mon, 12 Feb 2024 03:21:53 -0800 (PST) Message-ID: <398991e6-d09d-4f47-a110-4ff1e8356b6e@arm.com> Date: Mon, 12 Feb 2024 11:21:52 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 09/10] mm/mmu_gather: improve cond_resched() handling with large folios and expensive page freeing Content-Language: en-GB To: David Hildenbrand , linux-kernel@vger.kernel.org Cc: linux-mm@kvack.org, Andrew Morton , Matthew Wilcox , Catalin Marinas , Yin Fengwei , Michal Hocko , Will Deacon , "Aneesh Kumar K.V" , Nick Piggin , Peter Zijlstra , Michael Ellerman , Christophe Leroy , "Naveen N. Rao" , Heiko Carstens , Vasily Gorbik , Alexander Gordeev , Christian Borntraeger , Sven Schnelle , Arnd Bergmann , linux-arch@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux-s390@vger.kernel.org References: <20240209221509.585251-1-david@redhat.com> <20240209221509.585251-10-david@redhat.com> <6c66f7ca-4b14-4bbb-bf06-e81b3481b03f@redhat.com> <590946ad-a538-4c99-947f-93455c2d96c6@arm.com> <66ca6c58-1983-494f-b920-140be736f1d8@redhat.com> From: Ryan Roberts In-Reply-To: <66ca6c58-1983-494f-b920-140be736f1d8@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: 64B521C0005 X-Rspam-User: X-Stat-Signature: yextdsbfsdiqskimtyn49jde5bqhbxee X-Rspamd-Server: rspam03 X-HE-Tag: 1707736918-952490 X-HE-Meta: U2FsdGVkX19Fvj+pu6IwoG4+NoSJmujdz5S2Od/A0QWhitTdQTVSVmg/WEWvAyAF9rC6p7aXcOl7bMBhe/1rlO1Mjfs+LmeKDIkqcj9Fd1/pCaGbH9WaQJvsyg/wMt481tgQOpE8ITwwealOCGj9Ct20gejEeYjEa0BCclNO69Bq/yyAoA2xsMIGCYtjWAC2DaVKmg3oGD0lIs3eR78PYWVQW7z/YedLfJ3LEdvSC3Dsw2v6NHt+mQ3ELjHSKdx9K4m0TRcD1nOUKZ2+gklSUSSMNTIBIVm2ky3gWQVleOUew+ItvKVror3deGXRAtXTlyJelj+wEGLQszw2sJvt5Uj8QBmveZl04Gz+/1UlGxnQkfzNwJJLOeArat+h3/7Frv+DgxXb8Ta7REQqtLKlY3ZH4XIrn/9htk8gyg2llpb1GpDHWmWKkwRb1JBztY7QaZlMSs8JkP6+L8H9BTAK2/G61GVPmjDuBQXElwFtLQj3uCjXuZt+vbyheFeoXdh54kSWHFBHtsnl3LoEco/ix4iTd37JLapiNwIZJP33qyrfNN3CICWgSZhh6JMOqjmz05YZkRsRHZgz19drEJYgbjkGC7bqMsBJKCG5pSVXMSkW7tWIFdxD/O2JTRuAgXzBs9R23vJO9Zp9Cx6YMVu/oNohOTORcCiYhUgBFh49M0JBwBKzl/ln74Fb1//Qhpcx97kKj7ao4BRbWmZfmBi1zre4c0KKTv0YH0YqJ+VDQt/MNaW7aCZhxZXnMc9BlPLMAo58XLXOSNTc9lQ2sqcPnBvvCCks8B1TZ5q5xhfBj040uVtw5wmfYAACzIXYUjzdHeTYPQ9X/3+4YnNTsIxQzuOFLdOKcbiBl2lVflh9VB9+C9CMOWRfboWnoPcRujyERekQyr+x3QpOlyE+pWDDO5izloX5ZvLAblztmm64z+rxSUi8/cHTUsUEK9eHShyy0m3BwpQRcIuaD0gGdfA ecvV+i0g 5GVUgyfWTxPQ48QdNHNz0kLEFaqqrg939izYGmBYMBFNrwZwUhuAnuhHIPn2qDUCJS71OcOCgFQ5W3IFYVnY1ti8kFyFBlxgNMVeEwSspgR9YjA/5jdmh409x69trBRfewRLedyESaWKvvKQ9Iy3mPBZCjBQMOo3d9x2Dm5Vsc+rhH9YVmnEe8hQ4dW4qQWah4oQ4tcJIW8nq8KISesxVI/3FGfD1vA4RKlv1vR6nDi5jD02c9DOZOQv1kVNBcgv3o1aSAacTW2PQZIpmFDbvuMt60A== 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 12/02/2024 11:05, David Hildenbrand wrote: > On 12.02.24 11:56, David Hildenbrand wrote: >> On 12.02.24 11:32, Ryan Roberts wrote: >>> On 12/02/2024 10:11, David Hildenbrand wrote: >>>> Hi Ryan, >>>> >>>>>> -static void tlb_batch_pages_flush(struct mmu_gather *tlb) >>>>>> +static void __tlb_batch_free_encoded_pages(struct mmu_gather_batch *batch) >>>>>>     { >>>>>> -    struct mmu_gather_batch *batch; >>>>>> - >>>>>> -    for (batch = &tlb->local; batch && batch->nr; batch = batch->next) { >>>>>> -        struct encoded_page **pages = batch->encoded_pages; >>>>>> +    struct encoded_page **pages = batch->encoded_pages; >>>>>> +    unsigned int nr, nr_pages; >>>>>>     +    /* >>>>>> +     * We might end up freeing a lot of pages. Reschedule on a regular >>>>>> +     * basis to avoid soft lockups in configurations without full >>>>>> +     * preemption enabled. The magic number of 512 folios seems to work. >>>>>> +     */ >>>>>> +    if (!page_poisoning_enabled_static() && !want_init_on_free()) { >>>>> >>>>> Is the performance win really worth 2 separate implementations keyed off this? >>>>> It seems a bit fragile, in case any other operations get added to free >>>>> which are >>>>> proportional to size in future. Why not just always do the conservative >>>>> version? >>>> >>>> I really don't want to iterate over all entries on the "sane" common case. We >>>> already do that two times: >>>> >>>> a) free_pages_and_swap_cache() >>>> >>>> b) release_pages() >>>> >>>> Only the latter really is required, and I'm planning on removing the one in (a) >>>> to move it into (b) as well. >>>> >>>> So I keep it separate to keep any unnecessary overhead to the setups that are >>>> already terribly slow. >>>> >>>> No need to iterate a page full of entries if it can be easily avoided. >>>> Especially, no need to degrade the common order-0 case. >>> >>> Yeah, I understand all that. But given this is all coming from an array, (so >>> easy to prefetch?) and will presumably all fit in the cache for the common case, >>> at least, so its hot for (a) and (b), does separating this out really make a >>> measurable performance difference? If yes then absolutely this optimizaiton >>> makes sense. But if not, I think its a bit questionable. >> >> I primarily added it because >> >> (a) we learned that each cycle counts during mmap() just like it does >> during fork(). >> >> (b) Linus was similarly concerned about optimizing out another batching >> walk in c47454823bd4 ("mm: mmu_gather: allow more than one batch of >> delayed rmaps"): >> >> "it needs to walk that array of pages while still holding the page table >> lock, and our mmu_gather infrastructure allows for batching quite a lot >> of pages.  We may have thousands on pages queued up for freeing, and we >> wanted to walk only the last batch if we then added a dirty page to the >> queue." >> >> So if it matters enough for reducing the time we hold the page table >> lock, it surely adds "some" overhead in general. >> >> >>> >>> You're the boss though, so if your experience tells you this is neccessary, then >>> I'm ok with that. >> >> I did not do any measurements myself, I just did that intuitively as >> above. After all, it's all pretty straight forward (keeping the existing >> logic, we need a new one either way) and not that much code. >> >> So unless there are strong opinions, I'd just leave the common case as >> it was, and the odd case be special. > > I think we can just reduce the code duplication easily: > > diff --git a/mm/mmu_gather.c b/mm/mmu_gather.c > index d175c0f1e2c8..99b3e9408aa0 100644 > --- a/mm/mmu_gather.c > +++ b/mm/mmu_gather.c > @@ -91,18 +91,21 @@ void tlb_flush_rmaps(struct mmu_gather *tlb, struct > vm_area_struct *vma) >  } >  #endif >   > -static void tlb_batch_pages_flush(struct mmu_gather *tlb) > -{ > -    struct mmu_gather_batch *batch; > +/* > + * We might end up freeing a lot of pages. Reschedule on a regular > + * basis to avoid soft lockups in configurations without full > + * preemption enabled. The magic number of 512 folios seems to work. > + */ > +#define MAX_NR_FOLIOS_PER_FREE        512 >   > -    for (batch = &tlb->local; batch && batch->nr; batch = batch->next) { > -        struct encoded_page **pages = batch->encoded_pages; > +static void __tlb_batch_free_encoded_pages(struct mmu_gather_batch *batch) > +{ > +    struct encoded_page **pages = batch->encoded_pages; > +    unsigned int nr, nr_pages; >   > -        while (batch->nr) { > -            /* > -             * limit free batch count when PAGE_SIZE > 4K > -             */ > -            unsigned int nr = min(512U, batch->nr); > +    while (batch->nr) { > +        if (!page_poisoning_enabled_static() && !want_init_on_free()) { > +            nr = min(MAX_NR_FOLIOS_PER_FREE, batch->nr); >   >              /* >               * Make sure we cover page + nr_pages, and don't leave > @@ -111,14 +114,39 @@ static void tlb_batch_pages_flush(struct mmu_gather *tlb) >              if (unlikely(encoded_page_flags(pages[nr - 1]) & >                       ENCODED_PAGE_BIT_NR_PAGES_NEXT)) >                  nr++; > +        } else { > +            /* > +             * With page poisoning and init_on_free, the time it > +             * takes to free memory grows proportionally with the > +             * actual memory size. Therefore, limit based on the > +             * actual memory size and not the number of involved > +             * folios. > +             */ > +            for (nr = 0, nr_pages = 0; > +                 nr < batch->nr && nr_pages < MAX_NR_FOLIOS_PER_FREE; > +                 nr++) { > +                if (unlikely(encoded_page_flags(pages[nr]) & > +                         ENCODED_PAGE_BIT_NR_PAGES_NEXT)) > +                    nr_pages += encoded_nr_pages(pages[++nr]); > +                else > +                    nr_pages++; > +            } > +        } >   > -            free_pages_and_swap_cache(pages, nr); > -            pages += nr; > -            batch->nr -= nr; > +        free_pages_and_swap_cache(pages, nr); > +        pages += nr; > +        batch->nr -= nr; >   > -            cond_resched(); > -        } > +        cond_resched(); >      } > +} > + > +static void tlb_batch_pages_flush(struct mmu_gather *tlb) > +{ > +    struct mmu_gather_batch *batch; > + > +    for (batch = &tlb->local; batch && batch->nr; batch = batch->next) > +        __tlb_batch_free_encoded_pages(batch); >      tlb->active = &tlb->local; >  } >   Yes this is much cleaner IMHO! I don't think putting the poison and init_on_free checks inside the while loops should make a whole lot of difference - you're only going round that loop once in the common (4K pages) case. Reviewed-by: Ryan Roberts