Re: [Xen-devel] [PATCH] add page_get_owner_and_reference() related ASSERT()s

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.08.15 at 11:31, <tim@xxxxxxx> wrote:
> At 09:50 +0100 on 13 Aug (1439459451), Ian Campbell wrote:
>> On Fri, 2015-07-24 at 06:37 -0600, Jan Beulich wrote:
>> > The function shouldn't return NULL after having obtained a reference,
>> > or else the caller won't know to drop it.
>> 
>> > Also its result shouldn't be ignored - if calling code is certain that
>> > a page already has a non-zero refcount, it better ASSERT()s so.
>> 
>> How is page_get_owner_and_reference sure the reference count was not zero
>> on entry? (WRT the call to page_get_owner in page_get_owner_and_reference
>> itself) Or have I misunderstood the assertion being made here?
> 
> That path is fine -- if the count was zero, it will return NULL before
> trying to get the owner.  But I'm not convinced that the assertion is
> correct -- are there really no pages anywhere that have a recount but
> no owner?  What about, e.g. shadow pool pages or other uses of
> MEMF_no_owner?  If a guest can cause Xen to try to get_page() one of
> those, then we'd hit the new assertion.
> 
> How about unlikely(!owner) path that drops the taken ref instead?

That would be dead code - a page the ref count of wasn't zero
prior to the function taking an extra ref shouldn't be unowned.
If the new ASSERT() ever triggers we know we have a bug
elsewhere (which would be hidden if we did what you suggest).

> It'd be great to add a comment explaining the semantics of the call,
> since the uninformed reader might expect it to take a ref in all
> cases.

It would seem to me that this ought to be implicit from the
__must_check that the patch adds, as otherwise there would
be no (reasonable, i.e. leaving aside refcount overflow) way
to get back NULL here. But yes, if you think this is _too_
implicit, I could certainly add a comment to the declaration.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel