| Message ID | 1503597355-21334-2-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Aug 24, 2017 at 06:55:53PM +0100, Andrew Cooper wrote: > It is redundant with the *page parameter. Rename the grant_frame parameter to > indicate that it is local, and highlight the correctness of the change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Wei Liu <wei.liu2@citrix.com> > > - rc = acquire_grant_for_copy(td, trans_gref, rd->domain_id, > - readonly, &grant_frame, page, > - &trans_page_off, &trans_length, > - false); > + rc = acquire_grant_for_copy( > + td, trans_gref, rd->domain_id, readonly, page, &trans_page_off, > + &trans_length, false); The change of style seems unnecessary, but I don't think I care that much.
>>> On 24.08.17 at 19:55, <andrew.cooper3@citrix.com> wrote: > It is redundant with the *page parameter. Rename the grant_frame parameter to > indicate that it is local, and highlight the correctness of the change. I don't understand this second sentence: For one, grant_frame isn't and wasn't a parameter (but a local variable), and then I also don't see what is being highlighted to validate the correctness. Or am I being mislead by it not reading "and to highlight ..."? So far I was believing that such an omission is okay in German, but not in English. > @@ -2238,10 +2240,9 @@ acquire_grant_for_copy( > active_entry_release(act); > grant_read_unlock(rgt); > > - rc = acquire_grant_for_copy(td, trans_gref, rd->domain_id, > - readonly, &grant_frame, page, > - &trans_page_off, &trans_length, > - false); > + rc = acquire_grant_for_copy( > + td, trans_gref, rd->domain_id, readonly, page, &trans_page_off, > + &trans_length, false); As hinted at by Wei, I'd prefer if you didn't alter the style to the uglier variant - we really should use that only if the commonly used one really gets unwieldy, which imo is not the case here. > @@ -2255,6 +2256,8 @@ 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 > @@ -2262,7 +2265,7 @@ acquire_grant_for_copy( > */ > if ( rgt->gt_version != 2 || > act->pin != old_pin || > - (old_pin && (act->domid != ldom || act->frame != grant_frame || > + (old_pin && (act->domid != ldom || act->frame != frame || > act->start != trans_page_off || > act->length != trans_length || > act->trans_domain != td || > @@ -2286,7 +2289,7 @@ acquire_grant_for_copy( > act->length = trans_length; > act->trans_domain = td; > act->trans_gref = trans_gref; > - act->frame = grant_frame; > + act->frame = frame; These three get re-done in patch 2 - I think you could easily bring them into their final shape right away, proving the correctness of the change by reducing the scope of frame to ... > @@ -2310,7 +2313,7 @@ acquire_grant_for_copy( > { > unsigned long gfn = shared_entry_v1(rgt, gref).frame; > > - rc = get_paged_frame(gfn, &grant_frame, page, readonly, rd); > + rc = get_paged_frame(gfn, &frame, page, readonly, rd); ... the outer if() this and the following hunks are contained in. Jan
On 25/08/17 11:13, Jan Beulich wrote: >>>> On 24.08.17 at 19:55, <andrew.cooper3@citrix.com> wrote: >> It is redundant with the *page parameter. Rename the grant_frame parameter to >> indicate that it is local, and highlight the correctness of the change. > I don't understand this second sentence: For one, grant_frame isn't > and wasn't a parameter (but a local variable), and then I also don't > see what is being highlighted to validate the correctness. Or am I > being mislead by it not reading "and to highlight ..."? So far I was > believing that such an omission is okay in German, but not in English. Taking grant_frame out entirely at this point (is what I did originally, but) does not make the patch read as obviously-safe. Changing its name causes the compiler to help spot all uses. > >> @@ -2238,10 +2240,9 @@ acquire_grant_for_copy( >> active_entry_release(act); >> grant_read_unlock(rgt); >> >> - rc = acquire_grant_for_copy(td, trans_gref, rd->domain_id, >> - readonly, &grant_frame, page, >> - &trans_page_off, &trans_length, >> - false); >> + rc = acquire_grant_for_copy( >> + td, trans_gref, rd->domain_id, readonly, page, &trans_page_off, >> + &trans_length, false); > As hinted at by Wei, I'd prefer if you didn't alter the style to the > uglier variant - we really should use that only if the commonly > used one really gets unwieldy, which imo is not the case here. Ugly? What is ugly (in general) is squashing all parameters together on many lines on the RHS. In this case, the false on the end also interacts with my command line parameter patch, where it becomes opt_transitive_grants, and gets in the way of the previous alignment strategy. ~Andrew
>>> On 25.08.17 at 12:50, <andrew.cooper3@citrix.com> wrote: > On 25/08/17 11:13, Jan Beulich wrote: >>>>> On 24.08.17 at 19:55, <andrew.cooper3@citrix.com> wrote: >>> It is redundant with the *page parameter. Rename the grant_frame parameter to >>> indicate that it is local, and highlight the correctness of the change. >> I don't understand this second sentence: For one, grant_frame isn't >> and wasn't a parameter (but a local variable), and then I also don't >> see what is being highlighted to validate the correctness. Or am I >> being mislead by it not reading "and to highlight ..."? So far I was >> believing that such an omission is okay in German, but not in English. > > Taking grant_frame out entirely at this point (is what I did originally, > but) does not make the patch read as obviously-safe. Changing its name > causes the compiler to help spot all uses. With the light ambiguity resulting from there having been a function parameter of this very name. >>> @@ -2238,10 +2240,9 @@ acquire_grant_for_copy( >>> active_entry_release(act); >>> grant_read_unlock(rgt); >>> >>> - rc = acquire_grant_for_copy(td, trans_gref, rd->domain_id, >>> - readonly, &grant_frame, page, >>> - &trans_page_off, &trans_length, >>> - false); >>> + rc = acquire_grant_for_copy( >>> + td, trans_gref, rd->domain_id, readonly, page, &trans_page_off, >>> + &trans_length, false); >> As hinted at by Wei, I'd prefer if you didn't alter the style to the >> uglier variant - we really should use that only if the commonly >> used one really gets unwieldy, which imo is not the case here. > > Ugly? What is ugly (in general) is squashing all parameters together on > many lines on the RHS. As always with style it's a matter of taste: To me, the indentation of the style you switch to looks always odd, as does the line break right after the opening parenthesis (and this would extend to other expressions too). Of course, ./CODING_STYLE has nothing on this at all - "Long lines should be split at sensible places and the trailing portions indented" leaves open what "sensible" is, or how deep indentation ought to be. Anyway, I certainly won't block the patch over this, I just find it odd for style to be changed here without real need. Jan
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index 36895aa..188c477 100644 --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -2142,15 +2142,17 @@ static void fixup_status_for_copy_pin(const struct active_grant_entry *act, gnttab_clear_flag(_GTF_reading, status); } -/* Grab a frame number from a grant entry and update the flags and pin - count as appropriate. If rc == GNTST_okay, note that this *does* - take one ref count on the target page, stored in *page. - If there is any error, *page = NULL, no ref taken. */ +/* + * Grab a frame number from a grant entry and update the flags and pin + * count as appropriate. If rc == GNTST_okay, note that this *does* + * take one ref count on the target page, stored in *page. + * If there is any error, *page = NULL, no ref taken. + */ static int acquire_grant_for_copy( struct domain *rd, grant_ref_t gref, domid_t ldom, bool readonly, - unsigned long *frame, struct page_info **page, - uint16_t *page_off, uint16_t *length, bool allow_transitive) + struct page_info **page, uint16_t *page_off, uint16_t *length, + bool allow_transitive) { struct grant_table *rgt = rd->grant_table; grant_entry_v2_t *sha2; @@ -2161,7 +2163,7 @@ acquire_grant_for_copy( domid_t trans_domid; grant_ref_t trans_gref; struct domain *td; - unsigned long grant_frame; + unsigned long frame; uint16_t trans_page_off; uint16_t trans_length; bool is_sub_page; @@ -2238,10 +2240,9 @@ acquire_grant_for_copy( active_entry_release(act); grant_read_unlock(rgt); - rc = acquire_grant_for_copy(td, trans_gref, rd->domain_id, - readonly, &grant_frame, page, - &trans_page_off, &trans_length, - false); + rc = acquire_grant_for_copy( + td, trans_gref, rd->domain_id, readonly, page, &trans_page_off, + &trans_length, false); grant_read_lock(rgt); act = active_entry_acquire(rgt, gref); @@ -2255,6 +2256,8 @@ 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 @@ -2262,7 +2265,7 @@ acquire_grant_for_copy( */ if ( rgt->gt_version != 2 || act->pin != old_pin || - (old_pin && (act->domid != ldom || act->frame != grant_frame || + (old_pin && (act->domid != ldom || act->frame != frame || act->start != trans_page_off || act->length != trans_length || act->trans_domain != td || @@ -2286,7 +2289,7 @@ acquire_grant_for_copy( act->length = trans_length; act->trans_domain = td; act->trans_gref = trans_gref; - act->frame = grant_frame; + act->frame = frame; act_set_gfn(act, INVALID_GFN); /* * The actual remote remote grant may or may not be a sub-page, @@ -2310,7 +2313,7 @@ acquire_grant_for_copy( { unsigned long gfn = shared_entry_v1(rgt, gref).frame; - rc = get_paged_frame(gfn, &grant_frame, page, readonly, rd); + rc = get_paged_frame(gfn, &frame, page, readonly, rd); if ( rc != GNTST_okay ) goto unlock_out_clear; act_set_gfn(act, _gfn(gfn)); @@ -2320,7 +2323,7 @@ acquire_grant_for_copy( } else if ( !(sha2->hdr.flags & GTF_sub_page) ) { - rc = get_paged_frame(sha2->full_page.frame, &grant_frame, page, + rc = get_paged_frame(sha2->full_page.frame, &frame, page, readonly, rd); if ( rc != GNTST_okay ) goto unlock_out_clear; @@ -2331,7 +2334,7 @@ acquire_grant_for_copy( } else { - rc = get_paged_frame(sha2->sub_page.frame, &grant_frame, page, + rc = get_paged_frame(sha2->sub_page.frame, &frame, page, readonly, rd); if ( rc != GNTST_okay ) goto unlock_out_clear; @@ -2349,7 +2352,7 @@ acquire_grant_for_copy( act->length = trans_length; act->trans_domain = td; act->trans_gref = trans_gref; - act->frame = grant_frame; + act->frame = frame; } } else @@ -2368,7 +2371,6 @@ acquire_grant_for_copy( *page_off = act->start; *length = act->length; - *frame = act->frame; active_entry_release(act); grant_read_unlock(rgt); @@ -2503,11 +2505,11 @@ static int gnttab_copy_claim_buf(const struct gnttab_copy *op, { rc = acquire_grant_for_copy(buf->domain, ptr->u.ref, current->domain->domain_id, - buf->read_only, - &buf->frame, &buf->page, + buf->read_only, &buf->page, &buf->ptr.offset, &buf->len, true); if ( rc != GNTST_okay ) goto out; + buf->frame = page_to_mfn(buf->page); buf->ptr.u.ref = ptr->u.ref; buf->have_grant = 1; }
It is redundant with the *page parameter. Rename the grant_frame parameter to indicate that it is local, and highlight the correctness of the change. 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 | 42 ++++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 20 deletions(-)