From: Julien Grall <julien.grall@linaro.org>
To: Stefano Stabellini <stefano.stabellini@eu.citrix.com>,
Zoltan Kiss <zoltan.kiss@citrix.com>
Cc: jonathan.davies@citrix.com, wei.liu2@citrix.com,
ian.campbell@citrix.com, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org
Subject: Re: [Xen-devel] [PATCH v6] xen/grant-table: Avoid m2p_override during mapping
Date: Sun, 02 Feb 2014 10:29:58 +0000 [thread overview]
Message-ID: <52EE1E26.2040308@linaro.org> (raw)
In-Reply-To: <alpine.DEB.2.02.1401232333280.15917@kaball.uk.xensource.com>
On 23/01/14 23:34, Stefano Stabellini wrote:
> On Thu, 23 Jan 2014, Zoltan Kiss wrote:
>> The grant mapping API does m2p_override unnecessarily: only gntdev needs it,
>> for blkback and future netback patches it just cause a lock contention, as
>> those pages never go to userspace. Therefore this series does the following:
>> - the original functions were renamed to __gnttab_[un]map_refs, with a new
>> parameter m2p_override
>> - based on m2p_override either they follow the original behaviour, or just set
>> the private flag and call set_phys_to_machine
>> - gnttab_[un]map_refs are now a wrapper to call __gnttab_[un]map_refs with
>> m2p_override false
>> - a new function gnttab_[un]map_refs_userspace provides the old behaviour
>>
>> It also removes a stray space from page.h and change ret to 0 if
>> XENFEAT_auto_translated_physmap, as that is the only possible return value
>> there.
>>
>> v2:
>> - move the storing of the old mfn in page->index to gnttab_map_refs
>> - move the function header update to a separate patch
>>
>> v3:
>> - a new approach to retain old behaviour where it needed
>> - squash the patches into one
>>
>> v4:
>> - move out the common bits from m2p* functions, and pass pfn/mfn as parameter
>> - clear page->private before doing anything with the page, so m2p_find_override
>> won't race with this
>>
>> v5:
>> - change return value handling in __gnttab_[un]map_refs
>> - remove a stray space in page.h
>> - add detail why ret = 0 now at some places
>>
>> v6:
>> - don't pass pfn to m2p* functions, just get it locally
>>
>> Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com>
>> Suggested-by: David Vrabel <david.vrabel@citrix.com>
>
> Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Hello,
This patch is breaking Linux compilation on ARM:
drivers/xen/grant-table.c: In function ‘__gnttab_map_refs’:
drivers/xen/grant-table.c:989:3: error: implicit declaration of function ‘FOREIGN_FRAME’ [-Werror=implicit-function-declaration]
if (unlikely(!set_phys_to_machine(pfn, FOREIGN_FRAME(mfn)))) {
^
drivers/xen/grant-table.c: In function ‘__gnttab_unmap_refs’:
drivers/xen/grant-table.c:1054:3: error: implicit declaration of function ‘get_phys_to_machine’ [-Werror=implicit-function-declaration]
mfn = get_phys_to_machine(pfn);
^
drivers/xen/grant-table.c:1055:43: error: ‘FOREIGN_FRAME_BIT’ undeclared (first use in this function)
if (mfn == INVALID_P2M_ENTRY || !(mfn & FOREIGN_FRAME_BIT)) {
^
drivers/xen/grant-table.c:1055:43: note: each undeclared identifier is reported only once for each function it appears in
drivers/xen/grant-table.c:1068:9: error: too many arguments to function ‘m2p_remove_override’
mfn);
^
In file included from include/xen/page.h:4:0,
from drivers/xen/grant-table.c:48:
/local/home/julien/works/midway/linux/arch/arm/include/asm/xen/page.h:106:19: note: declared here
static inline int m2p_remove_override(struct page *page, bool clear_pte)
^
cc1: some warnings being treated as errors
>
>
>
>> arch/x86/include/asm/xen/page.h | 5 +-
>> arch/x86/xen/p2m.c | 17 +------
>> drivers/block/xen-blkback/blkback.c | 15 +++---
>> drivers/xen/gntdev.c | 13 +++--
>> drivers/xen/grant-table.c | 89 ++++++++++++++++++++++++++++++-----
>> include/xen/grant_table.h | 8 +++-
>> 6 files changed, 101 insertions(+), 46 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/xen/page.h b/arch/x86/include/asm/xen/page.h
>> index b913915..ce47243 100644
>> --- a/arch/x86/include/asm/xen/page.h
>> +++ b/arch/x86/include/asm/xen/page.h
>> @@ -52,7 +52,8 @@ extern unsigned long set_phys_range_identity(unsigned long pfn_s,
>> extern int m2p_add_override(unsigned long mfn, struct page *page,
>> struct gnttab_map_grant_ref *kmap_op);
>> extern int m2p_remove_override(struct page *page,
>> - struct gnttab_map_grant_ref *kmap_op);
>> + struct gnttab_map_grant_ref *kmap_op,
>> + unsigned long mfn);
>> extern struct page *m2p_find_override(unsigned long mfn);
>> extern unsigned long m2p_find_override_pfn(unsigned long mfn, unsigned long pfn);
>>
>> @@ -121,7 +122,7 @@ static inline unsigned long mfn_to_pfn(unsigned long mfn)
>> pfn = m2p_find_override_pfn(mfn, ~0);
>> }
>>
>> - /*
>> + /*
>> * pfn is ~0 if there are no entries in the m2p for mfn or if the
>> * entry doesn't map back to the mfn and m2p_override doesn't have a
>> * valid entry for it.
>> diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
>> index 2ae8699..bd4724b 100644
>> --- a/arch/x86/xen/p2m.c
>> +++ b/arch/x86/xen/p2m.c
>> @@ -888,13 +888,6 @@ int m2p_add_override(unsigned long mfn, struct page *page,
>> "m2p_add_override: pfn %lx not mapped", pfn))
>> return -EINVAL;
>> }
>> - WARN_ON(PagePrivate(page));
>> - SetPagePrivate(page);
>> - set_page_private(page, mfn);
>> - page->index = pfn_to_mfn(pfn);
>> -
>> - if (unlikely(!set_phys_to_machine(pfn, FOREIGN_FRAME(mfn))))
>> - return -ENOMEM;
>>
>> if (kmap_op != NULL) {
>> if (!PageHighMem(page)) {
>> @@ -933,19 +926,16 @@ int m2p_add_override(unsigned long mfn, struct page *page,
>> }
>> EXPORT_SYMBOL_GPL(m2p_add_override);
>> int m2p_remove_override(struct page *page,
>> - struct gnttab_map_grant_ref *kmap_op)
>> + struct gnttab_map_grant_ref *kmap_op,
>> + unsigned long mfn)
>> {
>> unsigned long flags;
>> - unsigned long mfn;
>> unsigned long pfn;
>> unsigned long uninitialized_var(address);
>> unsigned level;
>> pte_t *ptep = NULL;
>>
>> pfn = page_to_pfn(page);
>> - mfn = get_phys_to_machine(pfn);
>> - if (mfn == INVALID_P2M_ENTRY || !(mfn & FOREIGN_FRAME_BIT))
>> - return -EINVAL;
>>
>> if (!PageHighMem(page)) {
>> address = (unsigned long)__va(pfn << PAGE_SHIFT);
>> @@ -959,10 +949,7 @@ int m2p_remove_override(struct page *page,
>> spin_lock_irqsave(&m2p_override_lock, flags);
>> list_del(&page->lru);
>> spin_unlock_irqrestore(&m2p_override_lock, flags);
>> - WARN_ON(!PagePrivate(page));
>> - ClearPagePrivate(page);
>>
>> - set_phys_to_machine(pfn, page->index);
>> if (kmap_op != NULL) {
>> if (!PageHighMem(page)) {
>> struct multicall_space mcs;
>> diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
>> index 6620b73..875025f 100644
>> --- a/drivers/block/xen-blkback/blkback.c
>> +++ b/drivers/block/xen-blkback/blkback.c
>> @@ -285,8 +285,7 @@ static void free_persistent_gnts(struct xen_blkif *blkif, struct rb_root *root,
>>
>> if (++segs_to_unmap == BLKIF_MAX_SEGMENTS_PER_REQUEST ||
>> !rb_next(&persistent_gnt->node)) {
>> - ret = gnttab_unmap_refs(unmap, NULL, pages,
>> - segs_to_unmap);
>> + ret = gnttab_unmap_refs(unmap, pages, segs_to_unmap);
>> BUG_ON(ret);
>> put_free_pages(blkif, pages, segs_to_unmap);
>> segs_to_unmap = 0;
>> @@ -321,8 +320,7 @@ static void unmap_purged_grants(struct work_struct *work)
>> pages[segs_to_unmap] = persistent_gnt->page;
>>
>> if (++segs_to_unmap == BLKIF_MAX_SEGMENTS_PER_REQUEST) {
>> - ret = gnttab_unmap_refs(unmap, NULL, pages,
>> - segs_to_unmap);
>> + ret = gnttab_unmap_refs(unmap, pages, segs_to_unmap);
>> BUG_ON(ret);
>> put_free_pages(blkif, pages, segs_to_unmap);
>> segs_to_unmap = 0;
>> @@ -330,7 +328,7 @@ static void unmap_purged_grants(struct work_struct *work)
>> kfree(persistent_gnt);
>> }
>> if (segs_to_unmap > 0) {
>> - ret = gnttab_unmap_refs(unmap, NULL, pages, segs_to_unmap);
>> + ret = gnttab_unmap_refs(unmap, pages, segs_to_unmap);
>> BUG_ON(ret);
>> put_free_pages(blkif, pages, segs_to_unmap);
>> }
>> @@ -670,15 +668,14 @@ static void xen_blkbk_unmap(struct xen_blkif *blkif,
>> GNTMAP_host_map, pages[i]->handle);
>> pages[i]->handle = BLKBACK_INVALID_HANDLE;
>> if (++invcount == BLKIF_MAX_SEGMENTS_PER_REQUEST) {
>> - ret = gnttab_unmap_refs(unmap, NULL, unmap_pages,
>> - invcount);
>> + ret = gnttab_unmap_refs(unmap, unmap_pages, invcount);
>> BUG_ON(ret);
>> put_free_pages(blkif, unmap_pages, invcount);
>> invcount = 0;
>> }
>> }
>> if (invcount) {
>> - ret = gnttab_unmap_refs(unmap, NULL, unmap_pages, invcount);
>> + ret = gnttab_unmap_refs(unmap, unmap_pages, invcount);
>> BUG_ON(ret);
>> put_free_pages(blkif, unmap_pages, invcount);
>> }
>> @@ -740,7 +737,7 @@ again:
>> }
>>
>> if (segs_to_map) {
>> - ret = gnttab_map_refs(map, NULL, pages_to_gnt, segs_to_map);
>> + ret = gnttab_map_refs(map, pages_to_gnt, segs_to_map);
>> BUG_ON(ret);
>> }
>>
>> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
>> index e41c79c..e652c0e 100644
>> --- a/drivers/xen/gntdev.c
>> +++ b/drivers/xen/gntdev.c
>> @@ -284,8 +284,10 @@ static int map_grant_pages(struct grant_map *map)
>> }
>>
>> pr_debug("map %d+%d\n", map->index, map->count);
>> - err = gnttab_map_refs(map->map_ops, use_ptemod ? map->kmap_ops : NULL,
>> - map->pages, map->count);
>> + err = gnttab_map_refs_userspace(map->map_ops,
>> + use_ptemod ? map->kmap_ops : NULL,
>> + map->pages,
>> + map->count);
>> if (err)
>> return err;
>>
>> @@ -315,9 +317,10 @@ static int __unmap_grant_pages(struct grant_map *map, int offset, int pages)
>> }
>> }
>>
>> - err = gnttab_unmap_refs(map->unmap_ops + offset,
>> - use_ptemod ? map->kmap_ops + offset : NULL, map->pages + offset,
>> - pages);
>> + err = gnttab_unmap_refs_userspace(map->unmap_ops + offset,
>> + use_ptemod ? map->kmap_ops + offset : NULL,
>> + map->pages + offset,
>> + pages);
>> if (err)
>> return err;
>>
>> diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
>> index aa846a4..e4ddfeb 100644
>> --- a/drivers/xen/grant-table.c
>> +++ b/drivers/xen/grant-table.c
>> @@ -880,15 +880,17 @@ void gnttab_batch_copy(struct gnttab_copy *batch, unsigned count)
>> }
>> EXPORT_SYMBOL_GPL(gnttab_batch_copy);
>>
>> -int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
>> +int __gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
>> struct gnttab_map_grant_ref *kmap_ops,
>> - struct page **pages, unsigned int count)
>> + struct page **pages, unsigned int count,
>> + bool m2p_override)
>> {
>> int i, ret;
>> bool lazy = false;
>> pte_t *pte;
>> - unsigned long mfn;
>> + unsigned long mfn, pfn;
>>
>> + BUG_ON(kmap_ops && !m2p_override);
>> ret = HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, map_ops, count);
>> if (ret)
>> return ret;
>> @@ -907,10 +909,12 @@ int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
>> set_phys_to_machine(map_ops[i].host_addr >> PAGE_SHIFT,
>> map_ops[i].dev_bus_addr >> PAGE_SHIFT);
>> }
>> - return ret;
>> + return 0;
>> }
>>
>> - if (!in_interrupt() && paravirt_get_lazy_mode() == PARAVIRT_LAZY_NONE) {
>> + if (m2p_override &&
>> + !in_interrupt() &&
>> + paravirt_get_lazy_mode() == PARAVIRT_LAZY_NONE) {
>> arch_enter_lazy_mmu_mode();
>> lazy = true;
>> }
>> @@ -927,8 +931,20 @@ 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);
>> + pfn = page_to_pfn(pages[i]);
>> +
>> + WARN_ON(PagePrivate(pages[i]));
>> + SetPagePrivate(pages[i]);
>> + set_page_private(pages[i], mfn);
>> +
>> + pages[i]->index = pfn_to_mfn(pfn);
>> + if (unlikely(!set_phys_to_machine(pfn, FOREIGN_FRAME(mfn)))) {
>> + ret = -ENOMEM;
>> + goto out;
>> + }
>> + if (m2p_override)
>> + ret = m2p_add_override(mfn, pages[i], kmap_ops ?
>> + &kmap_ops[i] : NULL);
>> if (ret)
>> goto out;
>> }
>> @@ -939,15 +955,32 @@ int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
>>
>> return ret;
>> }
>> +
>> +int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
>> + struct page **pages, unsigned int count)
>> +{
>> + return __gnttab_map_refs(map_ops, NULL, pages, count, false);
>> +}
>> EXPORT_SYMBOL_GPL(gnttab_map_refs);
>>
>> -int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
>> +int gnttab_map_refs_userspace(struct gnttab_map_grant_ref *map_ops,
>> + struct gnttab_map_grant_ref *kmap_ops,
>> + struct page **pages, unsigned int count)
>> +{
>> + return __gnttab_map_refs(map_ops, kmap_ops, pages, count, true);
>> +}
>> +EXPORT_SYMBOL_GPL(gnttab_map_refs_userspace);
>> +
>> +int __gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
>> struct gnttab_map_grant_ref *kmap_ops,
>> - struct page **pages, unsigned int count)
>> + struct page **pages, unsigned int count,
>> + bool m2p_override)
>> {
>> int i, ret;
>> bool lazy = false;
>> + unsigned long pfn, mfn;
>>
>> + BUG_ON(kmap_ops && !m2p_override);
>> ret = HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, unmap_ops, count);
>> if (ret)
>> return ret;
>> @@ -958,17 +991,33 @@ int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
>> set_phys_to_machine(unmap_ops[i].host_addr >> PAGE_SHIFT,
>> INVALID_P2M_ENTRY);
>> }
>> - return ret;
>> + return 0;
>> }
>>
>> - if (!in_interrupt() && paravirt_get_lazy_mode() == PARAVIRT_LAZY_NONE) {
>> + if (m2p_override &&
>> + !in_interrupt() &&
>> + paravirt_get_lazy_mode() == PARAVIRT_LAZY_NONE) {
>> arch_enter_lazy_mmu_mode();
>> lazy = true;
>> }
>>
>> for (i = 0; i < count; i++) {
>> - ret = m2p_remove_override(pages[i], kmap_ops ?
>> - &kmap_ops[i] : NULL);
>> + pfn = page_to_pfn(pages[i]);
>> + mfn = get_phys_to_machine(pfn);
>> + if (mfn == INVALID_P2M_ENTRY || !(mfn & FOREIGN_FRAME_BIT)) {
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> + set_page_private(pages[i], INVALID_P2M_ENTRY);
>> + WARN_ON(!PagePrivate(pages[i]));
>> + ClearPagePrivate(pages[i]);
>> + set_phys_to_machine(pfn, pages[i]->index);
>> + if (m2p_override)
>> + ret = m2p_remove_override(pages[i],
>> + kmap_ops ?
>> + &kmap_ops[i] : NULL,
>> + mfn);
>> if (ret)
>> goto out;
>> }
>> @@ -979,8 +1028,22 @@ int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
>>
>> return ret;
>> }
>> +
>> +int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *map_ops,
>> + struct page **pages, unsigned int count)
>> +{
>> + return __gnttab_unmap_refs(map_ops, NULL, pages, count, false);
>> +}
>> EXPORT_SYMBOL_GPL(gnttab_unmap_refs);
>>
>> +int gnttab_unmap_refs_userspace(struct gnttab_unmap_grant_ref *map_ops,
>> + struct gnttab_map_grant_ref *kmap_ops,
>> + struct page **pages, unsigned int count)
>> +{
>> + return __gnttab_unmap_refs(map_ops, kmap_ops, pages, count, true);
>> +}
>> +EXPORT_SYMBOL_GPL(gnttab_unmap_refs_userspace);
>> +
>> static unsigned nr_status_frames(unsigned nr_grant_frames)
>> {
>> BUG_ON(grefs_per_grant_frame == 0);
>> diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
>> index 694dcaf..9a919b1 100644
>> --- a/include/xen/grant_table.h
>> +++ b/include/xen/grant_table.h
>> @@ -184,11 +184,15 @@ unsigned int gnttab_max_grant_frames(void);
>> #define gnttab_map_vaddr(map) ((void *)(map.host_virt_addr))
>>
>> int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
>> - struct gnttab_map_grant_ref *kmap_ops,
>> struct page **pages, unsigned int count);
>> +int gnttab_map_refs_userspace(struct gnttab_map_grant_ref *map_ops,
>> + struct gnttab_map_grant_ref *kmap_ops,
>> + struct page **pages, unsigned int count);
>> int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
>> - struct gnttab_map_grant_ref *kunmap_ops,
>> struct page **pages, unsigned int count);
>> +int gnttab_unmap_refs_userspace(struct gnttab_unmap_grant_ref *unmap_ops,
>> + struct gnttab_map_grant_ref *kunmap_ops,
>> + struct page **pages, unsigned int count);
>>
>> /* Perform a batch of grant map/copy operations. Retry every batch slot
>> * for which the hypervisor returns GNTST_eagain. This is typically due
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
>>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
--
Julien Grall
next prev parent reply other threads:[~2014-02-02 10:30 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-01-23 21:23 [PATCH v6] xen/grant-table: Avoid m2p_override during mapping Zoltan Kiss
2014-01-23 23:34 ` Stefano Stabellini
2014-02-02 10:29 ` Julien Grall [this message]
2014-02-02 18:52 ` [Xen-devel] " Zoltan Kiss
2014-02-03 10:57 ` David Vrabel
2014-02-03 11:13 ` Stefano Stabellini
2014-02-03 11:50 ` Konrad Rzeszutek Wilk
2014-02-03 13:27 ` Zoltan Kiss
2014-01-24 5:48 ` Matt Wilson
2014-01-24 10:57 ` [Xen-devel] " David Vrabel
2014-01-24 12:04 ` Stefano Stabellini
2014-01-24 17:20 ` Zoltan Kiss
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=52EE1E26.2040308@linaro.org \
--to=julien.grall@linaro.org \
--cc=ian.campbell@citrix.com \
--cc=jonathan.davies@citrix.com \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=stefano.stabellini@eu.citrix.com \
--cc=wei.liu2@citrix.com \
--cc=xen-devel@lists.xenproject.org \
--cc=zoltan.kiss@citrix.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.