| Message ID | 20231122193634.27250-2-brett.creeley@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hisi_acc_vfio_pci: locking updates | expand |
> -----Original Message----- > From: Brett Creeley [mailto:brett.creeley@amd.com] > Sent: 22 November 2023 19:37 > To: jgg@ziepe.ca; yishaih@nvidia.com; liulongfang > <liulongfang@huawei.com>; Shameerali Kolothum Thodi > <shameerali.kolothum.thodi@huawei.com>; kevin.tian@intel.com; > alex.williamson@redhat.com; kvm@vger.kernel.org; > linux-kernel@vger.kernel.org > Cc: shannon.nelson@amd.com; brett.creeley@amd.com > Subject: [PATCH vfio 1/2] hisi_acc_vfio_pci: Change reset_lock to mutex_lock > > Based on comments from other vfio vendors and the > maintainer the vfio/pds driver changed the reset_lock > to a mutex_lock. As part of that change it was requested > that the other vendor drivers be changed as well. So, > make the change. > > The comment that requested the change for reference: > https://lore.kernel.org/kvm/BN9PR11MB52769E037CB356AB15A0D9B88CA > 0A@BN9PR11MB5276.namprd11.prod.outlook.com/ > > Also, make checkpatch happy by moving the lock comment. > > Signed-off-by: Brett Creeley <brett.creeley@amd.com> > --- > drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 13 +++++++------ > drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h | 3 +-- > 2 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > index b2f9778c8366..2c049b8de4b4 100644 > --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > @@ -638,17 +638,17 @@ static void > hisi_acc_vf_state_mutex_unlock(struct hisi_acc_vf_core_device > *hisi_acc_vdev) > { > again: > - spin_lock(&hisi_acc_vdev->reset_lock); > + mutex_lock(&hisi_acc_vdev->reset_mutex); > if (hisi_acc_vdev->deferred_reset) { > hisi_acc_vdev->deferred_reset = false; > - spin_unlock(&hisi_acc_vdev->reset_lock); > + mutex_unlock(&hisi_acc_vdev->reset_mutex); Don't think we have that sleeping while atomic case for this here. Same for mlx5 as well. But if the idea is to have a common locking across vendor drivers, it is fine. Reviewed-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> Thanks, Shameer > 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); > + mutex_unlock(&hisi_acc_vdev->reset_mutex); > } > > static void hisi_acc_vf_start_device(struct hisi_acc_vf_core_device > *hisi_acc_vdev) > @@ -1108,13 +1108,13 @@ static void > hisi_acc_vf_pci_aer_reset_done(struct pci_dev *pdev) > * 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); > + mutex_lock(&hisi_acc_vdev->reset_mutex); > hisi_acc_vdev->deferred_reset = true; > if (!mutex_trylock(&hisi_acc_vdev->state_mutex)) { > - spin_unlock(&hisi_acc_vdev->reset_lock); > + mutex_unlock(&hisi_acc_vdev->reset_mutex); > return; > } > - spin_unlock(&hisi_acc_vdev->reset_lock); > + mutex_unlock(&hisi_acc_vdev->reset_mutex); > hisi_acc_vf_state_mutex_unlock(hisi_acc_vdev); > } > > @@ -1350,6 +1350,7 @@ static int hisi_acc_vfio_pci_migrn_init_dev(struct > vfio_device *core_vdev) > hisi_acc_vdev->pf_qm = pf_qm; > hisi_acc_vdev->vf_dev = pdev; > mutex_init(&hisi_acc_vdev->state_mutex); > + mutex_init(&hisi_acc_vdev->reset_mutex); > > core_vdev->migration_flags = VFIO_MIGRATION_STOP_COPY | > VFIO_MIGRATION_PRE_COPY; > core_vdev->mig_ops = &hisi_acc_vfio_pci_migrn_state_ops; > diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h > b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h > index dcabfeec6ca1..ed5ab332d0f3 100644 > --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h > +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h > @@ -109,8 +109,7 @@ 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 mutex reset_mutex; /* For reset handler */ > struct hisi_acc_vf_migration_file *resuming_migf; > struct hisi_acc_vf_migration_file *saving_migf; > }; > -- > 2.17.1
On Fri, Nov 24, 2023 at 08:46:58AM +0000, Shameerali Kolothum Thodi wrote: > > diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > > b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > > index b2f9778c8366..2c049b8de4b4 100644 > > --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > > +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > > @@ -638,17 +638,17 @@ static void > > hisi_acc_vf_state_mutex_unlock(struct hisi_acc_vf_core_device > > *hisi_acc_vdev) > > { > > again: > > - spin_lock(&hisi_acc_vdev->reset_lock); > > + mutex_lock(&hisi_acc_vdev->reset_mutex); > > if (hisi_acc_vdev->deferred_reset) { > > hisi_acc_vdev->deferred_reset = false; > > - spin_unlock(&hisi_acc_vdev->reset_lock); > > + mutex_unlock(&hisi_acc_vdev->reset_mutex); > > Don't think we have that sleeping while atomic case for this here. > Same for mlx5 as well. But if the idea is to have a common locking > across vendor drivers, it is fine. Yeah, I'm not sure about changing spinlocks to mutex's for no reason.. If we don't sleep and don't hold it for very long then the spinlock is appropriate Jason
> From: Jason Gunthorpe <jgg@ziepe.ca> > Sent: Tuesday, November 28, 2023 8:46 AM > > On Fri, Nov 24, 2023 at 08:46:58AM +0000, Shameerali Kolothum Thodi wrote: > > > diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > > > b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > > > index b2f9778c8366..2c049b8de4b4 100644 > > > --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > > > +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > > > @@ -638,17 +638,17 @@ static void > > > hisi_acc_vf_state_mutex_unlock(struct hisi_acc_vf_core_device > > > *hisi_acc_vdev) > > > { > > > again: > > > - spin_lock(&hisi_acc_vdev->reset_lock); > > > + mutex_lock(&hisi_acc_vdev->reset_mutex); > > > if (hisi_acc_vdev->deferred_reset) { > > > hisi_acc_vdev->deferred_reset = false; > > > - spin_unlock(&hisi_acc_vdev->reset_lock); > > > + mutex_unlock(&hisi_acc_vdev->reset_mutex); > > > > Don't think we have that sleeping while atomic case for this here. > > Same for mlx5 as well. But if the idea is to have a common locking > > across vendor drivers, it is fine. > > Yeah, I'm not sure about changing spinlocks to mutex's for no reason.. > If we don't sleep and don't hold it for very long then the spinlock is > appropriate > It's me suggesting Brett to fix other two drivers, expecting a common locking pattern would cause less confusion to future new variant drivers. If both of you don't think it necessary then let's drop it.
diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c index b2f9778c8366..2c049b8de4b4 100644 --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c @@ -638,17 +638,17 @@ static void hisi_acc_vf_state_mutex_unlock(struct hisi_acc_vf_core_device *hisi_acc_vdev) { again: - spin_lock(&hisi_acc_vdev->reset_lock); + mutex_lock(&hisi_acc_vdev->reset_mutex); if (hisi_acc_vdev->deferred_reset) { hisi_acc_vdev->deferred_reset = false; - spin_unlock(&hisi_acc_vdev->reset_lock); + mutex_unlock(&hisi_acc_vdev->reset_mutex); 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); + mutex_unlock(&hisi_acc_vdev->reset_mutex); } static void hisi_acc_vf_start_device(struct hisi_acc_vf_core_device *hisi_acc_vdev) @@ -1108,13 +1108,13 @@ static void hisi_acc_vf_pci_aer_reset_done(struct pci_dev *pdev) * 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); + mutex_lock(&hisi_acc_vdev->reset_mutex); hisi_acc_vdev->deferred_reset = true; if (!mutex_trylock(&hisi_acc_vdev->state_mutex)) { - spin_unlock(&hisi_acc_vdev->reset_lock); + mutex_unlock(&hisi_acc_vdev->reset_mutex); return; } - spin_unlock(&hisi_acc_vdev->reset_lock); + mutex_unlock(&hisi_acc_vdev->reset_mutex); hisi_acc_vf_state_mutex_unlock(hisi_acc_vdev); } @@ -1350,6 +1350,7 @@ static int hisi_acc_vfio_pci_migrn_init_dev(struct vfio_device *core_vdev) hisi_acc_vdev->pf_qm = pf_qm; hisi_acc_vdev->vf_dev = pdev; mutex_init(&hisi_acc_vdev->state_mutex); + mutex_init(&hisi_acc_vdev->reset_mutex); core_vdev->migration_flags = VFIO_MIGRATION_STOP_COPY | VFIO_MIGRATION_PRE_COPY; core_vdev->mig_ops = &hisi_acc_vfio_pci_migrn_state_ops; diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h index dcabfeec6ca1..ed5ab332d0f3 100644 --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h @@ -109,8 +109,7 @@ 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 mutex reset_mutex; /* For reset handler */ struct hisi_acc_vf_migration_file *resuming_migf; struct hisi_acc_vf_migration_file *saving_migf; };
Based on comments from other vfio vendors and the maintainer the vfio/pds driver changed the reset_lock to a mutex_lock. As part of that change it was requested that the other vendor drivers be changed as well. So, make the change. The comment that requested the change for reference: https://lore.kernel.org/kvm/BN9PR11MB52769E037CB356AB15A0D9B88CA0A@BN9PR11MB5276.namprd11.prod.outlook.com/ Also, make checkpatch happy by moving the lock comment. Signed-off-by: Brett Creeley <brett.creeley@amd.com> --- drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 13 +++++++------ drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h | 3 +-- 2 files changed, 8 insertions(+), 8 deletions(-)