[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

>>> 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.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.