From: Shaohua Li <shli@kernel.org>
To: Hugh Dickins <hughd@google.com>
Cc: Sasha Levin <sasha.levin@oracle.com>,
Andrew Morton <akpm@linux-foundation.org>,
Shaohua Li <shli@fusionio.com>, Rik van Riel <riel@redhat.com>,
Minchan Kim <minchan@kernel.org>,
linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: boot warnings due to swap: make each swap partition have one address_space
Date: Wed, 30 Jan 2013 17:59:44 +0800 [thread overview]
Message-ID: <20130130095944.GA11457@kernel.org> (raw)
In-Reply-To: <alpine.LNX.2.00.1301271321500.16981@eggly.anvils>
On Sun, Jan 27, 2013 at 01:40:40PM -0800, Hugh Dickins wrote:
> On Sun, 27 Jan 2013, Shaohua Li wrote:
> > On Sat, Jan 26, 2013 at 06:16:05PM -0800, Hugh Dickins wrote:
> > > On Fri, 25 Jan 2013, Shaohua Li wrote:
> > > > On Thu, Jan 24, 2013 at 10:45:57PM -0500, Sasha Levin wrote:
> > > >
> > > > Subject: give-each-swapper-space-separate-backing_dev_info
> > > >
> > > > The backing_dev_info can't be shared by all swapper address space.
> > >
> > > Whyever not? It's perfectly normal for different inodes/address_spaces
> > > to share a single backing_dev! Sasha's trace says that it's wrong to
> > > initialize it MAX_SWAPFILES times: fair enough. But why should I now
> > > want to spend 32kB (not even counting their __percpu counters) on all
> > > these pseudo-backing_devs?
> >
> > That's correct, silly me. Updated it.
>
> Looks much more to my taste, thank you!
>
> > >
> > > p.s. a grand little change would be to move page_cluster and swap_setup()
> > > from mm/swap.c to mm/swap_state.c: they have nothing to do with the other
> > > contents of swap.c, and everything to do with the contents of swap_state.c.
> > > Why swap.c is called swap.c is rather a mystery.
> >
> > Tried, but looks page_cluster is used in sysctl, moving to swap_state.c will
> > make it optional. don't want to add another #ifdef, so give up.
>
> Good point, thanks for trying, maybe I'll attack it next time it
> irritates me.
>
> I don't yet know whether I approve of your changes or not, but running
> with them to see (and I'll send another bugfix separately in a moment).
>
> I was the one who removed the swap_device_lock() which 2.4 used,
> because it almost always ended up having to take both swap_list_lock()
> and swap_device_lock(si). You seem to have done a much better job of
> separating them usefully, but I need to convince myself that it does
> end up safely.
>
> My reservations so far would be: how many installations actually have
> more than one swap area, so is it a good tradeoff to add more overhead
> to help those at the (slight) expense of everyone else? The increasingly
> ugly page_mapping() worries me, and the static array of swapper_spaces
> annoys me a little.
>
> I'm glad Minchan has now pointed you to Rik's posting of two years ago:
> I think there are more important changes to be made in that direction.
Not sure how others use multiple swaps, but current lock contention forces us
to use multiple swaps. I haven't carefully think about Rik's posting, but looks
it doesn't solve the lock contention problem.
Thanks,
Shaohua
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
WARNING: multiple messages have this Message-ID (diff)
From: Shaohua Li <shli@kernel.org>
To: Hugh Dickins <hughd@google.com>
Cc: Sasha Levin <sasha.levin@oracle.com>,
Andrew Morton <akpm@linux-foundation.org>,
Shaohua Li <shli@fusionio.com>, Rik van Riel <riel@redhat.com>,
Minchan Kim <minchan@kernel.org>,
linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: boot warnings due to swap: make each swap partition have one address_space
Date: Wed, 30 Jan 2013 17:59:44 +0800 [thread overview]
Message-ID: <20130130095944.GA11457@kernel.org> (raw)
In-Reply-To: <alpine.LNX.2.00.1301271321500.16981@eggly.anvils>
On Sun, Jan 27, 2013 at 01:40:40PM -0800, Hugh Dickins wrote:
> On Sun, 27 Jan 2013, Shaohua Li wrote:
> > On Sat, Jan 26, 2013 at 06:16:05PM -0800, Hugh Dickins wrote:
> > > On Fri, 25 Jan 2013, Shaohua Li wrote:
> > > > On Thu, Jan 24, 2013 at 10:45:57PM -0500, Sasha Levin wrote:
> > > >
> > > > Subject: give-each-swapper-space-separate-backing_dev_info
> > > >
> > > > The backing_dev_info can't be shared by all swapper address space.
> > >
> > > Whyever not? It's perfectly normal for different inodes/address_spaces
> > > to share a single backing_dev! Sasha's trace says that it's wrong to
> > > initialize it MAX_SWAPFILES times: fair enough. But why should I now
> > > want to spend 32kB (not even counting their __percpu counters) on all
> > > these pseudo-backing_devs?
> >
> > That's correct, silly me. Updated it.
>
> Looks much more to my taste, thank you!
>
> > >
> > > p.s. a grand little change would be to move page_cluster and swap_setup()
> > > from mm/swap.c to mm/swap_state.c: they have nothing to do with the other
> > > contents of swap.c, and everything to do with the contents of swap_state.c.
> > > Why swap.c is called swap.c is rather a mystery.
> >
> > Tried, but looks page_cluster is used in sysctl, moving to swap_state.c will
> > make it optional. don't want to add another #ifdef, so give up.
>
> Good point, thanks for trying, maybe I'll attack it next time it
> irritates me.
>
> I don't yet know whether I approve of your changes or not, but running
> with them to see (and I'll send another bugfix separately in a moment).
>
> I was the one who removed the swap_device_lock() which 2.4 used,
> because it almost always ended up having to take both swap_list_lock()
> and swap_device_lock(si). You seem to have done a much better job of
> separating them usefully, but I need to convince myself that it does
> end up safely.
>
> My reservations so far would be: how many installations actually have
> more than one swap area, so is it a good tradeoff to add more overhead
> to help those at the (slight) expense of everyone else? The increasingly
> ugly page_mapping() worries me, and the static array of swapper_spaces
> annoys me a little.
>
> I'm glad Minchan has now pointed you to Rik's posting of two years ago:
> I think there are more important changes to be made in that direction.
Not sure how others use multiple swaps, but current lock contention forces us
to use multiple swaps. I haven't carefully think about Rik's posting, but looks
it doesn't solve the lock contention problem.
Thanks,
Shaohua
next prev parent reply other threads:[~2013-01-30 10:00 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-01-25 3:45 boot warnings due to swap: make each swap partition have one address_space Sasha Levin
2013-01-25 3:45 ` Sasha Levin
2013-01-25 4:25 ` Shaohua Li
2013-01-25 4:25 ` Shaohua Li
2013-01-25 23:37 ` Sasha Levin
2013-01-25 23:37 ` Sasha Levin
2013-01-27 2:16 ` Hugh Dickins
2013-01-27 2:16 ` Hugh Dickins
2013-01-27 14:12 ` Shaohua Li
2013-01-27 14:12 ` Shaohua Li
2013-01-27 21:40 ` Hugh Dickins
2013-01-27 21:40 ` Hugh Dickins
2013-01-29 15:56 ` Valdis.Kletnieks
2013-01-30 9:59 ` Shaohua Li [this message]
2013-01-30 9:59 ` Shaohua Li
2013-02-04 5:12 ` Hugh Dickins
2013-02-04 5:12 ` Hugh Dickins
2013-01-27 21:47 ` [PATCH next/mmotm] swap: add per-partition lock for swapfile fix Hugh Dickins
2013-01-27 21:47 ` Hugh Dickins
2013-01-28 0:07 ` Shaohua Li
2013-01-28 0:07 ` Shaohua Li
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=20130130095944.GA11457@kernel.org \
--to=shli@kernel.org \
--cc=akpm@linux-foundation.org \
--cc=hughd@google.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=minchan@kernel.org \
--cc=riel@redhat.com \
--cc=sasha.levin@oracle.com \
--cc=shli@fusionio.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.