| Message ID | 20170904114206.32659-1-wei.liu2@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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 >>
>>> 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
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?
>>> 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
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 )
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(-)