* [PATCH] hisi_acc_vfio_pci: Remove the deferred_reset logic
@ 2024-02-29 9:11 Shameer Kolothum
2024-02-29 22:05 ` Brett Creeley
` (4 more replies)
0 siblings, 5 replies; 8+ messages in thread
From: Shameer Kolothum @ 2024-02-29 9:11 UTC (permalink / raw
To: kvm
Cc: alex.williamson, jgg, yishaih, kevin.tian, linuxarm, liulongfang,
bcreeley
The deferred_reset logic was added to vfio migration drivers to prevent
a circular locking dependency with respect to mm_lock and state mutex.
This is mainly because of the copy_to/from_user() functions(which takes
mm_lock) invoked under state mutex. But for HiSilicon driver, the only
place where we now hold the state mutex for copy_to_user is during the
PRE_COPY IOCTL. So for pre_copy, release the lock as soon as we have
updated the data and perform copy_to_user without state mutex. By this,
we can get rid of the deferred_reset logic.
Link: https://lore.kernel.org/kvm/20240220132459.GM13330@nvidia.com/
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
.../vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 48 +++++--------------
.../vfio/pci/hisilicon/hisi_acc_vfio_pci.h | 6 +--
2 files changed, 14 insertions(+), 40 deletions(-)
diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
index 4d27465c8f1a..9a3e97108ace 100644
--- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
+++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
@@ -630,25 +630,11 @@ static void hisi_acc_vf_disable_fds(struct hisi_acc_vf_core_device *hisi_acc_vde
}
}
-/*
- * This function is called in all state_mutex unlock cases to
- * handle a 'deferred_reset' if exists.
- */
-static void
-hisi_acc_vf_state_mutex_unlock(struct hisi_acc_vf_core_device *hisi_acc_vdev)
+static void hisi_acc_vf_reset(struct hisi_acc_vf_core_device *hisi_acc_vdev)
{
-again:
- spin_lock(&hisi_acc_vdev->reset_lock);
- if (hisi_acc_vdev->deferred_reset) {
- hisi_acc_vdev->deferred_reset = false;
- spin_unlock(&hisi_acc_vdev->reset_lock);
- hisi_acc_vdev->vf_qm_state = QM_NOT_READY;
- hisi_acc_vdev->mig_state = VFIO_DEVICE_STATE_RUNNING;
- hisi_acc_vf_disable_fds(hisi_acc_vdev);
- goto again;
- }
- mutex_unlock(&hisi_acc_vdev->state_mutex);
- spin_unlock(&hisi_acc_vdev->reset_lock);
+ hisi_acc_vdev->vf_qm_state = QM_NOT_READY;
+ hisi_acc_vdev->mig_state = VFIO_DEVICE_STATE_RUNNING;
+ hisi_acc_vf_disable_fds(hisi_acc_vdev);
}
static void hisi_acc_vf_start_device(struct hisi_acc_vf_core_device *hisi_acc_vdev)
@@ -804,8 +790,10 @@ static long hisi_acc_vf_precopy_ioctl(struct file *filp,
info.dirty_bytes = 0;
info.initial_bytes = migf->total_length - *pos;
+ mutex_unlock(&migf->lock);
+ mutex_unlock(&hisi_acc_vdev->state_mutex);
- ret = copy_to_user((void __user *)arg, &info, minsz) ? -EFAULT : 0;
+ return copy_to_user((void __user *)arg, &info, minsz) ? -EFAULT : 0;
out:
mutex_unlock(&migf->lock);
mutex_unlock(&hisi_acc_vdev->state_mutex);
@@ -1071,7 +1059,7 @@ hisi_acc_vfio_pci_set_device_state(struct vfio_device *vdev,
break;
}
}
- hisi_acc_vf_state_mutex_unlock(hisi_acc_vdev);
+ mutex_unlock(&hisi_acc_vdev->state_mutex);
return res;
}
@@ -1092,7 +1080,7 @@ hisi_acc_vfio_pci_get_device_state(struct vfio_device *vdev,
mutex_lock(&hisi_acc_vdev->state_mutex);
*curr_state = hisi_acc_vdev->mig_state;
- hisi_acc_vf_state_mutex_unlock(hisi_acc_vdev);
+ mutex_unlock(&hisi_acc_vdev->state_mutex);
return 0;
}
@@ -1104,21 +1092,9 @@ static void hisi_acc_vf_pci_aer_reset_done(struct pci_dev *pdev)
VFIO_MIGRATION_STOP_COPY)
return;
- /*
- * As the higher VFIO layers are holding locks across reset and using
- * those same locks with the mm_lock we need to prevent ABBA deadlock
- * with the state_mutex and mm_lock.
- * In case the state_mutex was taken already we defer the cleanup work
- * to the unlock flow of the other running context.
- */
- spin_lock(&hisi_acc_vdev->reset_lock);
- hisi_acc_vdev->deferred_reset = true;
- if (!mutex_trylock(&hisi_acc_vdev->state_mutex)) {
- spin_unlock(&hisi_acc_vdev->reset_lock);
- return;
- }
- spin_unlock(&hisi_acc_vdev->reset_lock);
- hisi_acc_vf_state_mutex_unlock(hisi_acc_vdev);
+ mutex_lock(&hisi_acc_vdev->state_mutex);
+ hisi_acc_vf_reset(hisi_acc_vdev);
+ mutex_unlock(&hisi_acc_vdev->state_mutex);
}
static int hisi_acc_vf_qm_init(struct hisi_acc_vf_core_device *hisi_acc_vdev)
diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
index dcabfeec6ca1..5bab46602fad 100644
--- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
+++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
@@ -98,8 +98,8 @@ struct hisi_acc_vf_migration_file {
struct hisi_acc_vf_core_device {
struct vfio_pci_core_device core_device;
- u8 match_done:1;
- u8 deferred_reset:1;
+ u8 match_done;
+
/* For migration state */
struct mutex state_mutex;
enum vfio_device_mig_state mig_state;
@@ -109,8 +109,6 @@ struct hisi_acc_vf_core_device {
struct hisi_qm vf_qm;
u32 vf_qm_state;
int vf_id;
- /* For reset handler */
- spinlock_t reset_lock;
struct hisi_acc_vf_migration_file *resuming_migf;
struct hisi_acc_vf_migration_file *saving_migf;
};
--
2.34.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] hisi_acc_vfio_pci: Remove the deferred_reset logic
2024-02-29 9:11 [PATCH] hisi_acc_vfio_pci: Remove the deferred_reset logic Shameer Kolothum
@ 2024-02-29 22:05 ` Brett Creeley
2024-03-04 20:27 ` Alex Williamson
2024-03-01 17:21 ` Brett Creeley
` (3 subsequent siblings)
4 siblings, 1 reply; 8+ messages in thread
From: Brett Creeley @ 2024-02-29 22:05 UTC (permalink / raw
To: Shameer Kolothum, kvm
Cc: alex.williamson, jgg, yishaih, kevin.tian, linuxarm, liulongfang
On 2/29/2024 1:11 AM, Shameer Kolothum wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> The deferred_reset logic was added to vfio migration drivers to prevent
> a circular locking dependency with respect to mm_lock and state mutex.
> This is mainly because of the copy_to/from_user() functions(which takes
> mm_lock) invoked under state mutex. But for HiSilicon driver, the only
> place where we now hold the state mutex for copy_to_user is during the
> PRE_COPY IOCTL. So for pre_copy, release the lock as soon as we have
> updated the data and perform copy_to_user without state mutex. By this,
> we can get rid of the deferred_reset logic.
>
> Link: https://lore.kernel.org/kvm/20240220132459.GM13330@nvidia.com/
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Shameer,
Thanks for providing this example. After seeing this, it probably
doens't make sense to accept my 2/2 patch at
https://lore.kernel.org/kvm/20240228003205.47311-3-brett.creeley@amd.com/.
I have reworked that patch and am currently doing some testing with it
to make sure it's functional. Once I have some results I will send a v3.
Thanks,
Brett
> ---
> .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 48 +++++--------------
> .../vfio/pci/hisilicon/hisi_acc_vfio_pci.h | 6 +--
> 2 files changed, 14 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> index 4d27465c8f1a..9a3e97108ace 100644
> --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> @@ -630,25 +630,11 @@ static void hisi_acc_vf_disable_fds(struct hisi_acc_vf_core_device *hisi_acc_vde
> }
> }
>
> -/*
> - * This function is called in all state_mutex unlock cases to
> - * handle a 'deferred_reset' if exists.
> - */
> -static void
> -hisi_acc_vf_state_mutex_unlock(struct hisi_acc_vf_core_device *hisi_acc_vdev)
> +static void hisi_acc_vf_reset(struct hisi_acc_vf_core_device *hisi_acc_vdev)
> {
> -again:
> - spin_lock(&hisi_acc_vdev->reset_lock);
> - if (hisi_acc_vdev->deferred_reset) {
> - hisi_acc_vdev->deferred_reset = false;
> - spin_unlock(&hisi_acc_vdev->reset_lock);
> - hisi_acc_vdev->vf_qm_state = QM_NOT_READY;
> - hisi_acc_vdev->mig_state = VFIO_DEVICE_STATE_RUNNING;
> - hisi_acc_vf_disable_fds(hisi_acc_vdev);
> - goto again;
> - }
> - mutex_unlock(&hisi_acc_vdev->state_mutex);
> - spin_unlock(&hisi_acc_vdev->reset_lock);
> + hisi_acc_vdev->vf_qm_state = QM_NOT_READY;
> + hisi_acc_vdev->mig_state = VFIO_DEVICE_STATE_RUNNING;
> + hisi_acc_vf_disable_fds(hisi_acc_vdev);
> }
>
> static void hisi_acc_vf_start_device(struct hisi_acc_vf_core_device *hisi_acc_vdev)
> @@ -804,8 +790,10 @@ static long hisi_acc_vf_precopy_ioctl(struct file *filp,
>
> info.dirty_bytes = 0;
> info.initial_bytes = migf->total_length - *pos;
> + mutex_unlock(&migf->lock);
> + mutex_unlock(&hisi_acc_vdev->state_mutex);
>
> - ret = copy_to_user((void __user *)arg, &info, minsz) ? -EFAULT : 0;
> + return copy_to_user((void __user *)arg, &info, minsz) ? -EFAULT : 0;
> out:
> mutex_unlock(&migf->lock);
> mutex_unlock(&hisi_acc_vdev->state_mutex);
> @@ -1071,7 +1059,7 @@ hisi_acc_vfio_pci_set_device_state(struct vfio_device *vdev,
> break;
> }
> }
> - hisi_acc_vf_state_mutex_unlock(hisi_acc_vdev);
> + mutex_unlock(&hisi_acc_vdev->state_mutex);
> return res;
> }
>
> @@ -1092,7 +1080,7 @@ hisi_acc_vfio_pci_get_device_state(struct vfio_device *vdev,
>
> mutex_lock(&hisi_acc_vdev->state_mutex);
> *curr_state = hisi_acc_vdev->mig_state;
> - hisi_acc_vf_state_mutex_unlock(hisi_acc_vdev);
> + mutex_unlock(&hisi_acc_vdev->state_mutex);
> return 0;
> }
>
> @@ -1104,21 +1092,9 @@ static void hisi_acc_vf_pci_aer_reset_done(struct pci_dev *pdev)
> VFIO_MIGRATION_STOP_COPY)
> return;
>
> - /*
> - * As the higher VFIO layers are holding locks across reset and using
> - * those same locks with the mm_lock we need to prevent ABBA deadlock
> - * with the state_mutex and mm_lock.
> - * In case the state_mutex was taken already we defer the cleanup work
> - * to the unlock flow of the other running context.
> - */
> - spin_lock(&hisi_acc_vdev->reset_lock);
> - hisi_acc_vdev->deferred_reset = true;
> - if (!mutex_trylock(&hisi_acc_vdev->state_mutex)) {
> - spin_unlock(&hisi_acc_vdev->reset_lock);
> - return;
> - }
> - spin_unlock(&hisi_acc_vdev->reset_lock);
> - hisi_acc_vf_state_mutex_unlock(hisi_acc_vdev);
> + mutex_lock(&hisi_acc_vdev->state_mutex);
> + hisi_acc_vf_reset(hisi_acc_vdev);
> + mutex_unlock(&hisi_acc_vdev->state_mutex);
> }
>
> static int hisi_acc_vf_qm_init(struct hisi_acc_vf_core_device *hisi_acc_vdev)
> diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
> index dcabfeec6ca1..5bab46602fad 100644
> --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
> +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
> @@ -98,8 +98,8 @@ struct hisi_acc_vf_migration_file {
>
> struct hisi_acc_vf_core_device {
> struct vfio_pci_core_device core_device;
> - u8 match_done:1;
> - u8 deferred_reset:1;
> + u8 match_done;
> +
> /* For migration state */
> struct mutex state_mutex;
> enum vfio_device_mig_state mig_state;
> @@ -109,8 +109,6 @@ struct hisi_acc_vf_core_device {
> struct hisi_qm vf_qm;
> u32 vf_qm_state;
> int vf_id;
> - /* For reset handler */
> - spinlock_t reset_lock;
> struct hisi_acc_vf_migration_file *resuming_migf;
> struct hisi_acc_vf_migration_file *saving_migf;
> };
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hisi_acc_vfio_pci: Remove the deferred_reset logic
2024-02-29 9:11 [PATCH] hisi_acc_vfio_pci: Remove the deferred_reset logic Shameer Kolothum
2024-02-29 22:05 ` Brett Creeley
@ 2024-03-01 17:21 ` Brett Creeley
2024-03-04 8:08 ` Tian, Kevin
` (2 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Brett Creeley @ 2024-03-01 17:21 UTC (permalink / raw
To: Shameer Kolothum, kvm
Cc: alex.williamson, jgg, yishaih, kevin.tian, linuxarm, liulongfang
On 2/29/2024 1:11 AM, Shameer Kolothum wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> The deferred_reset logic was added to vfio migration drivers to prevent
> a circular locking dependency with respect to mm_lock and state mutex.
> This is mainly because of the copy_to/from_user() functions(which takes
> mm_lock) invoked under state mutex. But for HiSilicon driver, the only
> place where we now hold the state mutex for copy_to_user is during the
> PRE_COPY IOCTL. So for pre_copy, release the lock as soon as we have
> updated the data and perform copy_to_user without state mutex. By this,
> we can get rid of the deferred_reset logic.
>
> Link: https://lore.kernel.org/kvm/20240220132459.GM13330@nvidia.com/
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
> .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 48 +++++--------------
> .../vfio/pci/hisilicon/hisi_acc_vfio_pci.h | 6 +--
> 2 files changed, 14 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> index 4d27465c8f1a..9a3e97108ace 100644
> --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> @@ -630,25 +630,11 @@ static void hisi_acc_vf_disable_fds(struct hisi_acc_vf_core_device *hisi_acc_vde
> }
> }
>
> -/*
> - * This function is called in all state_mutex unlock cases to
> - * handle a 'deferred_reset' if exists.
> - */
> -static void
> -hisi_acc_vf_state_mutex_unlock(struct hisi_acc_vf_core_device *hisi_acc_vdev)
> +static void hisi_acc_vf_reset(struct hisi_acc_vf_core_device *hisi_acc_vdev)
> {
> -again:
> - spin_lock(&hisi_acc_vdev->reset_lock);
> - if (hisi_acc_vdev->deferred_reset) {
> - hisi_acc_vdev->deferred_reset = false;
> - spin_unlock(&hisi_acc_vdev->reset_lock);
> - hisi_acc_vdev->vf_qm_state = QM_NOT_READY;
> - hisi_acc_vdev->mig_state = VFIO_DEVICE_STATE_RUNNING;
> - hisi_acc_vf_disable_fds(hisi_acc_vdev);
> - goto again;
> - }
> - mutex_unlock(&hisi_acc_vdev->state_mutex);
> - spin_unlock(&hisi_acc_vdev->reset_lock);
> + hisi_acc_vdev->vf_qm_state = QM_NOT_READY;
> + hisi_acc_vdev->mig_state = VFIO_DEVICE_STATE_RUNNING;
> + hisi_acc_vf_disable_fds(hisi_acc_vdev);
> }
>
> static void hisi_acc_vf_start_device(struct hisi_acc_vf_core_device *hisi_acc_vdev)
> @@ -804,8 +790,10 @@ static long hisi_acc_vf_precopy_ioctl(struct file *filp,
>
> info.dirty_bytes = 0;
> info.initial_bytes = migf->total_length - *pos;
> + mutex_unlock(&migf->lock);
> + mutex_unlock(&hisi_acc_vdev->state_mutex);
>
> - ret = copy_to_user((void __user *)arg, &info, minsz) ? -EFAULT : 0;
> + return copy_to_user((void __user *)arg, &info, minsz) ? -EFAULT : 0;
> out:
> mutex_unlock(&migf->lock);
> mutex_unlock(&hisi_acc_vdev->state_mutex);
> @@ -1071,7 +1059,7 @@ hisi_acc_vfio_pci_set_device_state(struct vfio_device *vdev,
> break;
> }
> }
> - hisi_acc_vf_state_mutex_unlock(hisi_acc_vdev);
> + mutex_unlock(&hisi_acc_vdev->state_mutex);
> return res;
> }
>
> @@ -1092,7 +1080,7 @@ hisi_acc_vfio_pci_get_device_state(struct vfio_device *vdev,
>
> mutex_lock(&hisi_acc_vdev->state_mutex);
> *curr_state = hisi_acc_vdev->mig_state;
> - hisi_acc_vf_state_mutex_unlock(hisi_acc_vdev);
> + mutex_unlock(&hisi_acc_vdev->state_mutex);
> return 0;
> }
>
> @@ -1104,21 +1092,9 @@ static void hisi_acc_vf_pci_aer_reset_done(struct pci_dev *pdev)
> VFIO_MIGRATION_STOP_COPY)
> return;
>
> - /*
> - * As the higher VFIO layers are holding locks across reset and using
> - * those same locks with the mm_lock we need to prevent ABBA deadlock
> - * with the state_mutex and mm_lock.
> - * In case the state_mutex was taken already we defer the cleanup work
> - * to the unlock flow of the other running context.
> - */
> - spin_lock(&hisi_acc_vdev->reset_lock);
> - hisi_acc_vdev->deferred_reset = true;
> - if (!mutex_trylock(&hisi_acc_vdev->state_mutex)) {
> - spin_unlock(&hisi_acc_vdev->reset_lock);
> - return;
> - }
> - spin_unlock(&hisi_acc_vdev->reset_lock);
> - hisi_acc_vf_state_mutex_unlock(hisi_acc_vdev);
> + mutex_lock(&hisi_acc_vdev->state_mutex);
> + hisi_acc_vf_reset(hisi_acc_vdev);
> + mutex_unlock(&hisi_acc_vdev->state_mutex);
> }
>
> static int hisi_acc_vf_qm_init(struct hisi_acc_vf_core_device *hisi_acc_vdev)
> diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
> index dcabfeec6ca1..5bab46602fad 100644
> --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
> +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
> @@ -98,8 +98,8 @@ struct hisi_acc_vf_migration_file {
>
> struct hisi_acc_vf_core_device {
> struct vfio_pci_core_device core_device;
> - u8 match_done:1;
> - u8 deferred_reset:1;
> + u8 match_done;
> +
> /* For migration state */
> struct mutex state_mutex;
> enum vfio_device_mig_state mig_state;
> @@ -109,8 +109,6 @@ struct hisi_acc_vf_core_device {
> struct hisi_qm vf_qm;
> u32 vf_qm_state;
> int vf_id;
> - /* For reset handler */
> - spinlock_t reset_lock;
> struct hisi_acc_vf_migration_file *resuming_migf;
> struct hisi_acc_vf_migration_file *saving_migf;
> };
> --
> 2.34.1
>
LGTM. Thanks again for the example.
FWIW: Reviewed-by: Brett Creeley <brett.creeley@amd.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] hisi_acc_vfio_pci: Remove the deferred_reset logic
2024-02-29 9:11 [PATCH] hisi_acc_vfio_pci: Remove the deferred_reset logic Shameer Kolothum
2024-02-29 22:05 ` Brett Creeley
2024-03-01 17:21 ` Brett Creeley
@ 2024-03-04 8:08 ` Tian, Kevin
2024-03-05 18:02 ` Jason Gunthorpe
2024-03-05 23:12 ` Alex Williamson
4 siblings, 0 replies; 8+ messages in thread
From: Tian, Kevin @ 2024-03-04 8:08 UTC (permalink / raw
To: Shameer Kolothum, kvm@vger.kernel.org
Cc: alex.williamson@redhat.com, jgg@ziepe.ca, yishaih@nvidia.com,
linuxarm@huawei.com, liulongfang@huawei.com, bcreeley@amd.com
> From: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> Sent: Thursday, February 29, 2024 5:12 PM
>
> The deferred_reset logic was added to vfio migration drivers to prevent
> a circular locking dependency with respect to mm_lock and state mutex.
> This is mainly because of the copy_to/from_user() functions(which takes
> mm_lock) invoked under state mutex. But for HiSilicon driver, the only
> place where we now hold the state mutex for copy_to_user is during the
> PRE_COPY IOCTL. So for pre_copy, release the lock as soon as we have
> updated the data and perform copy_to_user without state mutex. By this,
> we can get rid of the deferred_reset logic.
>
> Link: https://lore.kernel.org/kvm/20240220132459.GM13330@nvidia.com/
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hisi_acc_vfio_pci: Remove the deferred_reset logic
2024-02-29 22:05 ` Brett Creeley
@ 2024-03-04 20:27 ` Alex Williamson
2024-03-05 0:17 ` Brett Creeley
0 siblings, 1 reply; 8+ messages in thread
From: Alex Williamson @ 2024-03-04 20:27 UTC (permalink / raw
To: Brett Creeley
Cc: Shameer Kolothum, kvm, jgg, yishaih, kevin.tian, linuxarm,
liulongfang
On Thu, 29 Feb 2024 14:05:58 -0800
Brett Creeley <bcreeley@amd.com> wrote:
> On 2/29/2024 1:11 AM, Shameer Kolothum wrote:
> > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> >
> >
> > The deferred_reset logic was added to vfio migration drivers to prevent
> > a circular locking dependency with respect to mm_lock and state mutex.
> > This is mainly because of the copy_to/from_user() functions(which takes
> > mm_lock) invoked under state mutex. But for HiSilicon driver, the only
> > place where we now hold the state mutex for copy_to_user is during the
> > PRE_COPY IOCTL. So for pre_copy, release the lock as soon as we have
> > updated the data and perform copy_to_user without state mutex. By this,
> > we can get rid of the deferred_reset logic.
> >
> > Link: https://lore.kernel.org/kvm/20240220132459.GM13330@nvidia.com/
> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
>
> Shameer,
>
> Thanks for providing this example. After seeing this, it probably
> doens't make sense to accept my 2/2 patch at
> https://lore.kernel.org/kvm/20240228003205.47311-3-brett.creeley@amd.com/.
>
> I have reworked that patch and am currently doing some testing with it
> to make sure it's functional. Once I have some results I will send a v3.
Darn, somehow this thread snuck by me last week. Currently your series
is at the top of my next branch, so I'll just rebase it to 8512ed256334
("vfio/pds: Always clear the save/restore FDs on reset") to drop your
2/2 and wait for something new relative to the reset logic. Thanks,
Alex
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hisi_acc_vfio_pci: Remove the deferred_reset logic
2024-03-04 20:27 ` Alex Williamson
@ 2024-03-05 0:17 ` Brett Creeley
0 siblings, 0 replies; 8+ messages in thread
From: Brett Creeley @ 2024-03-05 0:17 UTC (permalink / raw
To: Alex Williamson
Cc: Shameer Kolothum, kvm, jgg, yishaih, kevin.tian, linuxarm,
liulongfang
On 3/4/2024 12:27 PM, Alex Williamson wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Thu, 29 Feb 2024 14:05:58 -0800
> Brett Creeley <bcreeley@amd.com> wrote:
>
>> On 2/29/2024 1:11 AM, Shameer Kolothum wrote:
>>> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>>>
>>>
>>> The deferred_reset logic was added to vfio migration drivers to prevent
>>> a circular locking dependency with respect to mm_lock and state mutex.
>>> This is mainly because of the copy_to/from_user() functions(which takes
>>> mm_lock) invoked under state mutex. But for HiSilicon driver, the only
>>> place where we now hold the state mutex for copy_to_user is during the
>>> PRE_COPY IOCTL. So for pre_copy, release the lock as soon as we have
>>> updated the data and perform copy_to_user without state mutex. By this,
>>> we can get rid of the deferred_reset logic.
>>>
>>> Link: https://lore.kernel.org/kvm/20240220132459.GM13330@nvidia.com/
>>> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
>>
>> Shameer,
>>
>> Thanks for providing this example. After seeing this, it probably
>> doens't make sense to accept my 2/2 patch at
>> https://lore.kernel.org/kvm/20240228003205.47311-3-brett.creeley@amd.com/.
>>
>> I have reworked that patch and am currently doing some testing with it
>> to make sure it's functional. Once I have some results I will send a v3.
>
> Darn, somehow this thread snuck by me last week. Currently your series
> is at the top of my next branch, so I'll just rebase it to 8512ed256334
> ("vfio/pds: Always clear the save/restore FDs on reset") to drop your
> 2/2 and wait for something new relative to the reset logic. Thanks,
>
> Alex
That works for me.
Thanks,
Brett
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hisi_acc_vfio_pci: Remove the deferred_reset logic
2024-02-29 9:11 [PATCH] hisi_acc_vfio_pci: Remove the deferred_reset logic Shameer Kolothum
` (2 preceding siblings ...)
2024-03-04 8:08 ` Tian, Kevin
@ 2024-03-05 18:02 ` Jason Gunthorpe
2024-03-05 23:12 ` Alex Williamson
4 siblings, 0 replies; 8+ messages in thread
From: Jason Gunthorpe @ 2024-03-05 18:02 UTC (permalink / raw
To: Shameer Kolothum
Cc: kvm, alex.williamson, yishaih, kevin.tian, linuxarm, liulongfang,
bcreeley
On Thu, Feb 29, 2024 at 09:11:52AM +0000, Shameer Kolothum wrote:
> The deferred_reset logic was added to vfio migration drivers to prevent
> a circular locking dependency with respect to mm_lock and state mutex.
> This is mainly because of the copy_to/from_user() functions(which takes
> mm_lock) invoked under state mutex. But for HiSilicon driver, the only
> place where we now hold the state mutex for copy_to_user is during the
> PRE_COPY IOCTL. So for pre_copy, release the lock as soon as we have
> updated the data and perform copy_to_user without state mutex. By this,
> we can get rid of the deferred_reset logic.
>
> Link: https://lore.kernel.org/kvm/20240220132459.GM13330@nvidia.com/
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
> .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 48 +++++--------------
> .../vfio/pci/hisilicon/hisi_acc_vfio_pci.h | 6 +--
> 2 files changed, 14 insertions(+), 40 deletions(-)
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Jason
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hisi_acc_vfio_pci: Remove the deferred_reset logic
2024-02-29 9:11 [PATCH] hisi_acc_vfio_pci: Remove the deferred_reset logic Shameer Kolothum
` (3 preceding siblings ...)
2024-03-05 18:02 ` Jason Gunthorpe
@ 2024-03-05 23:12 ` Alex Williamson
4 siblings, 0 replies; 8+ messages in thread
From: Alex Williamson @ 2024-03-05 23:12 UTC (permalink / raw
To: Shameer Kolothum
Cc: kvm, jgg, yishaih, kevin.tian, linuxarm, liulongfang, bcreeley
On Thu, 29 Feb 2024 09:11:52 +0000
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> The deferred_reset logic was added to vfio migration drivers to prevent
> a circular locking dependency with respect to mm_lock and state mutex.
> This is mainly because of the copy_to/from_user() functions(which takes
> mm_lock) invoked under state mutex. But for HiSilicon driver, the only
> place where we now hold the state mutex for copy_to_user is during the
> PRE_COPY IOCTL. So for pre_copy, release the lock as soon as we have
> updated the data and perform copy_to_user without state mutex. By this,
> we can get rid of the deferred_reset logic.
>
> Link: https://lore.kernel.org/kvm/20240220132459.GM13330@nvidia.com/
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
> .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 48 +++++--------------
> .../vfio/pci/hisilicon/hisi_acc_vfio_pci.h | 6 +--
> 2 files changed, 14 insertions(+), 40 deletions(-)
Applied to vfio next branch for v6.9. Thanks,
Alex
>
> diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> index 4d27465c8f1a..9a3e97108ace 100644
> --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> @@ -630,25 +630,11 @@ static void hisi_acc_vf_disable_fds(struct hisi_acc_vf_core_device *hisi_acc_vde
> }
> }
>
> -/*
> - * This function is called in all state_mutex unlock cases to
> - * handle a 'deferred_reset' if exists.
> - */
> -static void
> -hisi_acc_vf_state_mutex_unlock(struct hisi_acc_vf_core_device *hisi_acc_vdev)
> +static void hisi_acc_vf_reset(struct hisi_acc_vf_core_device *hisi_acc_vdev)
> {
> -again:
> - spin_lock(&hisi_acc_vdev->reset_lock);
> - if (hisi_acc_vdev->deferred_reset) {
> - hisi_acc_vdev->deferred_reset = false;
> - spin_unlock(&hisi_acc_vdev->reset_lock);
> - hisi_acc_vdev->vf_qm_state = QM_NOT_READY;
> - hisi_acc_vdev->mig_state = VFIO_DEVICE_STATE_RUNNING;
> - hisi_acc_vf_disable_fds(hisi_acc_vdev);
> - goto again;
> - }
> - mutex_unlock(&hisi_acc_vdev->state_mutex);
> - spin_unlock(&hisi_acc_vdev->reset_lock);
> + hisi_acc_vdev->vf_qm_state = QM_NOT_READY;
> + hisi_acc_vdev->mig_state = VFIO_DEVICE_STATE_RUNNING;
> + hisi_acc_vf_disable_fds(hisi_acc_vdev);
> }
>
> static void hisi_acc_vf_start_device(struct hisi_acc_vf_core_device *hisi_acc_vdev)
> @@ -804,8 +790,10 @@ static long hisi_acc_vf_precopy_ioctl(struct file *filp,
>
> info.dirty_bytes = 0;
> info.initial_bytes = migf->total_length - *pos;
> + mutex_unlock(&migf->lock);
> + mutex_unlock(&hisi_acc_vdev->state_mutex);
>
> - ret = copy_to_user((void __user *)arg, &info, minsz) ? -EFAULT : 0;
> + return copy_to_user((void __user *)arg, &info, minsz) ? -EFAULT : 0;
> out:
> mutex_unlock(&migf->lock);
> mutex_unlock(&hisi_acc_vdev->state_mutex);
> @@ -1071,7 +1059,7 @@ hisi_acc_vfio_pci_set_device_state(struct vfio_device *vdev,
> break;
> }
> }
> - hisi_acc_vf_state_mutex_unlock(hisi_acc_vdev);
> + mutex_unlock(&hisi_acc_vdev->state_mutex);
> return res;
> }
>
> @@ -1092,7 +1080,7 @@ hisi_acc_vfio_pci_get_device_state(struct vfio_device *vdev,
>
> mutex_lock(&hisi_acc_vdev->state_mutex);
> *curr_state = hisi_acc_vdev->mig_state;
> - hisi_acc_vf_state_mutex_unlock(hisi_acc_vdev);
> + mutex_unlock(&hisi_acc_vdev->state_mutex);
> return 0;
> }
>
> @@ -1104,21 +1092,9 @@ static void hisi_acc_vf_pci_aer_reset_done(struct pci_dev *pdev)
> VFIO_MIGRATION_STOP_COPY)
> return;
>
> - /*
> - * As the higher VFIO layers are holding locks across reset and using
> - * those same locks with the mm_lock we need to prevent ABBA deadlock
> - * with the state_mutex and mm_lock.
> - * In case the state_mutex was taken already we defer the cleanup work
> - * to the unlock flow of the other running context.
> - */
> - spin_lock(&hisi_acc_vdev->reset_lock);
> - hisi_acc_vdev->deferred_reset = true;
> - if (!mutex_trylock(&hisi_acc_vdev->state_mutex)) {
> - spin_unlock(&hisi_acc_vdev->reset_lock);
> - return;
> - }
> - spin_unlock(&hisi_acc_vdev->reset_lock);
> - hisi_acc_vf_state_mutex_unlock(hisi_acc_vdev);
> + mutex_lock(&hisi_acc_vdev->state_mutex);
> + hisi_acc_vf_reset(hisi_acc_vdev);
> + mutex_unlock(&hisi_acc_vdev->state_mutex);
> }
>
> static int hisi_acc_vf_qm_init(struct hisi_acc_vf_core_device *hisi_acc_vdev)
> diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
> index dcabfeec6ca1..5bab46602fad 100644
> --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
> +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
> @@ -98,8 +98,8 @@ struct hisi_acc_vf_migration_file {
>
> struct hisi_acc_vf_core_device {
> struct vfio_pci_core_device core_device;
> - u8 match_done:1;
> - u8 deferred_reset:1;
> + u8 match_done;
> +
> /* For migration state */
> struct mutex state_mutex;
> enum vfio_device_mig_state mig_state;
> @@ -109,8 +109,6 @@ struct hisi_acc_vf_core_device {
> struct hisi_qm vf_qm;
> u32 vf_qm_state;
> int vf_id;
> - /* For reset handler */
> - spinlock_t reset_lock;
> struct hisi_acc_vf_migration_file *resuming_migf;
> struct hisi_acc_vf_migration_file *saving_migf;
> };
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-03-05 23:13 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-29 9:11 [PATCH] hisi_acc_vfio_pci: Remove the deferred_reset logic Shameer Kolothum
2024-02-29 22:05 ` Brett Creeley
2024-03-04 20:27 ` Alex Williamson
2024-03-05 0:17 ` Brett Creeley
2024-03-01 17:21 ` Brett Creeley
2024-03-04 8:08 ` Tian, Kevin
2024-03-05 18:02 ` Jason Gunthorpe
2024-03-05 23:12 ` Alex Williamson
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 read-only IMAP folder(s) and NNTP newsgroup(s).