From: George Dunlap <George.Dunlap@eu.citrix.com>
To: Olaf Hering <olaf@aepfle.de>
Cc: xen-devel@lists.xensource.com
Subject: Re: [PATCH 05/21] xenpaging: add signal handling
Date: Thu, 2 Dec 2010 11:48:11 +0000 [thread overview]
Message-ID: <AANLkTikt_=5X6_CMmjuN3egbns9mX+mmFAqTH66+TTvH@mail.gmail.com> (raw)
In-Reply-To: <20101202095914.GB29462@aepfle.de>
Yeah, they certainly seem like reasonable changes. They just need to
be pointed out, so that people have an idea the actual impact of the
patch (both now when accepting it, and later when going back trying to
figure out where something broke).
FYI, I normally do a bunch of work (involving a large number of
changes) first, then do "hg diff > working.diff ; hg update -C" and
then go through working.diff to see where the individual changes are
best suited, whether in a new patch, or in a modification to an
existing patch. (emacs diff-mode is really helpful here.) That helps
me review my own code, and promotes good patch hygiene. :-)
Peace,
-George
On Thu, Dec 2, 2010 at 9:59 AM, Olaf Hering <olaf@aepfle.de> wrote:
> On Fri, Nov 26, George Dunlap wrote:
>
>> Olaf, I haven't been looking at these patches as we've been going
>> along, but there seem to be two things happening in this patch not
>> mentioned in the description:
>
> This was a "grown" patch.
>
>> * Making xenpaging_teardown() not skip when a tear-down item fails,
>> but continue to try to tear down the rest
>
> If a domain is shutting down, xc_mem_event_disable will always fail
> because d->is_dying is checked. Thats why I removed the bail_out part.
>
>> * Making return values for the program as a whole (1 for initializing
>> the paging, 2 for a failed file open)
>
> return codes are currently not perfect, sometimes -1 is leaked.
> I think xenpaging should either return 0 or 1.
>
>> These kinds of things should at least be mentioned in the description;
>> and I would personally probably pull them out and put them in a
>> separate patch.
>
> Will do better next time.
> Thanks for the comment.
>
>
> Olaf
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>
next prev parent reply other threads:[~2010-12-02 11:48 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-11-26 13:49 [PATCH 00/21] xenpaging changes for xen-unstable Olaf Hering
2010-11-26 13:49 ` [PATCH 01/21] xenpaging: break endless loop during inital page-out with large pagefiles Olaf Hering
2010-11-26 13:49 ` [PATCH 02/21] xenpaging: Open paging file only if xenpaging_init() succeeds Olaf Hering
2010-11-26 13:49 ` [PATCH 03/21] xenpaging: allow only one xenpaging binary per guest Olaf Hering
2010-11-26 13:49 ` [PATCH 04/21] xenpaging: populate paged-out pages unconditionally Olaf Hering
2010-11-26 13:49 ` [PATCH 05/21] xenpaging: add signal handling Olaf Hering
2010-11-26 14:29 ` George Dunlap
2010-12-02 9:59 ` Olaf Hering
2010-12-02 11:48 ` George Dunlap [this message]
2010-11-26 13:49 ` [PATCH 06/21] xenpaging: print info when free request slots drop below 2 Olaf Hering
2010-11-26 13:49 ` [PATCH 07/21] xenpaging: print p2mt for already paged-in pages Olaf Hering
2010-11-26 13:49 ` [PATCH 08/21] xenpaging: notify policy only on resume Olaf Hering
2010-11-26 13:49 ` [PATCH 09/21] xenpaging: allow negative num_pages and limit num_pages Olaf Hering
2010-11-26 13:49 ` [PATCH 10/21] xenpaging: when populating a page, check if populating is already in progress Olaf Hering
2010-11-26 13:49 ` [PATCH 11/21] xenpaging: optimize p2m_mem_paging_populate Olaf Hering
2010-11-26 13:49 ` [PATCH 12/21] xenpaging: print xenpaging cmdline options Olaf Hering
2010-11-26 13:49 ` [PATCH 13/21] xenpaging: handle temporary out-of-memory conditions during page-in Olaf Hering
2010-11-26 13:49 ` [PATCH 14/21] xenpaging: increase recently used pages from 4MB to 64MB Olaf Hering
2010-11-26 13:49 ` [PATCH 15/21] xenpaging: update machine_to_phys_mapping during page-in and page deallocation Olaf Hering
2010-11-26 13:49 ` [PATCH 16/21] xenpaging: drop paged pages in guest_remove_page Olaf Hering
2010-11-26 13:49 ` [PATCH 17/21] xenpaging: handle HVMCOPY_gfn_paged_out in copy_from/to_user Olaf Hering
2010-11-26 13:49 ` [PATCH 18/21] xenpaging: prevent page-out of first 16MB Olaf Hering
2010-12-16 16:59 ` Olaf Hering
2010-12-17 9:28 ` Jan Beulich
2010-12-17 12:18 ` Olaf Hering
2010-11-26 13:49 ` [PATCH 19/21] xenpaging: start xenpaging via config option Olaf Hering
2010-11-26 13:49 ` [PATCH 20/21] xenpaging: add dynamic startup delay for xenpaging Olaf Hering
2010-11-26 13:49 ` [PATCH 21/21] xenpaging: (sparse) documenation Olaf Hering
2010-12-07 17:57 ` Ian Jackson
2010-12-07 17:56 ` [PATCH 00/21] xenpaging changes for xen-unstable Ian Jackson
2010-12-07 18:05 ` Keir Fraser
2010-12-07 18:06 ` Keir Fraser
2010-12-07 18:22 ` Ian Jackson
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='AANLkTikt_=5X6_CMmjuN3egbns9mX+mmFAqTH66+TTvH@mail.gmail.com' \
--to=george.dunlap@eu.citrix.com \
--cc=olaf@aepfle.de \
--cc=xen-devel@lists.xensource.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).