Re: [Xen-devel] [PATCH 2/3] gnttab: Drop the frame parameter from get_paged_frame()
On Fri, Aug 25, 2017 at 10:38:19AM +0100, Andrew Cooper wrote: > >> +*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. > Sure. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/3] gnttab: Drop the frame parameter from get_paged_frame()
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 > Reviewed-by: Wei Liu Thanks. > >> --- >> CC: George Dunlap >> CC: Jan Beulich >> CC: Konrad Rzeszutek Wilk >> CC: Stefano Stabellini >> CC: Tim Deegan >> CC: Wei Liu >> --- >> 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@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/3] gnttab: Drop the frame parameter from get_paged_frame()
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 Reviewed-by: Wei Liu > --- > CC: George Dunlap > CC: Jan Beulich > CC: Konrad Rzeszutek Wilk > CC: Stefano Stabellini > CC: Tim Deegan > CC: Wei Liu > --- > 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? ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 2/3] gnttab: Drop the frame parameter from get_paged_frame()
It is redundant with the *page parameter. Signed-off-by: Andrew Cooper --- CC: George Dunlap CC: Jan Beulich CC: Konrad Rzeszutek Wilk CC: Stefano Stabellini CC: Tim Deegan CC: Wei Liu --- 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)) ) { -*frame = mfn_x(INVALID_MFN); *page = NULL; rc = GNTST_bad_page; } @@ -804,7 +802,7 @@ map_grant_ref( struct grant_table *lgt, *rgt; struct vcpu *led; grant_handle_t handle; -unsigned long frame = 0; +unsigned long frame; struct page_info *pg = NULL; intrc = GNTST_okay; u32old_pin; @@ -896,13 +894,12 @@ map_grant_ref( shared_entry_v1(rgt, op->ref).frame : shared_entry_v2(rgt, op->ref).full_page.frame; -rc = get_paged_frame(gfn, &frame, &pg, - op->flags & GNTMAP_readonly, rd); +rc = get_paged_frame(gfn, &pg, op->flags & GNTMAP_readonly, rd); if ( rc != GNTST_okay ) goto unlock_out_clear; act_set_gfn(act, _gfn(gfn)); act->domid = ld->domain_id; -act->frame = frame; +act->frame = page_to_mfn(pg); act->start = 0; act->length = PAGE_SIZE; act->is_sub_page = false; @@ -2163,7 +2160,6 @@ acquire_grant_for_copy( domid_t trans_domid; grant_ref_t trans_gref; struct domain *td; -unsigned long frame; uint16_t trans_page_off; uint16_t trans_length; bool is_sub_page; @@ -2256,8 +2252,6 @@ acquire_grant_for_copy( return rc; } -frame = page_to_mfn(*page); - /* * We dropped the lock, so we have to check that the grant didn't * change, and that nobody else tried to pin/unpin it. If anything @@ -2265,7 +2259,8 @@ acquire_grant_for_copy( */ if ( rgt->gt_version != 2 || act->pin != old_pin || - (old_pin && (act->domid != ldom || act->frame != frame || + (old_pin && (act->domid != ldom || + act->frame != page_to_mfn(*page) || act->start != trans_page_off || act->length != trans_length || act->trans_domain != td || @@ -2289,7 +2284,7 @@ acquire_grant_for_copy( act->length = trans_length; act->trans_domain = td; act->trans_gref = trans_gref; -act->frame = frame; +act->frame = page_to_mfn(*page); act_set_gfn(act, INVALID_GFN); /* * The actual remote remote grant may or may not be a sub-page, @@ -2313,7 +2308,7 @@ acquire_grant_for_copy( { unsigned long gfn = shared_entry_v1(rgt, gref).frame; -rc = get_paged_frame(gfn, &frame, page, readonly, rd); +rc = get_page