[PATCH] grant-table: don't set m2p override if kmap_ops is not set

[PATCH] grant-table: don't set m2p override if kmap_ops is not set

[Roger Pau Monne]

IMHO there's no reason to set a m2p override if the mapping is done in
kernel space, so only set the m2p override when kmap_ops is set.

When kmap_ops is not set, just set the correct p2m translation, this
avoids touching the m2p lock, reducing contention around it.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reported-by: Matt Wilson <msw@amazon.com>
Cc: Matt Wilson <msw@amazon.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: David Vrabel <david.vrabel@citrix.com>
Cc: Boris Ostrovsky <david.vrabel@citrix.com>
---
 drivers/xen/grant-table.c |   30 ++++++++++++++++++++++++++----
 1 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index 04cdeb8..ebf7126 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -909,8 +909,19 @@ int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
 		} else {
 			mfn = PFN_DOWN(map_ops[i].dev_bus_addr);
 		}
-		ret = m2p_add_override(mfn, pages[i], kmap_ops ?
-				       &kmap_ops[i] : NULL);
+
+		if (kmap_ops) {
+			ret = m2p_add_override(mfn, pages[i], &kmap_ops[i]);
+		} else {
+			/* No need to set the m2p override, just set p2m */
+			WARN_ON(PagePrivate(pages[i]));
+			SetPagePrivate(pages[i]);
+			set_page_private(pages[i], mfn);
+
+			if (unlikely(!set_phys_to_machine(page_to_pfn(pages[i]), FOREIGN_FRAME(mfn))))
+				ret = -ENOMEM;
+		}
+
 		if (ret)
 			return ret;
 	}
@@ -942,8 +953,19 @@ int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
 	}
 
 	for (i = 0; i < count; i++) {
-		ret = m2p_remove_override(pages[i], kmap_ops ?
-				       &kmap_ops[i] : NULL);
+		if (kmap_ops) {
+			ret = m2p_remove_override(pages[i], &kmap_ops[i]);
+		} else {
+			/* Remove p2m entry */
+			unsigned long mfn;
+
+			WARN_ON(!PagePrivate(pages[i]));
+			mfn = page_private(pages[i]);
+			ClearPagePrivate(pages[i]);
+
+			if (unlikely(!set_phys_to_machine(page_to_pfn(pages[i]), mfn)))
+				ret = -ENOMEM;
+		}
 		if (ret)
 			return ret;
 	}
-- 
1.7.7.5 (Apple Git-26)

Re: [PATCH] grant-table: don't set m2p override if kmap_ops is not set

[Roger Pau Monné]

On 05/11/13 12:24, Roger Pau Monne wrote:
> IMHO there's no reason to set a m2p override if the mapping is done in
> kernel space, so only set the m2p override when kmap_ops is set.
> 
> When kmap_ops is not set, just set the correct p2m translation, this
> avoids touching the m2p lock, reducing contention around it.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Reported-by: Matt Wilson <msw@amazon.com>
> Cc: Matt Wilson <msw@amazon.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: David Vrabel <david.vrabel@citrix.com>
> Cc: Boris Ostrovsky <david.vrabel@citrix.com>

Sorry Boris, I got your address wrong.

Re: [PATCH] grant-table: don't set m2p override if kmap_ops is not set

[David Vrabel]

On 05/11/13 11:24, Roger Pau Monne wrote:
> IMHO there's no reason to set a m2p override if the mapping is done in
> kernel space, so only set the m2p override when kmap_ops is set.

Can you provide a more detailed reasoning about why this is safe?

Did you consider other ways of improving scalability?  e.g., a
per-bucket read-write lock.

David

Re: [PATCH] grant-table: don't set m2p override if kmap_ops is not set

[Roger Pau Monné]

On 05/11/13 13:36, David Vrabel wrote:
> On 05/11/13 11:24, Roger Pau Monne wrote:
>> IMHO there's no reason to set a m2p override if the mapping is done in
>> kernel space, so only set the m2p override when kmap_ops is set.
> 
> Can you provide a more detailed reasoning about why this is safe?

To tell the truth, I don't understand why we need to use the m2p
override for kernel space only mappings, my understanding is that this
m2p override is needed for user space mappings only (where we actually
end up doing two mappings, one in kernel space and one in user space).
For kernel space I don't see why we need to do anything else than
setting the right p2m translation.

> Did you consider other ways of improving scalability?  e.g., a
> per-bucket read-write lock.

Not really, the main focus of this patch is to optimize kernel space
only mappings (like the ones done in blkback), so why go down this road
when we can actually remove the need for a lock at all in this case?

Re: [PATCH] grant-table: don't set m2p override if kmap_ops is not set

[Konrad Rzeszutek Wilk]

On Tue, Nov 05, 2013 at 03:47:08PM +0100, Roger Pau Monné wrote:
> On 05/11/13 13:36, David Vrabel wrote:
> > On 05/11/13 11:24, Roger Pau Monne wrote:
> >> IMHO there's no reason to set a m2p override if the mapping is done in
> >> kernel space, so only set the m2p override when kmap_ops is set.
> > 
> > Can you provide a more detailed reasoning about why this is safe?
> 
> To tell the truth, I don't understand why we need to use the m2p
> override for kernel space only mappings, my understanding is that this
> m2p override is needed for user space mappings only (where we actually
> end up doing two mappings, one in kernel space and one in user space).
> For kernel space I don't see why we need to do anything else than
> setting the right p2m translation.

We needed the m2p when doing DMA operations. As the driver would
want the bus address (so p2m) and then when unmapping the DMA we
only get the bus address  - so we needed to do a m2p lookup.

But perhaps there is a better way to do this in the SWIOTLB code.

> 
> > Did you consider other ways of improving scalability?  e.g., a
> > per-bucket read-write lock.
> 
> Not really, the main focus of this patch is to optimize kernel space
> only mappings (like the ones done in blkback), so why go down this road
> when we can actually remove the need for a lock at all in this case?

Re: [PATCH] grant-table: don't set m2p override if kmap_ops is not set

[Roger Pau Monné]

