| Message ID | 1503597355-21334-3-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@citrix.com> Reviewed-by: Wei Liu <wei.liu2@citrix.com> > --- > CC: George Dunlap <George.Dunlap@eu.citrix.com> > CC: Jan Beulich <JBeulich@suse.com> > CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Tim Deegan <tim@xen.org> > CC: Wei Liu <wei.liu2@citrix.com> > --- > 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?
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@citrix.com> > Reviewed-by: Wei Liu <wei.liu2@citrix.com> Thanks. > >> --- >> CC: George Dunlap <George.Dunlap@eu.citrix.com> >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> >> CC: Stefano Stabellini <sstabellini@kernel.org> >> CC: Tim Deegan <tim@xen.org> >> CC: Wei Liu <wei.liu2@citrix.com> >> --- >> 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
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.
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; int rc = GNTST_okay; u32 old_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_paged_frame(gfn, page, readonly, rd); if ( rc != GNTST_okay ) goto unlock_out_clear; act_set_gfn(act, _gfn(gfn)); @@ -2323,8 +2318,7 @@ acquire_grant_for_copy( } else if ( !(sha2->hdr.flags & GTF_sub_page) ) { - rc = get_paged_frame(sha2->full_page.frame, &frame, page, - readonly, rd); + rc = get_paged_frame(sha2->full_page.frame, page, readonly, rd); if ( rc != GNTST_okay ) goto unlock_out_clear; act_set_gfn(act, _gfn(sha2->full_page.frame)); @@ -2334,8 +2328,7 @@ acquire_grant_for_copy( } else { - rc = get_paged_frame(sha2->sub_page.frame, &frame, page, - readonly, rd); + rc = get_paged_frame(sha2->sub_page.frame, page, readonly, rd); if ( rc != GNTST_okay ) goto unlock_out_clear; act_set_gfn(act, _gfn(sha2->sub_page.frame)); @@ -2352,7 +2345,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); } } else @@ -2515,12 +2508,13 @@ static int gnttab_copy_claim_buf(const struct gnttab_copy *op, } else { - rc = get_paged_frame(ptr->u.gmfn, &buf->frame, &buf->page, + rc = get_paged_frame(ptr->u.gmfn, &buf->page, buf->read_only, buf->domain); if ( rc != GNTST_okay ) PIN_FAIL(out, rc, "source frame %"PRI_xen_pfn" invalid.\n", ptr->u.gmfn); + buf->frame = page_to_mfn(buf->page); buf->ptr.u.gmfn = ptr->u.gmfn; buf->ptr.offset = 0; buf->len = PAGE_SIZE;
It is redundant with the *page parameter. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: George Dunlap <George.Dunlap@eu.citrix.com> CC: Jan Beulich <JBeulich@suse.com> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Tim Deegan <tim@xen.org> CC: Wei Liu <wei.liu2@citrix.com> --- xen/common/grant_table.c | 50 +++++++++++++++++++++--------------------------- 1 file changed, 22 insertions(+), 28 deletions(-)