linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Matthew Wilcox <willy@infradead.org>
To: Christoph Hellwig <hch@infradead.org>
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
Date: Sat, 5 Jun 2021 05:26:59 +0100	[thread overview]
Message-ID: <YLr9E4tWle9Qa1Xy@casper.infradead.org> (raw)
In-Reply-To: <YK9VagEO+bKurYlz@infradead.org>

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?

  reply	other threads:[~2021-06-05  4:27 UTC|newest]

Thread overview: 96+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-11 21:47 [PATCH v10 00/33] Memory folios Matthew Wilcox (Oracle)
2021-05-11 21:47 ` [PATCH v10 01/33] mm: Introduce struct folio Matthew Wilcox (Oracle)
2021-05-14 10:34   ` Vlastimil Babka
2021-05-14 10:40   ` Vlastimil Babka
2021-05-14 11:47     ` Matthew Wilcox
2021-05-15 10:55   ` William Kucharski
2021-05-15 20:14     ` Matthew Wilcox
2021-05-16 19:26       ` William Kucharski
2021-05-27  8:09   ` Christoph Hellwig
2021-05-11 21:47 ` [PATCH v10 02/33] mm: Add folio_pgdat and folio_zone Matthew Wilcox (Oracle)
2021-05-14 10:35   ` Vlastimil Babka
2021-05-11 21:47 ` [PATCH v10 03/33] mm/vmstat: Add functions to account folio statistics Matthew Wilcox (Oracle)
2021-05-14 10:36   ` Vlastimil Babka
2021-05-11 21:47 ` [PATCH v10 04/33] mm/debug: Add VM_BUG_ON_FOLIO and VM_WARN_ON_ONCE_FOLIO Matthew Wilcox (Oracle)
2021-05-14 10:44   ` Vlastimil Babka
2021-05-11 21:47 ` [PATCH v10 05/33] mm: Add folio reference count functions Matthew Wilcox (Oracle)
2021-05-14 11:04   ` Vlastimil Babka
2021-05-11 21:47 ` [PATCH v10 06/33] mm: Add folio_put Matthew Wilcox (Oracle)
2021-05-14 11:52   ` Vlastimil Babka
2021-05-11 21:47 ` [PATCH v10 07/33] mm: Add folio_get Matthew Wilcox (Oracle)
2021-05-14 11:56   ` Vlastimil Babka
2021-05-14 14:24     ` Matthew Wilcox
2021-05-14 15:39       ` Vlastimil Babka
2021-05-27  8:10       ` Christoph Hellwig
2021-05-27 22:53         ` Andrew Morton
2021-05-11 21:47 ` [PATCH v10 08/33] mm: Add folio_try_get_rcu Matthew Wilcox (Oracle)
2021-05-14 12:11   ` Vlastimil Babka
2021-05-27  8:16   ` Christoph Hellwig
2021-06-05  4:26     ` Matthew Wilcox [this message]
2021-06-06 14:13       ` Christoph Hellwig
2021-05-11 21:47 ` [PATCH v10 09/33] mm: Add folio flag manipulation functions Matthew Wilcox (Oracle)
2021-05-14 15:29   ` Vlastimil Babka
2021-05-11 21:47 ` [PATCH v10 10/33] mm: Add folio_young and folio_idle Matthew Wilcox (Oracle)
2021-05-14 15:33   ` Vlastimil Babka
2021-05-27  8:17   ` Christoph Hellwig
2021-05-11 21:47 ` [PATCH v10 11/33] mm: Handle per-folio private data Matthew Wilcox (Oracle)
2021-05-14 15:41   ` Vlastimil Babka
2021-05-11 21:47 ` [PATCH v10 12/33] mm/filemap: Add folio_index, folio_file_page and folio_contains Matthew Wilcox (Oracle)
2021-05-14 15:55   ` Vlastimil Babka
2021-05-15 15:51     ` Matthew Wilcox
2021-05-11 21:47 ` [PATCH v10 13/33] mm/filemap: Add folio_next_index Matthew Wilcox (Oracle)
2021-05-14 17:07   ` Vlastimil Babka
2021-05-11 21:47 ` [PATCH v10 14/33] mm/filemap: Add folio_offset and folio_file_offset Matthew Wilcox (Oracle)
2021-05-14 17:08   ` Vlastimil Babka
2021-05-11 21:47 ` [PATCH v10 15/33] mm/util: Add folio_mapping and folio_file_mapping Matthew Wilcox (Oracle)
2021-05-14 17:29   ` Vlastimil Babka
2021-05-11 21:47 ` [PATCH v10 16/33] mm: Add folio_mapcount Matthew Wilcox (Oracle)
2021-05-14 17:39   ` Vlastimil Babka
2021-05-18 18:45   ` Matthew Wilcox
2021-05-11 21:47 ` [PATCH v10 17/33] mm/memcg: Add folio wrappers for various functions Matthew Wilcox (Oracle)
2021-05-18  9:57   ` Vlastimil Babka
2021-05-11 21:47 ` [PATCH v10 18/33] mm/filemap: Add folio_unlock Matthew Wilcox (Oracle)
2021-05-18 10:06   ` Vlastimil Babka
2021-05-18 11:30     ` Matthew Wilcox
2021-05-11 21:47 ` [PATCH v10 19/33] mm/filemap: Add folio_lock Matthew Wilcox (Oracle)
2021-05-18 10:26   ` Vlastimil Babka
2021-05-11 21:47 ` [PATCH v10 20/33] mm/filemap: Add folio_lock_killable Matthew Wilcox (Oracle)
2021-05-18 10:31   ` Vlastimil Babka
2021-05-11 21:47 ` [PATCH v10 21/33] mm/filemap: Add __folio_lock_async Matthew Wilcox (Oracle)
2021-05-18 10:34   ` Vlastimil Babka
2021-05-11 21:47 ` [PATCH v10 22/33] mm/filemap: Add __folio_lock_or_retry Matthew Wilcox (Oracle)
2021-05-18 10:38   ` Vlastimil Babka
2021-05-18 10:45     ` Vlastimil Babka
2021-05-18 13:35     ` Matthew Wilcox
2021-05-11 21:47 ` [PATCH v10 23/33] mm/filemap: Add folio_wait_locked Matthew Wilcox (Oracle)
2021-05-18 10:41   ` Vlastimil Babka
2021-05-11 21:47 ` [PATCH v10 24/33] mm/swap: Add folio_rotate_reclaimable Matthew Wilcox (Oracle)
2021-05-18 10:48   ` Vlastimil Babka
2021-05-27  8:19   ` Christoph Hellwig
2021-05-11 21:47 ` [PATCH v10 25/33] mm/filemap: Add folio_end_writeback Matthew Wilcox (Oracle)
2021-05-18 11:08   ` Vlastimil Babka
2021-05-11 21:47 ` [PATCH v10 26/33] mm/writeback: Add folio_wait_writeback Matthew Wilcox (Oracle)
2021-05-18 11:12   ` Vlastimil Babka
2021-05-11 21:47 ` [PATCH v10 27/33] mm/writeback: Add folio_wait_stable Matthew Wilcox (Oracle)
2021-05-18 11:42   ` Vlastimil Babka
2021-05-18 13:55     ` Matthew Wilcox
2021-05-11 21:47 ` [PATCH v10 28/33] mm/filemap: Add folio_wait_bit Matthew Wilcox (Oracle)
2021-05-18 11:51   ` Vlastimil Babka
2021-05-11 21:47 ` [PATCH v10 29/33] mm/filemap: Add folio_wake_bit Matthew Wilcox (Oracle)
2021-05-18 11:53   ` Vlastimil Babka
2021-05-11 21:47 ` [PATCH v10 30/33] mm/filemap: Convert page wait queues to be folios Matthew Wilcox (Oracle)
2021-05-18 12:23   ` Vlastimil Babka
2021-05-11 21:47 ` [PATCH v10 31/33] mm/filemap: Add folio private_2 functions Matthew Wilcox (Oracle)
2021-05-18 12:26   ` Vlastimil Babka
2021-05-27  8:21   ` Christoph Hellwig
2021-05-11 21:47 ` [PATCH v10 32/33] fs/netfs: Add folio fscache functions Matthew Wilcox (Oracle)
2021-05-18 13:48   ` Vlastimil Babka
2021-05-27  8:23   ` Christoph Hellwig
2021-05-11 21:47 ` [PATCH v10 33/33] mm: Add folio_mapped Matthew Wilcox (Oracle)
2021-05-18 14:17   ` Vlastimil Babka
2021-05-27  8:31   ` Christoph Hellwig
2021-05-13 14:50 ` [PATCH v10 00/33] Memory folios Matthew Wilcox
2021-05-15 10:26 ` William Kucharski
2021-06-04  1:07 ` Matteo Croce
2021-06-04  2:13   ` Matthew Wilcox
2021-06-08 14:56     ` Matteo Croce

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YLr9E4tWle9Qa1Xy@casper.infradead.org \
    --to=willy@infradead.org \
    --cc=akpm@linux-foundation.org \
    --cc=hch@infradead.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --subject='Re: [PATCH v10 08/33] mm: Add folio_try_get_rcu' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox