From: Kirti Wankhede <kwankhede@nvidia.com>
To: Alex Williamson <alex.williamson@redhat.com>
Cc: <pbonzini@redhat.com>, <kraxel@redhat.com>, <cjia@nvidia.com>,
<qemu-devel@nongnu.org>, <kvm@vger.kernel.org>,
<kevin.tian@intel.com>, <jike.song@intel.com>,
<bjsdjshi@linux.vnet.ibm.com>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v13 11/22] vfio iommu: Add blocking notifier to notify DMA_UNMAP
Date: Wed, 16 Nov 2016 09:13:37 +0530 [thread overview]
Message-ID: <473d10c5-b2cb-e976-a923-b5add22bcde6@nvidia.com> (raw)
In-Reply-To: <20161115202522.16d1990e@t450s.home>
On 11/16/2016 8:55 AM, Alex Williamson wrote:
> On Tue, 15 Nov 2016 20:16:12 -0700
> Alex Williamson <alex.williamson@redhat.com> wrote:
>
>> On Wed, 16 Nov 2016 08:16:15 +0530
>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>>
>>> On 11/16/2016 3:49 AM, Alex Williamson wrote:
>>>> On Tue, 15 Nov 2016 20:59:54 +0530
>>>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>>>>
>>> ...
>>>
>>>>> @@ -854,7 +857,28 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
>>>>> */
>>>>> if (dma->task->mm != current->mm)
>>>>> break;
>>>>> +
>>>>> unmapped += dma->size;
>>>>> +
>>>>> + if (iommu->external_domain && !RB_EMPTY_ROOT(&dma->pfn_list)) {
>>>>> + struct vfio_iommu_type1_dma_unmap nb_unmap;
>>>>> +
>>>>> + nb_unmap.iova = dma->iova;
>>>>> + nb_unmap.size = dma->size;
>>>>> +
>>>>> + /*
>>>>> + * Notifier callback would call vfio_unpin_pages() which
>>>>> + * would acquire iommu->lock. Release lock here and
>>>>> + * reacquire it again.
>>>>> + */
>>>>> + mutex_unlock(&iommu->lock);
>>>>> + blocking_notifier_call_chain(&iommu->notifier,
>>>>> + VFIO_IOMMU_NOTIFY_DMA_UNMAP,
>>>>> + &nb_unmap);
>>>>> + mutex_lock(&iommu->lock);
>>>>> + if (WARN_ON(!RB_EMPTY_ROOT(&dma->pfn_list)))
>>>>> + break;
>>>>> + }
>>>>
>>>>
>>>> Why exactly do we need to notify per vfio_dma rather than per unmap
>>>> request? If we do the latter we can send the notify first, limiting us
>>>> to races where a page is pinned between the notify and the locking,
>>>> whereas here, even our dma pointer is suspect once we re-acquire the
>>>> lock, we don't technically know if another unmap could have removed
>>>> that already. Perhaps something like this (untested):
>>>>
>>>
>>> There are checks to validate unmap request, like v2 check and who is
>>> calling unmap and is it allowed for that task to unmap. Before these
>>> checks its not sure that unmap region range which asked for would be
>>> unmapped all. Notify call should be at the place where its sure that the
>>> range provided to notify call is definitely going to be removed. My
>>> change do that.
>>
>> Ok, but that does solve the problem. What about this (untested):
>
> s/does/does not/
>
> BTW, I like how the retries here fill the gap in my previous proposal
> where we could still race re-pinning. We've given it an honest shot or
> someone is not participating if we've retried 10 times. I don't
> understand why the test for iommu->external_domain was there, clearly
> if the list is not empty, we need to notify. Thanks,
>
Ok. Retry is good to give a chance to unpin all. But is it really
required to use BUG_ON() that would panic the host. I think WARN_ON
should be fine and then when container is closed or when the last group
is removed from the container, vfio_iommu_type1_release() is called and
we have a chance to unpin it all.
Thanks,
Kirti
> Alex
>
>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>> index ee9a680..50cafdf 100644
>> --- a/drivers/vfio/vfio_iommu_type1.c
>> +++ b/drivers/vfio/vfio_iommu_type1.c
>> @@ -782,9 +782,9 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
>> struct vfio_iommu_type1_dma_unmap *unmap)
>> {
>> uint64_t mask;
>> - struct vfio_dma *dma;
>> + struct vfio_dma *dma, *dma_last = NULL;
>> size_t unmapped = 0;
>> - int ret = 0;
>> + int ret = 0, retries;
>>
>> mask = ((uint64_t)1 << __ffs(vfio_pgsize_bitmap(iommu))) - 1;
>>
>> @@ -794,7 +794,7 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
>> return -EINVAL;
>>
>> WARN_ON(mask & PAGE_MASK);
>> -
>> +again:
>> mutex_lock(&iommu->lock);
>>
>> /*
>> @@ -851,11 +851,16 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
>> if (dma->task->mm != current->mm)
>> break;
>>
>> - unmapped += dma->size;
>> -
>> - if (iommu->external_domain && !RB_EMPTY_ROOT(&dma->pfn_list)) {
>> + if (!RB_EMPTY_ROOT(&dma->pfn_list)) {
>> struct vfio_iommu_type1_dma_unmap nb_unmap;
>>
>> + if (dma_last == dma) {
>> + BUG_ON(++retries > 10);
>> + } else {
>> + dma_last = dma;
>> + retries = 0;
>> + }
>> +
>> nb_unmap.iova = dma->iova;
>> nb_unmap.size = dma->size;
>>
>> @@ -868,11 +873,11 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
>> blocking_notifier_call_chain(&iommu->notifier,
>> VFIO_IOMMU_NOTIFY_DMA_UNMAP,
>> &nb_unmap);
>> - mutex_lock(&iommu->lock);
>> - if (WARN_ON(!RB_EMPTY_ROOT(&dma->pfn_list)))
>> - break;
>> + goto again:
>> }
>> + unmapped += dma->size;
>> vfio_remove_dma(iommu, dma);
>> +
>> }
>>
>> unlock:
>
next prev parent reply other threads:[~2016-11-16 3:43 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-11-15 15:29 [PATCH v13 00/22] Add Mediated device support Kirti Wankhede
2016-11-15 15:29 ` [PATCH v13 01/22] vfio: Mediated device Core driver Kirti Wankhede
2016-11-15 15:29 ` [PATCH v13 02/22] vfio: VFIO based driver for Mediated devices Kirti Wankhede
[not found] ` <20161116022958.GA5531@bjsdjshi@linux.vnet.ibm.com>
2016-11-16 15:05 ` Kirti Wankhede
2016-11-15 15:29 ` [PATCH v13 03/22] vfio: Rearrange functions to get vfio_group from dev Kirti Wankhede
2016-11-15 15:29 ` [PATCH v13 04/22] vfio: Common function to increment container_users Kirti Wankhede
2016-11-15 15:29 ` [PATCH v13 05/22] vfio iommu: Added pin and unpin callback functions to vfio_iommu_driver_ops Kirti Wankhede
[not found] ` <20161116030328.GB5531@bjsdjshi@linux.vnet.ibm.com>
2016-11-16 15:06 ` Kirti Wankhede
2016-11-15 15:29 ` [PATCH v13 06/22] vfio iommu type1: Update arguments of vfio_lock_acct Kirti Wankhede
2016-11-15 15:29 ` [PATCH v13 07/22] vfio iommu type1: Update argument of vaddr_get_pfn() Kirti Wankhede
2016-11-15 15:29 ` [PATCH v13 08/22] vfio iommu type1: Add find_iommu_group() function Kirti Wankhede
2016-11-15 15:29 ` [PATCH v13 09/22] vfio iommu type1: Add task structure to vfio_dma Kirti Wankhede
[not found] ` <20161116060628.GC5531@bjsdjshi@linux.vnet.ibm.com>
2016-11-16 15:11 ` Kirti Wankhede
2016-11-15 15:29 ` [PATCH v13 10/22] vfio iommu type1: Add support for mediated devices Kirti Wankhede
2016-11-15 20:54 ` Alex Williamson
2016-11-15 15:29 ` [PATCH v13 11/22] vfio iommu: Add blocking notifier to notify DMA_UNMAP Kirti Wankhede
2016-11-15 22:19 ` Alex Williamson
2016-11-16 2:46 ` Kirti Wankhede
2016-11-16 3:16 ` Alex Williamson
2016-11-16 3:25 ` Alex Williamson
2016-11-16 3:43 ` Kirti Wankhede [this message]
2016-11-16 3:58 ` Alex Williamson
2016-11-16 4:16 ` Kirti Wankhede
2016-11-16 4:36 ` Alex Williamson
2016-11-16 15:22 ` Kirti Wankhede
2016-11-15 15:29 ` [PATCH v13 12/22] vfio: Add notifier callback to parent's ops structure of mdev Kirti Wankhede
[not found] ` <20161116063759.GD5531@bjsdjshi@linux.vnet.ibm.com>
2016-11-16 15:17 ` Kirti Wankhede
2016-11-15 15:29 ` [PATCH v13 13/22] vfio: Introduce common function to add capabilities Kirti Wankhede
2016-11-15 15:29 ` [PATCH v13 14/22] vfio_pci: Update vfio_pci to use vfio_info_add_capability() Kirti Wankhede
2016-11-15 15:29 ` [PATCH v13 15/22] vfio: Introduce vfio_set_irqs_validate_and_prepare() Kirti Wankhede
2016-11-15 15:29 ` [PATCH v13 16/22] vfio_pci: Updated to use vfio_set_irqs_validate_and_prepare() Kirti Wankhede
2016-11-15 15:30 ` [PATCH v13 17/22] vfio_platform: " Kirti Wankhede
2016-11-15 15:30 ` [PATCH v13 18/22] vfio: Define device_api strings Kirti Wankhede
2016-11-15 15:30 ` [PATCH v13 19/22] docs: Add Documentation for Mediated devices Kirti Wankhede
2016-11-15 15:30 ` [PATCH v13 20/22] docs: Sysfs ABI for mediated device framework Kirti Wankhede
2016-11-15 15:30 ` [PATCH v13 21/22] docs: Sample driver to demonstrate how to use Mediated " Kirti Wankhede
2016-11-15 15:30 ` [PATCH v13 22/22] MAINTAINERS: Add entry VFIO based Mediated device drivers Kirti Wankhede
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=473d10c5-b2cb-e976-a923-b5add22bcde6@nvidia.com \
--to=kwankhede@nvidia.com \
--cc=alex.williamson@redhat.com \
--cc=bjsdjshi@linux.vnet.ibm.com \
--cc=cjia@nvidia.com \
--cc=jike.song@intel.com \
--cc=kevin.tian@intel.com \
--cc=kraxel@redhat.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).