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=-8.6 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=unavailable 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 0F0B9C4743E for ; Sat, 5 Jun 2021 06:30:48 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id B27766120A for ; Sat, 5 Jun 2021 06:30:47 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B27766120A Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=infradead.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 4379F6B0036; Sat, 5 Jun 2021 02:30:47 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 3E8276B006C; Sat, 5 Jun 2021 02:30:47 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2B0416B006E; Sat, 5 Jun 2021 02:30:47 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0155.hostedemail.com [216.40.44.155]) by kanga.kvack.org (Postfix) with ESMTP id EF8836B0036 for ; Sat, 5 Jun 2021 02:30:46 -0400 (EDT) Received: from smtpin09.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id 90371824999B for ; Sat, 5 Jun 2021 06:30:46 +0000 (UTC) X-FDA: 78218696892.09.C6CF893 Received: from casper.infradead.org (casper.infradead.org [90.155.50.34]) by imf30.hostedemail.com (Postfix) with ESMTP id B7D63E000AC4 for ; Sat, 5 Jun 2021 06:30:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=t1o5jsPuj2bB+x0v0RUVBeVs8O94UkaRJZY+bRal2wE=; b=N30XP8vw531RzbUTEevaTgcCB8 eWi047CVUkbEVL+xY1VH/Bs8bUv0IfWJPW7Q+K9AbKl6aDx3/wq3xxw6QhlZvd9IXM9Qz9VKU4A9+ LAgSXAupsfxhTEefruKrmODaJpOvj5k9o5Ie7BrlRf6pZYsTk+S+euwHoM2yPCCAFL8c+DEGilo8U hUtxoqrqebBUuIy+uBSN/lPKtGblDN/EYHeEIVD2ZAFeNFJH+LJH1vDNBHuNGO4ckXcYIIi7udS1n OWlBSwCb3Trf4qdtoWtY8WvY+u/ZRQUGxTcIz+cmDIKdZpNTutXB3wRzOQ1Da4QLjqjOgjn2qdRoA w6qZqqUQ==; Received: from willy by casper.infradead.org with local (Exim 4.94 #2 (Red Hat Linux)) id 1lpNtj-00Dpag-C1; Sat, 05 Jun 2021 04:27:05 +0000 Date: Sat, 5 Jun 2021 05:26:59 +0100 From: Matthew Wilcox To: Christoph Hellwig Cc: akpm@linux-foundation.org, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v10 08/33] mm: Add folio_try_get_rcu Message-ID: References: <20210511214735.1836149-1-willy@infradead.org> <20210511214735.1836149-9-willy@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: B7D63E000AC4 X-Stat-Signature: it7yqpk89ydyxjphypwyoe6xbkraqw98 Authentication-Results: imf30.hostedemail.com; dkim=pass header.d=infradead.org header.s=casper.20170209 header.b=N30XP8vw; dmarc=none; spf=none (imf30.hostedemail.com: domain of willy@infradead.org has no SPF policy when checking 90.155.50.34) smtp.mailfrom=willy@infradead.org X-HE-Tag: 1622874608-590942 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 Thu, May 27, 2021 at 09:16:42AM +0100, Christoph Hellwig wrote: > On Tue, May 11, 2021 at 10:47:10PM +0100, Matthew Wilcox (Oracle) wrote: > > +static inline bool folio_ref_try_add_rcu(struct folio *folio, int count) > > Should this have a __ prefix and/or a don't use direct comment? I think it will get used directly ... its page counterpart is: mm/gup.c: if (unlikely(!page_cache_add_speculative(head, refs))) I deliberately left kernel-doc off this function so it's not described, but described folio_try_get_rcu() in excruciating detail. I hope that's enough. There's no comment on page_cache_add_speculative() today, so again, we're status quo. > > +{ > > +#ifdef CONFIG_TINY_RCU > > + /* > > + * The caller guarantees the folio will not be freed from interrupt > > + * context, so (on !SMP) we only need preemption to be disabled > > + * and TINY_RCU does that for us. > > + */ > > +# ifdef CONFIG_PREEMPT_COUNT > > + VM_BUG_ON(!in_atomic() && !irqs_disabled()); > > +# endif > > VM_BUG_ON(IS_ENABLED(CONFIG_PREEMPT_COUNT) && > !in_atomic() && !irqs_disabled()); > > ? I'm just moving it over, and honestly, I think it's slightly clearer this way. We can't check it if PREEMPT_COUNT isn't enabled, and I think that's expressed better by the ifdef than the IS_ENABLED(). > > + VM_BUG_ON_FOLIO(folio_ref_count(folio) == 0, folio); > > + folio_ref_add(folio, count); > > +#else > > + if (unlikely(!folio_ref_add_unless(folio, count, 0))) { > > + /* Either the folio has been freed, or will be freed. */ > > + return false; > > + } > > +#endif > > + return true; > > but is this tiny rcu optimization really worth it? I guess we're just > preserving it from the existing code and don't rock the boat.. I wondered that myself. It's been there since Nick introduced it in 2008 with commit e286781d5f2e. We certainly cared about small systems more then, but apparently we still care about UP enough to maintain CONFIG_TINY_RCU, so maybe this optimisation is still relevant. > > @@ -1746,6 +1746,26 @@ pgoff_t page_cache_prev_miss(struct address_space *mapping, > > } > > EXPORT_SYMBOL(page_cache_prev_miss); > > > > +/* > > + * Lockless page cache protocol: > > + * On the lookup side: > > + * 1. Load the folio from i_pages > > + * 2. Increment the refcount if it's not zero > > + * 3. If the folio is not found by xas_reload(), put the refcount and retry > > + * > > + * On the removal side: > > + * A. Freeze the page (by zeroing the refcount if nobody else has a reference) > > + * B. Remove the page from i_pages > > + * C. Return the page to the page allocator > > + * > > + * This means that any page may have its reference count temporarily > > + * increased by a speculative page cache (or fast GUP) lookup as it can > > + * be allocated by another user before the RCU grace period expires. > > + * Because the refcount temporarily acquired here may end up being the > > + * last refcount on the page, any page allocation must be freeable by > > + * put_folio(). > > + */ > > + > > /* > > * mapping_get_entry - Get a page cache entry. > > * @mapping: the address_space to search > > Is this really a good place for the comment? I'd expect it either near > a relevant function or at the top of a file. It's right before mapping_get_entry() which is the main lookup function for the page cache, so I think it meets your first criteria?