On 05/11/13 15:56, Konrad Rzeszutek Wilk wrote:
> On Tue, Nov 05, 2013 at 03:47:08PM +0100, Roger Pau Monné wrote:
>> On 05/11/13 13:36, David Vrabel wrote:
>>> On 05/11/13 11:24, Roger Pau Monne wrote:
>>>> IMHO there's no reason to set a m2p override if the mapping is done in
>>>> kernel space, so only set the m2p override when kmap_ops is set.
>>>
>>> Can you provide a more detailed reasoning about why this is safe?
>>
>> To tell the truth, I don't understand why we need to use the m2p
>> override for kernel space only mappings, my understanding is that this
>> m2p override is needed for user space mappings only (where we actually
>> end up doing two mappings, one in kernel space and one in user space).
>> For kernel space I don't see why we need to do anything else than
>> setting the right p2m translation.
> 
> We needed the m2p when doing DMA operations. As the driver would
> want the bus address (so p2m) and then when unmapping the DMA we
> only get the bus address  - so we needed to do a m2p lookup.

OK, we need a m2p (that we already have in machine_to_phys_mapping),
what I don't understand is why we need the m2p override.

With this patch the m2p is correctly provided by mfn_to_pfn_no_overrides
just by using the machine_to_phys_mapping array, why we need to add an
override?

Re: [Xen-devel] [PATCH] grant-table: don't set m2p override if kmap_ops is not set

[Ian Campbell]

On Tue, 2013-11-05 at 16:01 +0100, Roger Pau Monné wrote:
> On 05/11/13 15:56, Konrad Rzeszutek Wilk wrote:
> > On Tue, Nov 05, 2013 at 03:47:08PM +0100, Roger Pau Monné wrote:
> >> On 05/11/13 13:36, David Vrabel wrote:
> >>> On 05/11/13 11:24, Roger Pau Monne wrote:
> >>>> IMHO there's no reason to set a m2p override if the mapping is done in
> >>>> kernel space, so only set the m2p override when kmap_ops is set.
> >>>
> >>> Can you provide a more detailed reasoning about why this is safe?
> >>
> >> To tell the truth, I don't understand why we need to use the m2p
> >> override for kernel space only mappings, my understanding is that this
> >> m2p override is needed for user space mappings only (where we actually
> >> end up doing two mappings, one in kernel space and one in user space).
> >> For kernel space I don't see why we need to do anything else than
> >> setting the right p2m translation.
> > 
> > We needed the m2p when doing DMA operations. As the driver would
> > want the bus address (so p2m) and then when unmapping the DMA we
> > only get the bus address  - so we needed to do a m2p lookup.
> 
> OK, we need a m2p (that we already have in machine_to_phys_mapping),
> what I don't understand is why we need the m2p override.

The m2p is a host global table.

For a foreign page grant mapped into the current domain the m2p will
give you the foreign (owner) domain's p from the m, not the local one.

Ian.

Re: [Xen-devel] [PATCH] grant-table: don't set m2p override if kmap_ops is not set

[Roger Pau Monné]

On 05/11/13 16:08, Ian Campbell wrote:
> On Tue, 2013-11-05 at 16:01 +0100, Roger Pau Monné wrote:
>> On 05/11/13 15:56, Konrad Rzeszutek Wilk wrote:
>>> On Tue, Nov 05, 2013 at 03:47:08PM +0100, Roger Pau Monné wrote:
>>>> On 05/11/13 13:36, David Vrabel wrote:
>>>>> On 05/11/13 11:24, Roger Pau Monne wrote:
>>>>>> IMHO there's no reason to set a m2p override if the mapping is done in
>>>>>> kernel space, so only set the m2p override when kmap_ops is set.
>>>>>
>>>>> Can you provide a more detailed reasoning about why this is safe?
>>>>
>>>> To tell the truth, I don't understand why we need to use the m2p
>>>> override for kernel space only mappings, my understanding is that this
>>>> m2p override is needed for user space mappings only (where we actually
>>>> end up doing two mappings, one in kernel space and one in user space).
>>>> For kernel space I don't see why we need to do anything else than
>>>> setting the right p2m translation.
>>>
>>> We needed the m2p when doing DMA operations. As the driver would
>>> want the bus address (so p2m) and then when unmapping the DMA we
>>> only get the bus address  - so we needed to do a m2p lookup.
>>
>> OK, we need a m2p (that we already have in machine_to_phys_mapping),
>> what I don't understand is why we need the m2p override.
> 
> The m2p is a host global table.
> 
> For a foreign page grant mapped into the current domain the m2p will
> give you the foreign (owner) domain's p from the m, not the local one.

Yes, you are completely right, then I have to figure out why blkback
works fine with this patch applied (or at least it seems to work fine).

Re: [Xen-devel] [PATCH] grant-table: don't set m2p override if kmap_ops is not set

[Matt Wilson]

On Tue, Nov 05, 2013 at 05:03:58PM +0100, Roger Pau Monné wrote:
> On 05/11/13 16:08, Ian Campbell wrote:
> > On Tue, 2013-11-05 at 16:01 +0100, Roger Pau Monné wrote:
> >> On 05/11/13 15:56, Konrad Rzeszutek Wilk wrote:
> >>> On Tue, Nov 05, 2013 at 03:47:08PM +0100, Roger Pau Monné wrote:
> >>>> On 05/11/13 13:36, David Vrabel wrote:
> >>>>> On 05/11/13 11:24, Roger Pau Monne wrote:
> >>>>>> IMHO there's no reason to set a m2p override if the mapping is done in
> >>>>>> kernel space, so only set the m2p override when kmap_ops is set.
> >>>>>
> >>>>> Can you provide a more detailed reasoning about why this is safe?
> >>>>
> >>>> To tell the truth, I don't understand why we need to use the m2p
> >>>> override for kernel space only mappings, my understanding is that this
> >>>> m2p override is needed for user space mappings only (where we actually
> >>>> end up doing two mappings, one in kernel space and one in user space).
> >>>> For kernel space I don't see why we need to do anything else than
> >>>> setting the right p2m translation.
> >>>
> >>> We needed the m2p when doing DMA operations. As the driver would
> >>> want the bus address (so p2m) and then when unmapping the DMA we
> >>> only get the bus address  - so we needed to do a m2p lookup.
> >>
> >> OK, we need a m2p (that we already have in machine_to_phys_mapping),
> >> what I don't understand is why we need the m2p override.
> > 
> > The m2p is a host global table.
> > 
> > For a foreign page grant mapped into the current domain the m2p will
> > give you the foreign (owner) domain's p from the m, not the local one.
> 
> Yes, you are completely right, then I have to figure out why blkback
> works fine with this patch applied (or at least it seems to work fine).

