On 2022/10/18 14:55, liulongfang Wrote:
On 2022/10/17 21:57, Shameerali Kolothum Thodi Wrote:
-----Original Message----- From: liulongfang Sent: 17 October 2022 10:21 To: John Garry john.garry@huawei.com; alex.williamson@redhat.com; jgg@nvidia.com; Shameerali Kolothum Thodi shameerali.kolothum.thodi@huawei.com Cc: cohuck@redhat.com; linux-kernel@vger.kernel.org; linuxarm@openeuler.org Subject: Re: [Linuxarm] [PATCH 1/2] hisi_acc_vfio_pci: Add debugfs to migration driver
On 2022/10/14 17:20, John Garry wrote:
On 14/10/2022 03:57, Longfang Liu wrote:
+static void hisi_acc_vf_debugfs_init(struct hisi_acc_vf_core_device
*hisi_acc_vdev)
+{ + struct pci_dev *vf_pdev = hisi_acc_vdev->vf_dev; + struct device *dev = &vf_pdev->dev; + int ret;
+ if (!atomic_read(&hisi_acc_root_ref)) + hisi_acc_debugfs_root = debugfs_create_dir("hisi_vfio_acc",
NULL);
+ atomic_inc(&hisi_acc_root_ref);
This looks totally racy, such that I wonder why even bother using an atomic
for hisi_acc_root_ref.
When enabling VF, it is possible for multiple VMs to enable VF at the same time. The atomic variable is used to ensure that only one "hisi_vfio_acc" is created. When other VFs are enabled, it will not be created again, but will be used directly.
Indeed, why is hisi_acc_debugfs_root not created in the driver module init?
Because the normal function of VF is to perform encryption and decryption services, the live migration function is an auxiliary function, which no need to be used in scenarios where only encryption and decryption services are performed.
During module init, it can register ops(hisi_acc_vfio_pci_ops) that only perform encryption and decryption services, and can also register with live migration function ops(hisi_acc_vfio_pci_migrn_ops), and this debugfs only needs to register it when the the ops is hisi_acc_vfio_pci_migrn_ops.
Isn't the ops registration happens at probe()?
Both ops are registered in probe(), but if you move hisi_acc_debugfs_root() to probe(), this debugfs will be created regardless of whether you need to perform live migration.
In any case, I think you can move
the hisi_acc_debugfs_root creation to module_init() as suggested above and avoid the race.
mig_root_ref uses atomic to prevent race, and another function is for reference counting. Even if this hisi_acc_debugfs_root() is moved to module_init(), this mig_root_ref is still needed, because the driver needs to ensure that the debugfs file is created when the first VF is enabled, destroy it when the last VF have unloaded.
Thanks, Shameer
Thanks, Longfang.
Thanks, Longfang.