* [PATCH] x86/mm: use put_page_type_preemptible in put_page_from_l{3, 4}e
@ 2017-09-04 11:42 Wei Liu
2017-09-04 11:45 ` Wei Liu
2017-09-04 13:06 ` Jan Beulich
0 siblings, 2 replies; 6+ messages in thread
From: Wei Liu @ 2017-09-04 11:42 UTC (permalink / raw)
To: Xen-devel; +Cc: George Dunlap, Andrew Cooper, Wei Liu, Jan Beulich
No functional change.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
---
xen/arch/x86/mm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index e5b0cceae4..314da84720 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1376,7 +1376,7 @@ static int put_page_from_l3e(l3_pgentry_t l3e, unsigned long pfn,
if ( unlikely(partial > 0) )
{
ASSERT(!defer);
- return __put_page_type(pg, 1);
+ return put_page_type_preemptible(pg);
}
if ( defer )
@@ -1399,7 +1399,7 @@ static int put_page_from_l4e(l4_pgentry_t l4e, unsigned long pfn,
if ( unlikely(partial > 0) )
{
ASSERT(!defer);
- return __put_page_type(pg, 1);
+ return put_page_type_preemptible(pg);
}
if ( defer )
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/mm: use put_page_type_preemptible in put_page_from_l{3, 4}e
2017-09-04 11:42 [PATCH] x86/mm: use put_page_type_preemptible in put_page_from_l{3, 4}e Wei Liu
@ 2017-09-04 11:45 ` Wei Liu
2017-09-04 12:06 ` Andrew Cooper
2017-09-04 13:06 ` Jan Beulich
1 sibling, 1 reply; 6+ messages in thread
From: Wei Liu @ 2017-09-04 11:45 UTC (permalink / raw)
To: Xen-devel; +Cc: George Dunlap, Andrew Cooper, Wei Liu, Jan Beulich
On Mon, Sep 04, 2017 at 12:42:06PM +0100, Wei Liu wrote:
> No functional change.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> xen/arch/x86/mm.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index e5b0cceae4..314da84720 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
In fact the forward declaration for __put_page_type here can also be
deleted.
> @@ -1376,7 +1376,7 @@ static int put_page_from_l3e(l3_pgentry_t l3e, unsigned long pfn,
> if ( unlikely(partial > 0) )
> {
> ASSERT(!defer);
> - return __put_page_type(pg, 1);
> + return put_page_type_preemptible(pg);
> }
>
> if ( defer )
> @@ -1399,7 +1399,7 @@ static int put_page_from_l4e(l4_pgentry_t l4e, unsigned long pfn,
> if ( unlikely(partial > 0) )
> {
> ASSERT(!defer);
> - return __put_page_type(pg, 1);
> + return put_page_type_preemptible(pg);
> }
>
> if ( defer )
> --
> 2.11.0
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/mm: use put_page_type_preemptible in put_page_from_l{3, 4}e
2017-09-04 11:45 ` Wei Liu
@ 2017-09-04 12:06 ` Andrew Cooper
0 siblings, 0 replies; 6+ messages in thread
From: Andrew Cooper @ 2017-09-04 12:06 UTC (permalink / raw)
To: Wei Liu, Xen-devel; +Cc: George Dunlap, Jan Beulich
On 04/09/17 12:45, Wei Liu wrote:
> On Mon, Sep 04, 2017 at 12:42:06PM +0100, Wei Liu wrote:
>> No functional change.
>>
>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>> ---
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> Cc: George Dunlap <george.dunlap@eu.citrix.com>
>> ---
>> xen/arch/x86/mm.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
>> index e5b0cceae4..314da84720 100644
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
> In fact the forward declaration for __put_page_type here can also be
> deleted.
With this done, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> @@ -1376,7 +1376,7 @@ static int put_page_from_l3e(l3_pgentry_t l3e, unsigned long pfn,
>> if ( unlikely(partial > 0) )
>> {
>> ASSERT(!defer);
>> - return __put_page_type(pg, 1);
>> + return put_page_type_preemptible(pg);
>> }
>>
>> if ( defer )
>> @@ -1399,7 +1399,7 @@ static int put_page_from_l4e(l4_pgentry_t l4e, unsigned long pfn,
>> if ( unlikely(partial > 0) )
>> {
>> ASSERT(!defer);
>> - return __put_page_type(pg, 1);
>> + return put_page_type_preemptible(pg);
>> }
>>
>> if ( defer )
>> --
>> 2.11.0
>>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/mm: use put_page_type_preemptible in put_page_from_l{3, 4}e
2017-09-04 11:42 [PATCH] x86/mm: use put_page_type_preemptible in put_page_from_l{3, 4}e Wei Liu
2017-09-04 11:45 ` Wei Liu
@ 2017-09-04 13:06 ` Jan Beulich
2017-09-04 13:13 ` Wei Liu
1 sibling, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2017-09-04 13:06 UTC (permalink / raw)
To: Wei Liu; +Cc: George Dunlap, AndrewCooper, Xen-devel
>>> On 04.09.17 at 13:42, <wei.liu2@citrix.com> wrote:
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -1376,7 +1376,7 @@ static int put_page_from_l3e(l3_pgentry_t l3e, unsigned long pfn,
> if ( unlikely(partial > 0) )
> {
> ASSERT(!defer);
> - return __put_page_type(pg, 1);
> + return put_page_type_preemptible(pg);
> }
>
> if ( defer )
> @@ -1399,7 +1399,7 @@ static int put_page_from_l4e(l4_pgentry_t l4e, unsigned long pfn,
> if ( unlikely(partial > 0) )
> {
> ASSERT(!defer);
> - return __put_page_type(pg, 1);
> + return put_page_type_preemptible(pg);
> }
Is this really a good idea? put_page_type_preemptible() is just
a thin wrapper around __put_page_type(), so that the latter
can remain private to mm.c. By going through the wrapper you
add another branch into a path that I would guess isn't executed
frequently enough for its constituents to remain in the uops
cache, but possibly frequently enough for the extra branch to
matter. Otoh I see we use get_page_type_preemptible() in mm.c
too in two places (albeit that one also has an extra ASSERT())...
Bottom line - since you've got Andrew's approval, I don't mean
to outright object to the change, but I like this extra aspect to
be taken into account before deciding whether to really put it in.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/mm: use put_page_type_preemptible in put_page_from_l{3, 4}e
2017-09-04 13:06 ` Jan Beulich
@ 2017-09-04 13:13 ` Wei Liu
2017-09-04 13:20 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Wei Liu @ 2017-09-04 13:13 UTC (permalink / raw)
To: Jan Beulich; +Cc: George Dunlap, AndrewCooper, Wei Liu, Xen-devel
On Mon, Sep 04, 2017 at 07:06:51AM -0600, Jan Beulich wrote:
> >>> On 04.09.17 at 13:42, <wei.liu2@citrix.com> wrote:
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -1376,7 +1376,7 @@ static int put_page_from_l3e(l3_pgentry_t l3e, unsigned long pfn,
> > if ( unlikely(partial > 0) )
> > {
> > ASSERT(!defer);
> > - return __put_page_type(pg, 1);
> > + return put_page_type_preemptible(pg);
> > }
> >
> > if ( defer )
> > @@ -1399,7 +1399,7 @@ static int put_page_from_l4e(l4_pgentry_t l4e, unsigned long pfn,
> > if ( unlikely(partial > 0) )
> > {
> > ASSERT(!defer);
> > - return __put_page_type(pg, 1);
> > + return put_page_type_preemptible(pg);
> > }
>
> Is this really a good idea? put_page_type_preemptible() is just
> a thin wrapper around __put_page_type(), so that the latter
> can remain private to mm.c. By going through the wrapper you
> add another branch into a path that I would guess isn't executed
> frequently enough for its constituents to remain in the uops
> cache, but possibly frequently enough for the extra branch to
> matter. Otoh I see we use get_page_type_preemptible() in mm.c
> too in two places (albeit that one also has an extra ASSERT())...
>
This patch is taken from my mm.c refactoring series.
Like you said, __put_page_type should remain private to mm.c. By making
this change I can move put_page_from_l{2,3,4}e to pv/mm.c and leave
__put_page_type in mm.c. Is this a good enough reason for you?
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/mm: use put_page_type_preemptible in put_page_from_l{3, 4}e
2017-09-04 13:13 ` Wei Liu
@ 2017-09-04 13:20 ` Jan Beulich
0 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2017-09-04 13:20 UTC (permalink / raw)
To: Wei Liu; +Cc: George Dunlap, AndrewCooper, Xen-devel
>>> On 04.09.17 at 15:13, <wei.liu2@citrix.com> wrote:
> On Mon, Sep 04, 2017 at 07:06:51AM -0600, Jan Beulich wrote:
>> >>> On 04.09.17 at 13:42, <wei.liu2@citrix.com> wrote:
>> > --- a/xen/arch/x86/mm.c
>> > +++ b/xen/arch/x86/mm.c
>> > @@ -1376,7 +1376,7 @@ static int put_page_from_l3e(l3_pgentry_t l3e,
> unsigned long pfn,
>> > if ( unlikely(partial > 0) )
>> > {
>> > ASSERT(!defer);
>> > - return __put_page_type(pg, 1);
>> > + return put_page_type_preemptible(pg);
>> > }
>> >
>> > if ( defer )
>> > @@ -1399,7 +1399,7 @@ static int put_page_from_l4e(l4_pgentry_t l4e,
> unsigned long pfn,
>> > if ( unlikely(partial > 0) )
>> > {
>> > ASSERT(!defer);
>> > - return __put_page_type(pg, 1);
>> > + return put_page_type_preemptible(pg);
>> > }
>>
>> Is this really a good idea? put_page_type_preemptible() is just
>> a thin wrapper around __put_page_type(), so that the latter
>> can remain private to mm.c. By going through the wrapper you
>> add another branch into a path that I would guess isn't executed
>> frequently enough for its constituents to remain in the uops
>> cache, but possibly frequently enough for the extra branch to
>> matter. Otoh I see we use get_page_type_preemptible() in mm.c
>> too in two places (albeit that one also has an extra ASSERT())...
>>
>
> This patch is taken from my mm.c refactoring series.
>
> Like you said, __put_page_type should remain private to mm.c. By making
> this change I can move put_page_from_l{2,3,4}e to pv/mm.c and leave
> __put_page_type in mm.c. Is this a good enough reason for you?
Ah, I see, that's fine then.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-09-04 13:23 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-04 11:42 [PATCH] x86/mm: use put_page_type_preemptible in put_page_from_l{3, 4}e Wei Liu
2017-09-04 11:45 ` Wei Liu
2017-09-04 12:06 ` Andrew Cooper
2017-09-04 13:06 ` Jan Beulich
2017-09-04 13:13 ` Wei Liu
2017-09-04 13:20 ` Jan Beulich
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.