blkback also works for me when testing a similar patch. I'm still
confused. One thing with your proposed patch: I'm not sure that you're
putting back the correct mfn.

Adding Anthony to the thread.

--msw

Re: [Xen-devel] [PATCH] grant-table: don't set m2p override if kmap_ops is not set

[Anthony Liguori]

Matt Wilson <msw@linux.com> writes:

> On Tue, Nov 05, 2013 at 05:03:58PM +0100, Roger Pau Monné wrote:
>> On 05/11/13 16:08, Ian Campbell wrote:
>> > On Tue, 2013-11-05 at 16:01 +0100, Roger Pau Monné wrote:
>> >> On 05/11/13 15:56, Konrad Rzeszutek Wilk wrote:
>> >>> On Tue, Nov 05, 2013 at 03:47:08PM +0100, Roger Pau Monné wrote:
>> >>>> On 05/11/13 13:36, David Vrabel wrote:
>> >>>>> On 05/11/13 11:24, Roger Pau Monne wrote:
>> >>>>>> IMHO there's no reason to set a m2p override if the mapping is done in
>> >>>>>> kernel space, so only set the m2p override when kmap_ops is set.
>> >>>>>
>> >>>>> Can you provide a more detailed reasoning about why this is safe?
>> >>>>
>> >>>> To tell the truth, I don't understand why we need to use the m2p
>> >>>> override for kernel space only mappings, my understanding is that this
>> >>>> m2p override is needed for user space mappings only (where we actually
>> >>>> end up doing two mappings, one in kernel space and one in user space).
>> >>>> For kernel space I don't see why we need to do anything else than
>> >>>> setting the right p2m translation.
>> >>>
>> >>> We needed the m2p when doing DMA operations. As the driver would
>> >>> want the bus address (so p2m) and then when unmapping the DMA we
>> >>> only get the bus address  - so we needed to do a m2p lookup.
>> >>
>> >> OK, we need a m2p (that we already have in machine_to_phys_mapping),
>> >> what I don't understand is why we need the m2p override.
>> > 
>> > The m2p is a host global table.
>> > 
>> > For a foreign page grant mapped into the current domain the m2p will
>> > give you the foreign (owner) domain's p from the m, not the local one.
>> 
>> Yes, you are completely right, then I have to figure out why blkback
>> works fine with this patch applied (or at least it seems to work fine).
>
> blkback also works for me when testing a similar patch. I'm still
> confused. One thing with your proposed patch: I'm not sure that you're
> putting back the correct mfn.

It's perfectly fine to store a foreign pfn in the m2p table.  The m2p
override table is used by the grant device to allow a reverse lookup of
the real mfn to a pfn even if it's foreign.

blkback doesn't actually need this though.  This was introduced in:

commit 5dc03639cc903f887931831d69895facb5260f4b
Author: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date:   Tue Mar 1 16:46:45 2011 -0500

    xen/blkback: Utilize the M2P override mechanism for GNTMAP_host_map

Purely as an optimization.  In practice though due to lock contention it
slows things down.

I think an alternative would be to use a read/write lock instead of just
a spinlock since it's the read path that is the most hot.

I haven't tested that yet though.

Regards,

Anthony Liguori

>
> Adding Anthony to the thread.
>
> --msw

Re: [Xen-devel] [PATCH] grant-table: don't set m2p override if kmap_ops is not set

[Konrad Rzeszutek Wilk]

On Tue, Nov 05, 2013 at 12:53:17PM -0800, Anthony Liguori wrote:
> Matt Wilson <msw@linux.com> writes:
> 
> > On Tue, Nov 05, 2013 at 05:03:58PM +0100, Roger Pau Monné wrote:
> >> On 05/11/13 16:08, Ian Campbell wrote:
> >> > On Tue, 2013-11-05 at 16:01 +0100, Roger Pau Monné wrote:
> >> >> On 05/11/13 15:56, Konrad Rzeszutek Wilk wrote:
> >> >>> On Tue, Nov 05, 2013 at 03:47:08PM +0100, Roger Pau Monné wrote:
> >> >>>> On 05/11/13 13:36, David Vrabel wrote:
> >> >>>>> On 05/11/13 11:24, Roger Pau Monne wrote:
> >> >>>>>> IMHO there's no reason to set a m2p override if the mapping is done in
> >> >>>>>> kernel space, so only set the m2p override when kmap_ops is set.
> >> >>>>>
> >> >>>>> Can you provide a more detailed reasoning about why this is safe?
> >> >>>>
> >> >>>> To tell the truth, I don't understand why we need to use the m2p
> >> >>>> override for kernel space only mappings, my understanding is that this
> >> >>>> m2p override is needed for user space mappings only (where we actually
> >> >>>> end up doing two mappings, one in kernel space and one in user space).
> >> >>>> For kernel space I don't see why we need to do anything else than
> >> >>>> setting the right p2m translation.
> >> >>>
> >> >>> We needed the m2p when doing DMA operations. As the driver would
> >> >>> want the bus address (so p2m) and then when unmapping the DMA we
> >> >>> only get the bus address  - so we needed to do a m2p lookup.
> >> >>
> >> >> OK, we need a m2p (that we already have in machine_to_phys_mapping),
> >> >> what I don't understand is why we need the m2p override.
> >> > 
> >> > The m2p is a host global table.
> >> > 
> >> > For a foreign page grant mapped into the current domain the m2p will
> >> > give you the foreign (owner) domain's p from the m, not the local one.
> >> 
> >> Yes, you are completely right, then I have to figure out why blkback
> >> works fine with this patch applied (or at least it seems to work fine).
> >
> > blkback also works for me when testing a similar patch. I'm still
> > confused. One thing with your proposed patch: I'm not sure that you're
> > putting back the correct mfn.
> 
> It's perfectly fine to store a foreign pfn in the m2p table.  The m2p
> override table is used by the grant device to allow a reverse lookup of
> the real mfn to a pfn even if it's foreign.
> 
> blkback doesn't actually need this though.  This was introduced in:
> 
> commit 5dc03639cc903f887931831d69895facb5260f4b
> Author: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date:   Tue Mar 1 16:46:45 2011 -0500
> 
>     xen/blkback: Utilize the M2P override mechanism for GNTMAP_host_map
> 
> Purely as an optimization.  In practice though due to lock contention it
> slows things down.
> 
> I think an alternative would be to use a read/write lock instead of just
> a spinlock since it's the read path that is the most hot.

