Re: [Xen-devel] [PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Mon, Apr 29, 2019 at 9:52 AM George Dunlap <george.dunlap@xxxxxxxxxx> wrote:
>
> On 4/29/19 4:41 PM, Tamas K Lengyel wrote:
> > On Mon, Apr 29, 2019 at 9:01 AM Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >>
> >>>>> On 26.04.19 at 19:21, <tamas@xxxxxxxxxxxxx> wrote:
> >>> @@ -999,6 +996,10 @@ static int share_pages(struct domain *sd, gfn_t 
> >>> sgfn, shr_handle_t sh,
> >>>      mem_sharing_page_unlock(secondpg);
> >>>      mem_sharing_page_unlock(firstpg);
> >>>
> >>> +    BUG_ON(!put_count);
> >>> +    while ( put_count-- )
> >>> +        put_page_and_type(cpage);
> >>> +
> >>>      /* Free the client page */
> >>>      if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
> >>>          put_page(cpage);
> >>
> >> If this was to happen before all the put_page_and_type() calls,
> >> wouldn't it render unnecessary the extra get_page()/put_page()
> >> around this code region?
> >
> > It would - I already sent a version of the patch in that form but
> > there was unease expressed by George going that route because of the
> > complexity of the code and in his view it's the safe bet to keep the
> > extra references. I think the overhead of grabbing the extra
> > references is negligible enough that I'm not going to argue over it.
>
> Er, that's not what I said. :-)
>
> I gave four possible options, *one* of which was to change the ASSERT()
> to a BUG_ON(), and *one* of which was to keep the (probably redundant)
> get_page() and put_page() calls.  You appear to have done both. :-)

Ah indeed - I've combined those recommendations :)

>
> I haven't re-grokked the code here, but assuming I was correct 2 weeks
> ago, if you have the BUG_ON() there, you can get rid of the extra
> references.
>

Sure, but again, the overhead of having them in-place is negligible so
might as well just keep it.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel