On 4/29/19 4:41 PM, Tamas K Lengyel wrote: > On Mon, Apr 29, 2019 at 9:01 AM Jan Beulich <jbeul...@suse.com> wrote: >> >>>>> On 26.04.19 at 19:21, <ta...@tklengyel.com> 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. :-) 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. -George _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