The m2p hash table can also be expanded to lower the contention.

> 
> I haven't tested that yet though.

Looking forward to your patches :-)
> 
> Regards,
> 
> Anthony Liguori
> 
> >
> > Adding Anthony to the thread.
> >
> > --msw

Re: [Xen-devel] [PATCH] grant-table: don't set m2p override if kmap_ops is not set

[Anthony Liguori]

On Tue, Nov 5, 2013 at 1:16 PM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Tue, Nov 05, 2013 at 12:53:17PM -0800, Anthony Liguori wrote:
>> Matt Wilson <msw@linux.com> writes:
>>
>> > On Tue, Nov 05, 2013 at 05:03:58PM +0100, Roger Pau Monné wrote:
>> >> On 05/11/13 16:08, Ian Campbell wrote:
>> >> > On Tue, 2013-11-05 at 16:01 +0100, Roger Pau Monné wrote:
>> >> >> On 05/11/13 15:56, Konrad Rzeszutek Wilk wrote:
>> >> >>> On Tue, Nov 05, 2013 at 03:47:08PM +0100, Roger Pau Monné wrote:
>> >> >>>> On 05/11/13 13:36, David Vrabel wrote:
>> >> >>>>> On 05/11/13 11:24, Roger Pau Monne wrote:
>> >> >>>>>> IMHO there's no reason to set a m2p override if the mapping is done in
>> >> >>>>>> kernel space, so only set the m2p override when kmap_ops is set.
>> >> >>>>>
>> >> >>>>> Can you provide a more detailed reasoning about why this is safe?
>> >> >>>>
>> >> >>>> To tell the truth, I don't understand why we need to use the m2p
>> >> >>>> override for kernel space only mappings, my understanding is that this
>> >> >>>> m2p override is needed for user space mappings only (where we actually
>> >> >>>> end up doing two mappings, one in kernel space and one in user space).
>> >> >>>> For kernel space I don't see why we need to do anything else than
>> >> >>>> setting the right p2m translation.
>> >> >>>
>> >> >>> We needed the m2p when doing DMA operations. As the driver would
>> >> >>> want the bus address (so p2m) and then when unmapping the DMA we
>> >> >>> only get the bus address  - so we needed to do a m2p lookup.
>> >> >>
>> >> >> OK, we need a m2p (that we already have in machine_to_phys_mapping),
>> >> >> what I don't understand is why we need the m2p override.
>> >> >
>> >> > The m2p is a host global table.
>> >> >
>> >> > For a foreign page grant mapped into the current domain the m2p will
>> >> > give you the foreign (owner) domain's p from the m, not the local one.
>> >>
>> >> Yes, you are completely right, then I have to figure out why blkback
>> >> works fine with this patch applied (or at least it seems to work fine).
>> >
>> > blkback also works for me when testing a similar patch. I'm still
>> > confused. One thing with your proposed patch: I'm not sure that you're
>> > putting back the correct mfn.
>>
>> It's perfectly fine to store a foreign pfn in the m2p table.  The m2p
>> override table is used by the grant device to allow a reverse lookup of
>> the real mfn to a pfn even if it's foreign.
>>
>> blkback doesn't actually need this though.  This was introduced in:
>>
>> commit 5dc03639cc903f887931831d69895facb5260f4b
>> Author: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Date:   Tue Mar 1 16:46:45 2011 -0500
>>
>>     xen/blkback: Utilize the M2P override mechanism for GNTMAP_host_map
>>
>> Purely as an optimization.  In practice though due to lock contention it
>> slows things down.
>>
>> I think an alternative would be to use a read/write lock instead of just
>> a spinlock since it's the read path that is the most hot.
>
> The m2p hash table can also be expanded to lower the contention.

That was the first thing I tried :-)

The issue isn't lookup cost as much as the number of acquisitions and
the cost of adding/removing entries.

>> I haven't tested that yet though.
>
> Looking forward to your patches :-)

I'm really just being thorough in suggesting the rwlock.  I actually
think that not using the m2p override table is the right long term
fix.

Regards,

Anthony Liguori

>>
>> Regards,
>>
>> Anthony Liguori
>>
>> >
>> > Adding Anthony to the thread.
>> >
>> > --msw

Re: [Xen-devel] [PATCH] grant-table: don't set m2p override if kmap_ops is not set

[Matt Wilson]

