Re: [Xen-devel] [PATCH] x86/mm: use put_page_type_preemptible in put_page_from_l{3, 4}e

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.09.17 at 15:13, <wei.liu2@xxxxxxxxxx> wrote:
> On Mon, Sep 04, 2017 at 07:06:51AM -0600, Jan Beulich wrote:
>> >>> On 04.09.17 at 13:42, <wei.liu2@xxxxxxxxxx> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel