From: David Woodhouse <dwmw2@infradead.org>
To: Julien Grall <julien@xen.org>, Jan Beulich <jbeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
George Dunlap <george.dunlap@eu.citrix.com>,
Andrew Cooper <andrew.cooper3@citrix.com>,
Ian Jackson <ian.jackson@eu.citrix.com>,
George Dunlap <george.dunlap@citrix.com>,
Jeff Kubascik <jeff.kubascik@dornerworks.com>,
Stewart Hildebrand <stewart.hildebrand@dornerworks.com>,
xen-devel@lists.xenproject.org
Subject: Re: [Xen-devel] [PATCH 1/2] xen/mm: fold PGC_broken into PGC_state bits
Date: Tue, 17 Mar 2020 21:39:19 +0000 [thread overview]
Message-ID: <ff9726b0b190f98a493df5c19aff959013e09957.camel@infradead.org> (raw)
In-Reply-To: <28e1dfce-b4c5-7c73-0574-48fc4179443e@xen.org>
[-- Attachment #1.1: Type: text/plain, Size: 3528 bytes --]
On Fri, 2020-02-07 at 20:27 +0000, Julien Grall wrote:
> > + switch ( x & PGC_state )
> > {
> > - nx &= ~PGC_state;
> > - nx |= (((x & PGC_state) == PGC_state_free)
> > - ? PGC_state_offlined : PGC_state_offlining);
> > - }
> > + case PGC_state_inuse:
> > + case PGC_state_offlining:
> > + nx |= broken ? PGC_state_offlining : PGC_state_broken_offlining;
> > + break;
> > +
> > + case PGC_state_free:
> > + nx |= broken ? PGC_state_broken : PGC_state_offlined;
> >
> > - if ( broken )
> > - nx |= PGC_broken;
> > + case PGC_state_broken_offlining:
> > + nx |= PGC_state_broken_offlining;
> > +
> > + case PGC_state_offlined:
> > + case PGC_state_broken:
> > + nx |= PGC_state_broken;
>
> Shouldn't this be:
>
> case PGC_state_offlined:
> nx |= broken ? PGC_state_offlined : PGC_state_broken;
>
> case PGC_state_broken:
> nx |= PGC_state_broken;
>
> There are also quite a difference with the default case now. Without
> this patch, if you were to add a new state but not handling it here, you
> would transition to PGC_state_offlining. With this patch, we will
> transtion to 0 (i.e PGC_state_inuse for now).
>
> PGC_state_* are not an enum, the compiler can't help to catch new state
> that doesn't have a corresponding case. So I would suggest to add a
> default matching the current behavior and adding an
> ASSERT_UNREACHABLE(). Note that I am open to a different kind of
> handling here.
I revamped this, taking into account your later suggestion of a helper
that works on the count_info. Looks more like this:
/* If it was already broken, it stays broken */
if ( pgc_is_broken(x) )
broken = 1;
if ( pgc_is_offlined(x) || pgc_is(x, free) )
nx |= broken ? PGC_state_broken : PGC_state_offlined;
else
nx |= broken ? PGC_state_broken_offlining : PGC_state_offlining;
> > - /* Mutually-exclusive page states: { inuse, offlining, offlined, free }. */
> > -#define PGC_state PG_mask(3, 9)
> > -#define PGC_state_inuse PG_mask(0, 9)
> > -#define PGC_state_offlining PG_mask(1, 9)
> > -#define PGC_state_offlined PG_mask(2, 9)
> > -#define PGC_state_free PG_mask(3, 9)
> > -#define page_state_is(pg, st) (((pg)->count_info&PGC_state) == PGC_state_##st)
> > + /*
> > + * Mutually-exclusive page states:
> > + * { inuse, offlining, offlined, free, broken_offlining, broken }
> > + */
> > +#define PGC_state PG_mask(7, 9)
> > +#define PGC_state_inuse PG_mask(0, 9)
> > +#define PGC_state_offlining PG_mask(1, 9)
> > +#define PGC_state_offlined PG_mask(2, 9)
> > +#define PGC_state_free PG_mask(3, 9)
> > +#define PGC_state_broken_offlining PG_mask(4, 9)
> > +#define PGC_state_broken PG_mask(5, 9)
>
> I agree that making all the value aligned make it nicer to read, but
> this is increasing number of "unrelated" changes and makes the review
> more difficult.
>
> I would prefer if we leave the indentation alone for the current
> #define. But I am not going to push for it :).
I'm generally sympathetic to that point of view but at this point, all
those page states are kind of being redefined and it makes sense to
think about them all; having them all change doesn't hurt.
[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
next prev parent reply other threads:[~2020-03-17 21:40 UTC|newest]
Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-04 15:14 [Xen-devel] [XEN PATCH v2 1/2] Check zone before merging adjacent blocks in heap Stewart Hildebrand
2020-02-04 15:14 ` [Xen-devel] [DO NOT APPLY XEN PATCH v2 2/2] Test case for buddy allocator merging issue Stewart Hildebrand
2020-02-04 15:22 ` [Xen-devel] [XEN PATCH v2 1/2] Check zone before merging adjacent blocks in heap Jan Beulich
2020-02-04 15:37 ` George Dunlap
2020-02-05 9:50 ` David Woodhouse
2020-02-05 10:02 ` Jan Beulich
2020-02-05 10:24 ` David Woodhouse
2020-02-05 10:49 ` Jan Beulich
2020-02-05 11:23 ` David Woodhouse
2020-02-05 13:37 ` Jan Beulich
2020-02-05 14:12 ` David Woodhouse
2020-02-07 15:49 ` David Woodhouse
2020-02-07 15:57 ` [Xen-devel] [PATCH 1/2] xen/mm: fold PGC_broken into PGC_state bits David Woodhouse
2020-02-07 20:27 ` Julien Grall
2020-02-09 13:22 ` David Woodhouse
2020-02-09 17:59 ` Julien Grall
2020-03-17 21:39 ` David Woodhouse [this message]
2020-02-20 11:10 ` Jan Beulich
2020-03-17 21:52 ` David Woodhouse
2020-03-18 9:56 ` Jan Beulich
2020-03-18 12:31 ` Julien Grall
2020-03-18 13:23 ` Jan Beulich
2020-03-18 17:13 ` David Woodhouse
2020-03-19 8:49 ` Jan Beulich
2020-03-19 10:26 ` David Woodhouse
2020-03-19 11:59 ` Jan Beulich
2020-03-19 13:54 ` David Woodhouse
2020-03-19 14:46 ` Jan Beulich
2020-02-07 15:57 ` [Xen-devel] [PATCH 2/2] xen/mm: Introduce PG_state_uninitialised David Woodhouse
2020-02-07 16:30 ` Xia, Hongyan
2020-02-07 16:32 ` David Woodhouse
2020-02-07 16:40 ` Xia, Hongyan
2020-02-07 17:06 ` David Woodhouse
2020-02-07 18:04 ` David Woodhouse
2020-02-20 11:59 ` Jan Beulich
2020-02-20 13:27 ` Julien Grall
2020-03-17 22:15 ` David Woodhouse
2020-03-18 8:53 ` Paul Durrant
2020-03-18 10:10 ` Jan Beulich
2020-03-18 10:41 ` Paul Durrant
2020-03-18 11:12 ` Jan Beulich
2020-03-18 10:03 ` Jan Beulich
2020-03-18 12:11 ` David Woodhouse
2020-03-18 13:27 ` Jan Beulich
2020-02-05 10:22 ` [Xen-devel] [XEN PATCH v2 1/2] Check zone before merging adjacent blocks in heap Julien Grall
2020-02-05 10:32 ` David Woodhouse
2020-02-05 11:36 ` David Woodhouse
2020-02-04 15:37 ` Stewart Hildebrand
2020-03-19 21:17 [Xen-devel] [PATCH 0/2] Handle David Woodhouse
2020-03-19 21:21 ` [Xen-devel] [PATCH 1/2] xen/mm: fold PGC_broken into PGC_state bits David Woodhouse
2020-03-20 13:17 ` Paul Durrant
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=ff9726b0b190f98a493df5c19aff959013e09957.camel@infradead.org \
--to=dwmw2@infradead.org \
--cc=andrew.cooper3@citrix.com \
--cc=george.dunlap@citrix.com \
--cc=george.dunlap@eu.citrix.com \
--cc=ian.jackson@eu.citrix.com \
--cc=jbeulich@suse.com \
--cc=jeff.kubascik@dornerworks.com \
--cc=julien@xen.org \
--cc=konrad.wilk@oracle.com \
--cc=sstabellini@kernel.org \
--cc=stewart.hildebrand@dornerworks.com \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/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).