On Tue, Nov 05, 2013 at 02:09:53PM -0800, Anthony Liguori wrote:
> On Tue, Nov 5, 2013 at 1:16 PM, Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com> wrote:
> > On Tue, Nov 05, 2013 at 12:53:17PM -0800, Anthony Liguori wrote:
> >> Matt Wilson <msw@linux.com> writes:
> >>
> >> > On Tue, Nov 05, 2013 at 05:03:58PM +0100, Roger Pau Monné wrote:
> >> >> On 05/11/13 16:08, Ian Campbell wrote:
> >> >> > On Tue, 2013-11-05 at 16:01 +0100, Roger Pau Monné wrote:
> >> >> >> On 05/11/13 15:56, Konrad Rzeszutek Wilk wrote:
> >> >> >>> On Tue, Nov 05, 2013 at 03:47:08PM +0100, Roger Pau Monné wrote:
> >> >> >>>> On 05/11/13 13:36, David Vrabel wrote:
> >> >> >>>>> On 05/11/13 11:24, Roger Pau Monne wrote:
> >> >> >>>>>> IMHO there's no reason to set a m2p override if the mapping is done in
> >> >> >>>>>> kernel space, so only set the m2p override when kmap_ops is set.
> >> >> >>>>>
> >> >> >>>>> Can you provide a more detailed reasoning about why this is safe?
> >> >> >>>>
> >> >> >>>> To tell the truth, I don't understand why we need to use the m2p
> >> >> >>>> override for kernel space only mappings, my understanding is that this
> >> >> >>>> m2p override is needed for user space mappings only (where we actually
> >> >> >>>> end up doing two mappings, one in kernel space and one in user space).
> >> >> >>>> For kernel space I don't see why we need to do anything else than
> >> >> >>>> setting the right p2m translation.
> >> >> >>>
> >> >> >>> We needed the m2p when doing DMA operations. As the driver would
> >> >> >>> want the bus address (so p2m) and then when unmapping the DMA we
> >> >> >>> only get the bus address  - so we needed to do a m2p lookup.
> >> >> >>
> >> >> >> OK, we need a m2p (that we already have in machine_to_phys_mapping),
> >> >> >> what I don't understand is why we need the m2p override.
> >> >> >
> >> >> > The m2p is a host global table.
> >> >> >
> >> >> > For a foreign page grant mapped into the current domain the m2p will
> >> >> > give you the foreign (owner) domain's p from the m, not the local one.
> >> >>
> >> >> Yes, you are completely right, then I have to figure out why blkback
> >> >> works fine with this patch applied (or at least it seems to work fine).
> >> >
> >> > blkback also works for me when testing a similar patch. I'm still
> >> > confused. One thing with your proposed patch: I'm not sure that you're
> >> > putting back the correct mfn.
> >>
> >> It's perfectly fine to store a foreign pfn in the m2p table.  The m2p
> >> override table is used by the grant device to allow a reverse lookup of
> >> the real mfn to a pfn even if it's foreign.
> >>
> >> blkback doesn't actually need this though.  This was introduced in:
> >>
> >> commit 5dc03639cc903f887931831d69895facb5260f4b
> >> Author: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >> Date:   Tue Mar 1 16:46:45 2011 -0500
> >>
> >>     xen/blkback: Utilize the M2P override mechanism for GNTMAP_host_map
> >>
> >> Purely as an optimization.  In practice though due to lock contention it
> >> slows things down.
> >>
> >> I think an alternative would be to use a read/write lock instead of just
> >> a spinlock since it's the read path that is the most hot.
> >
> > The m2p hash table can also be expanded to lower the contention.
> 
> That was the first thing I tried :-)
> 
> The issue isn't lookup cost as much as the number of acquisitions and
> the cost of adding/removing entries.
> 
> >> I haven't tested that yet though.
> >
> > Looking forward to your patches :-)
> 
> I'm really just being thorough in suggesting the rwlock.  I actually
> think that not using the m2p override table is the right long term
> fix.

I tried a rwlock a while back. It didn't help much because we actually
have a problem with concurrent adds/removes as much as lookups.

--msw

Re: [Xen-devel] [PATCH] grant-table: don't set m2p override if kmap_ops is not set

[Ian Campbell]

On Tue, 2013-11-05 at 12:53 -0800, Anthony Liguori wrote:
> >> Yes, you are completely right, then I have to figure out why blkback
> >> works fine with this patch applied (or at least it seems to work fine).
> >
> > blkback also works for me when testing a similar patch. I'm still
> > confused. One thing with your proposed patch: I'm not sure that you're
> > putting back the correct mfn.

As long as it is unique and owned by the local domain I guess it doesn't
matter which mfn goes back? It's going to get given back to the generic
allocator so the content is irrelevant? (all speculation without looking
at blkback)

> It's perfectly fine to store a foreign pfn in the m2p table.

No, it's not. The m2p is host global and maps mfns to the pfns in the
owning domain. A domain which grant maps a foreign mfn into its address
space doesn't get to update the m2p for that mfn. Perhaps you were
thinking of the p2m?

Depending on how an OS mapping the foreign MFN does things it may well
be accurate to say that having a foreign pfn in the m2p table is
harmless, in as much as it will never try and use the m2p for foreign
pages for anything real, but it depends on the implementation of the
particular OS.

>   The m2p
> override table is used by the grant device to allow a reverse lookup of
> the real mfn to a pfn even if it's foreign.

Classic Xen used an array of struct page * overrides in the struct vma
for this sort of use case (e.g. in blktap). That obviously cannot fly
for pvops...

Ian.

Re: [Xen-devel] [PATCH] grant-table: don't set m2p override if kmap_ops is not set

[Anthony Liguori]

Ian Campbell <Ian.Campbell@citrix.com> writes:

> On Tue, 2013-11-05 at 12:53 -0800, Anthony Liguori wrote:
>> >> Yes, you are completely right, then I have to figure out why blkback
>> >> works fine with this patch applied (or at least it seems to work fine).
>> >
>> > blkback also works for me when testing a similar patch. I'm still
>> > confused. One thing with your proposed patch: I'm not sure that you're
>> > putting back the correct mfn.
>
> As long as it is unique and owned by the local domain I guess it doesn't
> matter which mfn goes back? It's going to get given back to the generic
> allocator so the content is irrelevant? (all speculation without looking
> at blkback)


Hrm, so m2p_add_override already does set_phys_to_machine(pfn,
FOREIGN_FRAME(mfn)) so this patch doesn't add anything that isn't
already there.  It's just removing the additional to the m2p override
table.

So what happens when the MFN isn't in the m2p override table?  It's
treated as a foreign PFN and m2p lookups fail.  But why is this a
problem for blkback?

