Re: [Xen-devel] [PATCH 2/3] gnttab: Drop the frame parameter from get_paged_frame()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 25/08/17 10:02, Wei Liu wrote:
> On Thu, Aug 24, 2017 at 06:55:54PM +0100, Andrew Cooper wrote:
>> It is redundant with the *page parameter.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Wei Liu <wei.liu2@xxxxxxxxxx>

Thanks.

>
>> ---
>> CC: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
>> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> CC: Tim Deegan <tim@xxxxxxx>
>> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
>> ---
>>  xen/common/grant_table.c | 50 
>> +++++++++++++++++++++---------------------------
>>  1 file changed, 22 insertions(+), 28 deletions(-)
>>
>> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
>> index 188c477..d8307b7 100644
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -257,13 +257,13 @@ static inline void active_entry_release(struct 
>> active_grant_entry *act)
>>      spin_unlock(&act->lock);
>>  }
>>  
>> -/* Check if the page has been paged out, or needs unsharing.
>> -   If rc == GNTST_okay, *page contains the page struct with a ref taken.
>> -   Caller must do put_page(*page).
>> -   If any error, *page = NULL, *frame = INVALID_MFN, no ref taken. */
>> -static int get_paged_frame(unsigned long gfn, unsigned long *frame,
>> -                           struct page_info **page, bool readonly,
>> -                           struct domain *rd)
>> +/*
>> + * Check if the page has been paged out, or needs unsharing.
>> + * If rc == GNTST_okay, *page contains the page struct with a ref taken.
>> + * Caller must do put_page(*page). If any error, *page = NULL, no ref taken.
>> + */
>> +static int get_paged_frame(unsigned long gfn, struct page_info **page,
>> +                           bool readonly, struct domain *rd)
>>  {
>>      int rc = GNTST_okay;
>>  #if defined(P2M_PAGED_TYPES) || defined(P2M_SHARED_TYPES)
>> @@ -273,7 +273,6 @@ static int get_paged_frame(unsigned long gfn, unsigned 
>> long *frame,
>>                                (readonly) ? P2M_ALLOC : P2M_UNSHARE);
>>      if ( !(*page) )
>>      {
>> -        *frame = mfn_x(INVALID_MFN);
>>          if ( p2m_is_shared(p2mt) )
>>              return GNTST_eagain;
>>          if ( p2m_is_paging(p2mt) )
>> @@ -283,13 +282,12 @@ static int get_paged_frame(unsigned long gfn, unsigned 
>> long *frame,
>>          }
>>          return GNTST_bad_page;
>>      }
>> -    *frame = page_to_mfn(*page);
>>  #else
>> -    *frame = mfn_x(gfn_to_mfn(rd, _gfn(gfn)));
>> -    *page = mfn_valid(_mfn(*frame)) ? mfn_to_page(*frame) : NULL;
>> +    mfn_t mfn = gfn_to_mfn(rd, _gfn(gfn));
>> +
>> +    *page = mfn_valid(mfn) ? mfn_to_page(mfn_x(mfn)) : NULL;
>>      if ( (!(*page)) || (!get_page(*page, rd)) )
> Mind dropping those unneeded parentheses?

I'm planning separate cleanup to this function (including renaming it). 
I'd prefer to defer changes like this to there.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel