Re: [Xen-devel] [PATCH 2/2] RFC: x86/mm: conditionally check page_lock/page_unlock ownership

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Fri, Apr 12, 2019 at 9:34 AM Jan Beulich <JBeulich@xxxxxxxx> wrote:
>
> >>> On 12.04.19 at 17:02, <tamas@xxxxxxxxxxxxx> wrote:
> > On Fri, Apr 12, 2019 at 8:39 AM Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >> >>> On 12.04.19 at 15:55, <tamas@xxxxxxxxxxxxx> wrote:
> >> > On Fri, Apr 12, 2019 at 6:01 AM Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >> >> >>> On 12.04.19 at 06:29, <tamas@xxxxxxxxxxxxx> wrote:
> >> >> > Patch cf4b30dca0a "Add debug code to detect illegal page_lock and 
> >> >> > put_page_type
> >> >> > ordering" added extra sanity checking to page_lock/page_unlock for 
> >> >> > debug builds
> >> >> > with the assumption that no hypervisor path ever locks two pages at 
> >> >> > once.
> >> >> >
> >> >> > This assumption doesn't hold during memory sharing.
> >> >>
> >> >> A fundamental question is - doesn't mem-sharing abuse page_lock()?
> >> >> I've never understood why it uses it, and back when it was introduced
> >> >> the function clearly wasn't meant to be used outside of PV memory
> >> >> management code.
> >> >
> >> > I have limited understanding of what the limitations are of page_lock
> >> > but here is the reasoning for taking the lock for two pages.
> >> > Mem_sharing takes the pages it wants to be sharable and assigns them
> >> > to dom_cow at least with one reference being held (since we just took
> >> > these pages from a live domain). Those pages may then be assigned to
> >> > several actual domains at the same time, and each domain could be
> >> > destroyed at any given point. There is no requirement that the
> >> > original domain still be alive since the owner of these pages is now
> >> > dom_cow. The backing pages owned by dom_cow only get dropped when the
> >> > reference count hits zero, ie. no actual domain actually uses those
> >> > pages anymore. This could happen if all domains unshare the gfn's that
> >> > use this page as the backing or if all domains that use them for
> >> > mem_sharing get destroyed. When we share pages, we want to make sure
> >> > those don't get dropped under our feet while we are in the middle of
> >> > this function. So we lock both pages.
> >>
> >> Thanks for the explanation, but I'm afraid this doesn't address
> >> my remark: You need _some_ kind of lock for this, but I was
> >> questioning whether using page_lock() is the right choice. From
> >> what you say, obtaining proper references to the page would
> >> seem what you actually want to avoid them disappearing
> >> behind your back.
> >
> > Yes, but beside the references ensuring the pages don't get dropped
> > there is also a bunch of book-keeping operations in mem_sharing (the
> > rmap bits) that also look like they need the locking to ensure no
> > concurrent updates happen for the same page. I guess we could
> > introduce a new lock but it would also need to be per-page, so to me
> > it looks like it would just duplicate what page_lock does already.
>
> Hmm, I guess I can't further comment on this, as I know nothing
> about how mem-sharing works, and hence why such per-page
> locking would be needed in the first place. Since you have a
> struct page_sharing_info * hanging off of struct page_info,
> perhaps that's where such a lock would belong?

Yea, I was looking at that too. Certainly would make sense to me. I'll
try go that route instead.

> >> page_lock() is used on PV page table pages to exclude multiple
> >> updates to the same page table happening at the same time.
> >> In a !PV build it shouldn't even be available.
> >
> > I don't see page_lock being encapsulated by CONFIG_PV blocks so that
> > should be fine.
>
> Well, it can't be because of its use by mem-sharing, or else
> we'd break the build.

OK :)

> >> > Also, I don't see why it would be fundamentally wrong to hold the
> >> > locks to two pages at once. Is this a problem with the lock
> >> > implementation itself?
> >>
> >> No, the implementation itself should be fine (but I also didn't say
> >> there's a problem locking two pages). Lock order might be an issue,
> >> but seem to be dealing with that fine (considering mem_sharing.c
> >> alone).
> >
> > Could you expand on the lock ordering requirement? I noticed
> > mem_sharing aquires the locks to the pages in increasing mfn order and
> > tried to decipher why that is done but I couldn't really make heads or
> > tails.
>
> As always with multiple locks, they need to be acquired in the
> same order by all parties. When no hierarchy gets established
> by code structure, consistent ordering needs to be enforced
> by other means, like comparing MFNs in the case here. You'll
> find something similar in grant table code, for example.

Makes sense.

Thanks,
Tamas

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