>> It's perfectly fine to store a foreign pfn in the m2p table.
>
> No, it's not. The m2p is host global and maps mfns to the pfns in the
> owning domain. A domain which grant maps a foreign mfn into its address
> space doesn't get to update the m2p for that mfn. Perhaps you were
> thinking of the p2m?

Yes, I typoed that bit.

> Depending on how an OS mapping the foreign MFN does things it may well
> be accurate to say that having a foreign pfn in the m2p table is
> harmless, in as much as it will never try and use the m2p for foreign
> pages for anything real, but it depends on the implementation of the
> particular OS.

Note that this patch isn't doing anything to the m2p table.

>>   The m2p
>> override table is used by the grant device to allow a reverse lookup of
>> the real mfn to a pfn even if it's foreign.
>
> Classic Xen used an array of struct page * overrides in the struct vma
> for this sort of use case (e.g. in blktap). That obviously cannot fly
> for pvops...

I don't think the usage from blkback precludes using the gntdev either.
It just so happens that the m2p_add_override does extra work but I don't
think it actually benefits blkback in any way.

Am I missing something?

Regards,

Anthony Liguori

>
> Ian.

Re: [Xen-devel] [PATCH] grant-table: don't set m2p override if kmap_ops is not set

[David Vrabel]

On 05/11/13 20:53, Anthony Liguori wrote:
> Matt Wilson <msw@linux.com> writes:
> 
>> On Tue, Nov 05, 2013 at 05:03:58PM +0100, Roger Pau Monné wrote:
>>> On 05/11/13 16:08, Ian Campbell wrote:
>>>> On Tue, 2013-11-05 at 16:01 +0100, Roger Pau Monné wrote:
>>>>> On 05/11/13 15:56, Konrad Rzeszutek Wilk wrote:
>>>>>> On Tue, Nov 05, 2013 at 03:47:08PM +0100, Roger Pau Monné wrote:
>>>>>>> On 05/11/13 13:36, David Vrabel wrote:
>>>>>>>> On 05/11/13 11:24, Roger Pau Monne wrote:
>>>>>>>>> IMHO there's no reason to set a m2p override if the mapping is done in
>>>>>>>>> kernel space, so only set the m2p override when kmap_ops is set.
>>>>>>>>
>>>>>>>> Can you provide a more detailed reasoning about why this is safe?
>>>>>>>
>>>>>>> To tell the truth, I don't understand why we need to use the m2p
>>>>>>> override for kernel space only mappings, my understanding is that this
>>>>>>> m2p override is needed for user space mappings only (where we actually
>>>>>>> end up doing two mappings, one in kernel space and one in user space).
>>>>>>> For kernel space I don't see why we need to do anything else than
>>>>>>> setting the right p2m translation.
>>>>>>
>>>>>> We needed the m2p when doing DMA operations. As the driver would
>>>>>> want the bus address (so p2m) and then when unmapping the DMA we
>>>>>> only get the bus address  - so we needed to do a m2p lookup.

No. A m2p lookup should not be needed for DMA unmap.

We only need the p2m lookup on DMA map to get the bus address -- there's
should be nothing to do on the unmap.

However, there is dma_mark_clean(phys_to_virt(phys), size) call in
xen_unmap_single() which would require the m2p lookup but this call is
not necessary as dma_mark_clean() is a no-op for anything except ia64
and we do not care about ia64.

>>>>> OK, we need a m2p (that we already have in machine_to_phys_mapping),
>>>>> what I don't understand is why we need the m2p override.
>>>>
>>>> The m2p is a host global table.
>>>>
>>>> For a foreign page grant mapped into the current domain the m2p will
>>>> give you the foreign (owner) domain's p from the m, not the local one.
>>>
>>> Yes, you are completely right, then I have to figure out why blkback
>>> works fine with this patch applied (or at least it seems to work fine).
>>
>> blkback also works for me when testing a similar patch. I'm still
>> confused. One thing with your proposed patch: I'm not sure that you're
>> putting back the correct mfn.

Matt, Anthony, I presume you have profiling results or performance data
that support this proposed change?  Can you provide them?

> It's perfectly fine to store a foreign pfn in the m2p table.  The m2p
> override table is used by the grant device to allow a reverse lookup of
> the real mfn to a pfn even if it's foreign.
> 
> blkback doesn't actually need this though.  This was introduced in:
> 
> commit 5dc03639cc903f887931831d69895facb5260f4b
> Author: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date:   Tue Mar 1 16:46:45 2011 -0500
> 
>     xen/blkback: Utilize the M2P override mechanism for GNTMAP_host_map
> 
> Purely as an optimization.  In practice though due to lock contention it
> slows things down.

The full changeset description for this change doesn't make sense to me.

    xen/blkback: Utilize the M2P override mechanism for GNTMAP_host_map

    Instead of doing copy grants lets do mapping grants using
    the M2P(and P2M) override mechanism.

As all it is doing is replacing set_phys_to_machine() calls with
m2p_add_override().

David

Re: [Xen-devel] [PATCH] grant-table: don't set m2p override if kmap_ops is not set

[Matt Wilson]

On Wed, Nov 06, 2013 at 11:34:27AM +0000, David Vrabel wrote:
[...]
> 
> Matt, Anthony, I presume you have profiling results or performance data
> that support this proposed change?  Can you provide them?

I've measured 10-20% performance improvement in configurations where:

1) dom0 has a moderate number of vCPUs doing blkback work
2) domU has 32 vCPUs
3) 24 configured VBDs without persistent grant support
4) some lock contention in grant table hypercalls has been alleviated

More specific results are still in the works.

