From: Hugh Dickins <hughd@google.com>
To: Weijie Yang <weijieut@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
Mel Gorman <mgorman@suse.de>, Michal Hocko <mhocko@suse.cz>,
Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>,
linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] mm: swap: Use swapfiles in priority order
Date: Mon, 24 Feb 2014 00:28:01 -0800 (PST) [thread overview]
Message-ID: <alpine.LSU.2.11.1402232344280.1890@eggly.anvils> (raw)
In-Reply-To: <CABdxLJHS5kw0rpD=+77iQtc6PMeRXoWnh-nh5VzjjfGHJ5wLGQ@mail.gmail.com>
On Sun, 16 Feb 2014, Weijie Yang wrote:
> On Fri, Feb 14, 2014 at 9:10 PM, Christian Ehrhardt
> <ehrhardt@linux.vnet.ibm.com> wrote:
> > Weijie Yang <weijie.yang.kh <at> gmail.com> writes:
> >
> >>
> >> On Thu, Feb 13, 2014 at 6:42 PM, Mel Gorman <mgorman <at> suse.de> wrote:
> > [...]
> >> > - for (type = swap_list.next; type >= 0 && wrapped < 2; type = next) {
> >> > + for (type = swap_list.head; type >= 0 && wrapped < 2; type = next) {
> >>
> > [...]
> >> Does it lead to a "schlemiel the painter's algorithm"?
> >> (please forgive my rude words, but I can't find a precise word to describe it
> >>
> >> How about modify it like this?
> >>
> > [...]
> >> - next = swap_list.head;
> >> + next = type;
> > [...]
> >
> > Hi,
> > unfortunately withou studying the code more thoroughly I'm not even sure if
> > you meant you code to extend or replace Mels patch.
> >
> > To be sure about your intention. You refered to algorithm scaling because
> > you were afraid the new code would scan the full list all the time right ?
> >
> > But simply letting the machines give a try for both options I can now
> > qualify both.
> >
> > Just your patch creates a behaviour of jumping over priorities (see the
> > following example), so I hope you meant combining both patches.
> > With that in mind the patch I eventually tested the combined patch looking
> > like this:
>
> Hi Christian,
>
> My patch is not appropriate, so there is no need to combine it with Mel's patch.
>
> What I worried about Mel's patch is not only the search efficiency,
> actually it has
> negligible impact on system, but also the following scenario:
>
> If two swapfiles have the same priority, in ordinary semantic, they
> should be used
> in balance. But with Mel's patch, it will always get the free
> swap_entry from the
> swap_list.head in priority order, I worry it could break the balance.
>
> I think you can test this scenario if you have available test machines.
>
> Appreciate for your done.
Weijie, I agree with you on both points: Schlemiel effect of repeatedly
restarting from head (already an unintended defect before Mel's patch),
and more importantly the breakage of swapfiles at the same priority.
Sorry, it has to be a Nak to Mel's patch, which fixes one behavior
at the expense of another. And if we were to go that way, better
just to rip out all of swap_list.next and highest_priority_index.
I had hoped to respond today with a better patch; but I just haven't
got it right yet either. I think we don't need to rush to fix it,
but fix it we certainly should.
Christian, congratulations on discovering this wrong behavior: at
first I assumed it came from Shaohua's 3.9 highest_priority_index
changes, but no; then I assumed it came from my 2.6.14 swap_lock
changes; but now I think it goes back even before 2.4.0, probably
ever since there have been swap priorities.
Hugh
next prev parent reply other threads:[~2014-02-24 8:28 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-02-13 10:42 [PATCH] mm: swap: Use swapfiles in priority order Mel Gorman
2014-02-13 15:58 ` Weijie Yang
2014-02-14 10:17 ` Mel Gorman
2014-02-14 13:33 ` Weijie Yang
[not found] ` <loom.20140214T135753-812@post.gmane.org>
[not found] ` <CABdxLJHS5kw0rpD=+77iQtc6PMeRXoWnh-nh5VzjjfGHJ5wLGQ@mail.gmail.com>
2014-02-24 8:28 ` Hugh Dickins [this message]
2014-04-12 21:00 ` [PATCH 0/2] swap: simplify/fix swap_list handling and iteration Dan Streetman
2014-04-12 21:00 ` [PATCH 1/2] swap: change swap_info singly-linked list to list_head Dan Streetman
2014-04-23 10:34 ` Mel Gorman
2014-04-24 0:17 ` Shaohua Li
2014-04-24 8:30 ` Mel Gorman
2014-04-24 18:48 ` Dan Streetman
2014-04-25 4:15 ` Weijie Yang
2014-05-02 20:00 ` Dan Streetman
2014-05-04 9:39 ` Bob Liu
2014-05-04 20:16 ` Dan Streetman
2014-04-25 8:38 ` Mel Gorman
2014-04-12 21:00 ` [PATCH 2/2] swap: use separate priority list for available swap_infos Dan Streetman
2014-04-23 13:14 ` Mel Gorman
2014-04-24 17:52 ` Dan Streetman
2014-04-25 8:49 ` Mel Gorman
2014-05-02 19:02 ` [PATCHv2 0/4] swap: simplify/fix swap_list handling and iteration Dan Streetman
2014-05-02 19:02 ` [PATCHv2 1/4] swap: change swap_info singly-linked list to list_head Dan Streetman
2014-05-02 19:02 ` [PATCH 2/4] plist: add helper functions Dan Streetman
2014-05-12 10:35 ` Mel Gorman
2014-05-02 19:02 ` [PATCH 3/4] plist: add plist_rotate Dan Streetman
2014-05-06 2:18 ` Steven Rostedt
2014-05-06 20:12 ` Dan Streetman
2014-05-06 20:39 ` Steven Rostedt
2014-05-06 21:47 ` Dan Streetman
2014-05-06 22:43 ` Steven Rostedt
2014-05-02 19:02 ` [PATCH 4/4] swap: change swap_list_head to plist, add swap_avail_head Dan Streetman
2014-05-05 15:51 ` Dan Streetman
2014-05-05 19:13 ` Steven Rostedt
2014-05-05 19:38 ` Peter Zijlstra
2014-05-09 20:42 ` [PATCH] plist: make CONFIG_DEBUG_PI_LIST selectable Dan Streetman
2014-05-09 21:17 ` Steven Rostedt
2014-05-12 11:11 ` [PATCH 4/4] swap: change swap_list_head to plist, add swap_avail_head Mel Gorman
2014-05-12 13:00 ` Dan Streetman
2014-05-12 16:38 ` [PATCHv3 0/4] swap: simplify/fix swap_list handling and iteration Dan Streetman
2014-05-12 16:38 ` [PATCHv2 1/4] swap: change swap_info singly-linked list to list_head Dan Streetman
2014-05-12 16:38 ` [PATCH 2/4] plist: add helper functions Dan Streetman
2014-05-12 16:38 ` [PATCHv2 3/4] plist: add plist_requeue Dan Streetman
2014-05-13 10:33 ` Mel Gorman
2014-05-12 16:38 ` [PATCHv2 4/4] swap: change swap_list_head to plist, add swap_avail_head Dan Streetman
2014-05-13 10:34 ` Mel Gorman
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=alpine.LSU.2.11.1402232344280.1890@eggly.anvils \
--to=hughd@google.com \
--cc=akpm@linux-foundation.org \
--cc=ehrhardt@linux.vnet.ibm.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mgorman@suse.de \
--cc=mhocko@suse.cz \
--cc=weijieut@gmail.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 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).