LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Peter Zijlstra <a.p.zijlstra@chello.nl>
To: Andrew Morton <akpm@osdl.org>
Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	Ingo Molnar <mingo@elte.hu>
Subject: Re: [PATCH] mm: remove global locks from mm/highmem.c
Date: Mon, 29 Jan 2007 10:44:08 +0100	[thread overview]
Message-ID: <1170063848.6189.121.camel@twins> (raw)
In-Reply-To: <20070128142925.df2f4dce.akpm@osdl.org>

On Sun, 2007-01-28 at 14:29 -0800, Andrew Morton wrote:

> As Christoph says, it's very much preferred that code be migrated over to
> kmap_atomic().  Partly because kmap() is deadlockable in situations where a
> large number of threads are trying to take two kmaps at the same time and
> we run out.  This happened in the past, but incidences have gone away,
> probably because of kmap->kmap_atomic conversions.

> From which callsite have you measured problems?

CONFIG_HIGHPTE code in -rt was horrid. I'll do some measurements on
mainline.

> > Index: linux/include/linux/mm.h
> > ===================================================================
> > --- linux.orig/include/linux/mm.h
> > +++ linux/include/linux/mm.h
> > @@ -543,23 +543,39 @@ static __always_inline void *lowmem_page
> >  #endif
> >  
> >  #if defined(WANT_PAGE_VIRTUAL)
> > -#define page_address(page) ((page)->virtual)
> > -#define set_page_address(page, address)			\
> > -	do {						\
> > -		(page)->virtual = (address);		\
> > -	} while(0)
> > -#define page_address_init()  do { } while(0)
> > +/*
> > + * wrap page->virtual so it is safe to set/read locklessly
> > + */
> > +#define page_address(page) \
> > +	({ typeof((page)->virtual) v = (page)->virtual; \
> > +	 smp_read_barrier_depends(); \
> > +	 v; })
> > +
> > +static inline int set_page_address(struct page *page, void *address)
> > +{
> > +	if (address)
> > +		return cmpxchg(&page->virtual, NULL, address) == NULL;
> > +	else {
> > +		/*
> > +		 * cmpxchg is a bit abused because it is not guaranteed
> > +		 * safe wrt direct assignment on all platforms.
> > +		 */
> > +		void *virt = page->virtual;
> > +		return cmpxchg(&page->vitrual, virt, NULL) == virt;
> > +	}
> > +}
> 
> Have you verified that all architectures which can implement
> WANT_PAGE_VIRTUAL also implement cmpxchg?

It might have been my mistaken in understanding the latest cmpxchg
thread. My understanding was that since LL/SC is not exposable as a low
level primitive all platforms should implement a cmpxchg where some
would not be save against direct assignment.

Anyway, I'll do as Nick says and replace it with atomic_long_cmpxchg.

> Have you verified that sufficient headers are included for this to compile
> correctly on all WANT_PAGE_VIRTUAL-enabling architectures on all configs? 
> I don't see asm/system.h being included in mm.h and if I get yet another
> damned it-wont-compile patch I might do something irreversible.

Point taken.

> > +static int pkmap_get_free(void)
> >  {
> > -	unsigned long vaddr;
> > -	int count;
> > +	int i, pos, flush;
> > +	DECLARE_WAITQUEUE(wait, current);
> >  
> > -start:
> > -	count = LAST_PKMAP;
> > -	/* Find an empty entry */
> > -	for (;;) {
> > -		last_pkmap_nr = (last_pkmap_nr + 1) & LAST_PKMAP_MASK;
> 
> The old code used masking.
> 
> > -		if (!last_pkmap_nr) {
> > -			flush_all_zero_pkmaps();
> > -			count = LAST_PKMAP;
> > -		}
> > -		if (!pkmap_count[last_pkmap_nr])
> > -			break;	/* Found a usable entry */
> > -		if (--count)
> > -			continue;
> > +restart:
> > +	for (i = 0; i < LAST_PKMAP; i++) {
> > +		pos = atomic_inc_return(&pkmap_hand) % LAST_PKMAP;
> 
> The new code does more-expensive modulus.  Necessary?

I thought GCC would automagically use masking when presented with a
power-of-two constant. Can make it more explicit though.

> > +		flush = pkmap_try_free(pos);
> > +		if (flush >= 0)
> > +			goto got_one;
> > +	}
> > +
> > +	/*
> > +	 * wait for somebody else to unmap their entries
> > +	 */
> > +	__set_current_state(TASK_UNINTERRUPTIBLE);
> > +	add_wait_queue(&pkmap_map_wait, &wait);
> > +	schedule();
> > +	remove_wait_queue(&pkmap_map_wait, &wait);
> 
> This looks wrong.  What happens if everyone else does their unmap between
> the __set_current_state() and the add_wait_queue()?

Eek, you are quite right.


  parent reply	other threads:[~2007-01-29  9:45 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-01-28 14:11 Peter Zijlstra
2007-01-28 14:49 ` Christoph Hellwig
2007-01-28 15:17   ` Ingo Molnar
2007-01-28 15:28     ` Christoph Hellwig
2007-01-28 15:48       ` Ingo Molnar
2007-01-28 15:54         ` Christoph Hellwig
2007-01-28 18:19           ` Ingo Molnar
2007-01-28 22:29 ` Andrew Morton
2007-01-29  2:52   ` Nick Piggin
2007-01-29  9:44   ` Peter Zijlstra [this message]
2007-01-30  1:31     ` Martin J. Bligh
2007-01-30  1:41       ` Andrew Morton
2007-01-30  1:49         ` Martin J. Bligh
2007-01-30  2:15           ` Andrew Morton
2007-01-31  0:44             ` David Chinner
2007-01-31  1:11               ` Andrew Morton
2007-01-31  3:22                 ` David Chinner
2007-02-02 12:05                   ` Christoph Hellwig
2007-02-02 19:24                     ` Matt Mackall
2007-02-02 23:16                       ` David Chinner
2007-02-02 23:14                     ` David Chinner
2007-01-29 19:08   ` Ingo Molnar
2007-01-29 19:19     ` Hugh Dickins
2007-01-29 19:53       ` Ingo Molnar
2007-01-29 20:06     ` Ingo Molnar
2007-01-30  2:02     ` Nick Piggin

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=1170063848.6189.121.camel@twins \
    --to=a.p.zijlstra@chello.nl \
    --cc=akpm@osdl.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mingo@elte.hu \
    --subject='Re: [PATCH] mm: remove global locks from mm/highmem.c' \
    /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
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).