> > It's perfectly fine to store a foreign pfn in the m2p table.  The m2p
> > override table is used by the grant device to allow a reverse lookup of
> > the real mfn to a pfn even if it's foreign.
> > 
> > blkback doesn't actually need this though.  This was introduced in:
> > 
> > commit 5dc03639cc903f887931831d69895facb5260f4b
> > Author: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Date:   Tue Mar 1 16:46:45 2011 -0500
> > 
> >     xen/blkback: Utilize the M2P override mechanism for GNTMAP_host_map
> > 
> > Purely as an optimization.  In practice though due to lock contention it
> > slows things down.
> 
> The full changeset description for this change doesn't make sense to me.
> 
>     xen/blkback: Utilize the M2P override mechanism for GNTMAP_host_map
> 
>     Instead of doing copy grants lets do mapping grants using
>     the M2P(and P2M) override mechanism.
> 
> As all it is doing is replacing set_phys_to_machine() calls with
> m2p_add_override().

Indeed, since this had nothing to do with copying. We were confused
also. Konrad?

--msw

Re: [Xen-devel] [PATCH] grant-table: don't set m2p override if kmap_ops is not set

[Konrad Rzeszutek Wilk]

On Wed, Nov 06, 2013 at 09:59:34AM -0800, Matt Wilson wrote:
> On Wed, Nov 06, 2013 at 11:34:27AM +0000, David Vrabel wrote:
> [...]
> > 
> > Matt, Anthony, I presume you have profiling results or performance data
> > that support this proposed change?  Can you provide them?
> 
> I've measured 10-20% performance improvement in configurations where:
> 
> 1) dom0 has a moderate number of vCPUs doing blkback work
> 2) domU has 32 vCPUs
> 3) 24 configured VBDs without persistent grant support
> 4) some lock contention in grant table hypercalls has been alleviated
> 
> More specific results are still in the works.
> 
> > > It's perfectly fine to store a foreign pfn in the m2p table.  The m2p
> > > override table is used by the grant device to allow a reverse lookup of
> > > the real mfn to a pfn even if it's foreign.
> > > 
> > > blkback doesn't actually need this though.  This was introduced in:
> > > 
> > > commit 5dc03639cc903f887931831d69895facb5260f4b
> > > Author: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > > Date:   Tue Mar 1 16:46:45 2011 -0500
> > > 
> > >     xen/blkback: Utilize the M2P override mechanism for GNTMAP_host_map
> > > 
> > > Purely as an optimization.  In practice though due to lock contention it
> > > slows things down.
> > 
> > The full changeset description for this change doesn't make sense to me.
> > 
> >     xen/blkback: Utilize the M2P override mechanism for GNTMAP_host_map
> > 
> >     Instead of doing copy grants lets do mapping grants using
> >     the M2P(and P2M) override mechanism.
> > 
> > As all it is doing is replacing set_phys_to_machine() calls with
> > m2p_add_override().
> 
> Indeed, since this had nothing to do with copying. We were confused
> also. Konrad?

<confused as well>

2011? Hm, I really don't remember. It does not look to be needed.

I think that back in 2011 the m2p override mechanism was much simpler
and was just a wrapper around set_phys_to_machine with a non-lock
write in the m2p override.

The only concern I had, which David had looked at and I did here
too was the DMA unmap operation, but as you can see from the
writeup - it is not warranted.

I think that the set_phys_to_machine is the way to go then
and ditching the m2p_override. Two questions remain:

 - How does this work when block back is running in an HVM domain?

 - Stefano, do we need to worry about the crazy scenario of
   dom0 using xen-blkfront and xen-blkback inside itself to fetch
   bits of data from QEMU?

?
Here is how it interacts with SWIOTLB:


1) Backend gets the foreign MFN from the grant call. Calls m2p_add_override
   which calls set_phys_to_machine and also adds the MFN on the m2p_overrides.
   Takes a lock.
2). Backend submit_bio, it ends up in AHCI driver. Said driver does dma_map_sg
3). We call xen_swiotlb_map_sg_attrs, which does:
	a) pfn_to_mfn, gets from the P2M the MFN | FOREIGN_FRAME_BIT. Strips
	   the FOREIGN_FRAME_BIT. Returns MFN.
	b). Setups up the sg->dma_address
	
    But it also might setup an bounce buffer in case the AHCI can't reach
    the MFN >> PAGE_SHIFT. At which point we just save the virtual
    address of the page handed to us in the IOTLB.

4). .. ahci driver does it cmd, once it is done it calls 'dma_unmap_sg'
   which ends up in xen_swiotlb_unmap_sg_attrs and we call xen_unmap_single
   which calls:

393         phys_addr_t paddr = xen_bus_to_phys(dev_addr);                          
   which ends up doing (in mfn_to_pfn):

112         pfn = mfn_to_pfn_no_overrides(mfn);                                    

   lookup in the M2P array. We find an MFN and then we check the P2M:

113         if (get_phys_to_machine(pfn) != mfn) {                                  

   Since the MFN hasn't changed (it is still the 'local' one instead
   of the forign one - since we can't modify the M2P) we end up
   with p2m(pfn) != mfn. As the p2m(pfn) ends up giving us the 'foreign'
   MFN value and the m2p(pfn) ends up giving us the 'local' MFN.

   So then we consult the override:
114                 /*                                                              
115                  * If this appears to be a foreign mfn (because the pfn         
116                  * doesn't map back to the mfn), then check the local override  
117                  * table to see if there's a better pfn to use.                 
118                  *                                                              
119                  * m2p_find_override_pfn returns ~0 if it doesn't find anything.
120                  */                                                             
121                 pfn = m2p_find_override_pfn(mfn, ~0);              

   And we find it the MFN of the foreign domain, return it back to the SWIOTLB
   as physical address (so mfn << PAGE_SHIFT) so it can a) either ignore it,
   or b) if the bounce buffer ended up being used - ignore it too. As the
   bounce buffer mechanism ends up using the original stashed virtual address
   and bounces between the bounce buffer and foreign owned virtual address.

   The dma_mark_clean is a nop.

Re: [Xen-devel] [PATCH] grant-table: don't set m2p override if kmap_ops is not set

[Stefano Stabellini]

On Wed, 6 Nov 2013, Konrad Rzeszutek Wilk wrote:
> On Wed, Nov 06, 2013 at 09:59:34AM -0800, Matt Wilson wrote:
> > On Wed, Nov 06, 2013 at 11:34:27AM +0000, David Vrabel wrote:
> > [...]
> > > 
> > > Matt, Anthony, I presume you have profiling results or performance data
> > > that support this proposed change?  Can you provide them?
> > 
> > I've measured 10-20% performance improvement in configurations where:
> > 
> > 1) dom0 has a moderate number of vCPUs doing blkback work
> > 2) domU has 32 vCPUs
> > 3) 24 configured VBDs without persistent grant support
> > 4) some lock contention in grant table hypercalls has been alleviated
> > 
> > More specific results are still in the works.
> > 
> > > > It's perfectly fine to store a foreign pfn in the m2p table.  The m2p
> > > > override table is used by the grant device to allow a reverse lookup of
> > > > the real mfn to a pfn even if it's foreign.
> > > > 
> > > > blkback doesn't actually need this though.  This was introduced in:
> > > > 
> > > > commit 5dc03639cc903f887931831d69895facb5260f4b
> > > > Author: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > > > Date:   Tue Mar 1 16:46:45 2011 -0500
> > > > 
> > > >     xen/blkback: Utilize the M2P override mechanism for GNTMAP_host_map
> > > > 
> > > > Purely as an optimization.  In practice though due to lock contention it
> > > > slows things down.
> > > 
> > > The full changeset description for this change doesn't make sense to me.
> > > 
> > >     xen/blkback: Utilize the M2P override mechanism for GNTMAP_host_map
> > > 
> > >     Instead of doing copy grants lets do mapping grants using
> > >     the M2P(and P2M) override mechanism.
> > > 
> > > As all it is doing is replacing set_phys_to_machine() calls with
> > > m2p_add_override().
> > 
> > Indeed, since this had nothing to do with copying. We were confused
> > also. Konrad?
> 
> <confused as well>
> 
> 2011? Hm, I really don't remember. It does not look to be needed.
> 
> I think that back in 2011 the m2p override mechanism was much simpler
> and was just a wrapper around set_phys_to_machine with a non-lock
> write in the m2p override.
> 
> The only concern I had, which David had looked at and I did here
> too was the DMA unmap operation, but as you can see from the
> writeup - it is not warranted.
> 
> I think that the set_phys_to_machine is the way to go then
> and ditching the m2p_override. Two questions remain:
> 
>  - How does this work when block back is running in an HVM domain?

HVM guests don't use the m2p_override, so no problems.


>  - Stefano, do we need to worry about the crazy scenario of
>    dom0 using xen-blkfront and xen-blkback inside itself to fetch
>    bits of data from QEMU?

It's not a crazy scenario, in fact it is enabled by default on modern
systems :-)
In any case it wouldn't be affected by changes that only impact the "no
kmap_ops" case, because QEMU uses the gntdev device, therefore kmap_ops
would be set.


> ?
> Here is how it interacts with SWIOTLB:
> 
> 
> 1) Backend gets the foreign MFN from the grant call. Calls m2p_add_override
>    which calls set_phys_to_machine and also adds the MFN on the m2p_overrides.
>    Takes a lock.
> 2). Backend submit_bio, it ends up in AHCI driver. Said driver does dma_map_sg
> 3). We call xen_swiotlb_map_sg_attrs, which does:
> 	a) pfn_to_mfn, gets from the P2M the MFN | FOREIGN_FRAME_BIT. Strips
> 	   the FOREIGN_FRAME_BIT. Returns MFN.
> 	b). Setups up the sg->dma_address
> 	
>     But it also might setup an bounce buffer in case the AHCI can't reach
>     the MFN >> PAGE_SHIFT. At which point we just save the virtual
>     address of the page handed to us in the IOTLB.
> 
> 4). .. ahci driver does it cmd, once it is done it calls 'dma_unmap_sg'
>    which ends up in xen_swiotlb_unmap_sg_attrs and we call xen_unmap_single
>    which calls:
> 
> 393         phys_addr_t paddr = xen_bus_to_phys(dev_addr);                          
>    which ends up doing (in mfn_to_pfn):
> 
> 112         pfn = mfn_to_pfn_no_overrides(mfn);                                    
> 
>    lookup in the M2P array. We find an MFN and then we check the P2M:
> 
> 113         if (get_phys_to_machine(pfn) != mfn) {                                  
> 
>    Since the MFN hasn't changed (it is still the 'local' one instead
>    of the forign one - since we can't modify the M2P) we end up
>    with p2m(pfn) != mfn. As the p2m(pfn) ends up giving us the 'foreign'
>    MFN value and the m2p(pfn) ends up giving us the 'local' MFN.
> 
>    So then we consult the override:
> 114                 /*                                                              
> 115                  * If this appears to be a foreign mfn (because the pfn         
> 116                  * doesn't map back to the mfn), then check the local override  
> 117                  * table to see if there's a better pfn to use.                 
> 118                  *                                                              
> 119                  * m2p_find_override_pfn returns ~0 if it doesn't find anything.
> 120                  */                                                             
> 121                 pfn = m2p_find_override_pfn(mfn, ~0);              
> 
>    And we find it the MFN of the foreign domain, return it back to the SWIOTLB
>    as physical address (so mfn << PAGE_SHIFT) so it can a) either ignore it,
>    or b) if the bounce buffer ended up being used - ignore it too. As the
>    bounce buffer mechanism ends up using the original stashed virtual address
>    and bounces between the bounce buffer and foreign owned virtual address.
> 
>    The dma_mark_clean is a nop.


Removing the m2p_add_override from blkback would cause xen_bus_to_phys
to return ~0 on x86 if the swiotlb is not bouncing the dma request.
In that case all the following operations in xen_unmap_single would
fail, but nothing actually needs to be done on x86.
If the request is bounced everything works as usual.

The situation is different on ARM: when dma requests are not bounced we
need to call the native ARM unmap_page dma_op for cache coherency
reasons. But on ARM we don't use the m2p_override, we opt instead for a
couple of rbtrees updated at every call of set_phys_to_machine.
Effectively we would still have something equivalent to the
m2p_override.

Therefore I think that we can avoid the m2p_add_override call from
blkback but I would like a very clear comment in the code to explain the
situation.