| Message ID | 20240815004617.2325269-1-philipchen@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | virtio_pmem: Add freeze/restore callbacks | expand |
Hi maintainers, Can anyone let me know if this patch makes sense? Any comment/feedback is appreciated. Thanks in advance! On Wed, Aug 14, 2024 at 5:46 PM Philip Chen <philipchen@chromium.org> wrote: > > Add basic freeze/restore PM callbacks to support hibernation (S4): > - On freeze, delete vq and quiesce the device to prepare for > snapshotting. > - On restore, re-init vq and mark DRIVER_OK. > > Signed-off-by: Philip Chen <philipchen@chromium.org> > --- > drivers/nvdimm/virtio_pmem.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/drivers/nvdimm/virtio_pmem.c b/drivers/nvdimm/virtio_pmem.c > index c9b97aeabf85..2396d19ce549 100644 > --- a/drivers/nvdimm/virtio_pmem.c > +++ b/drivers/nvdimm/virtio_pmem.c > @@ -143,6 +143,28 @@ static void virtio_pmem_remove(struct virtio_device *vdev) > virtio_reset_device(vdev); > } > > +static int virtio_pmem_freeze(struct virtio_device *vdev) > +{ > + vdev->config->del_vqs(vdev); > + virtio_reset_device(vdev); > + > + return 0; > +} > + > +static int virtio_pmem_restore(struct virtio_device *vdev) > +{ > + int ret; > + > + ret = init_vq(vdev->priv); > + if (ret) { > + dev_err(&vdev->dev, "failed to initialize virtio pmem's vq\n"); > + return ret; > + } > + virtio_device_ready(vdev); > + > + return 0; > +} > + > static unsigned int features[] = { > VIRTIO_PMEM_F_SHMEM_REGION, > }; > @@ -155,6 +177,8 @@ static struct virtio_driver virtio_pmem_driver = { > .validate = virtio_pmem_validate, > .probe = virtio_pmem_probe, > .remove = virtio_pmem_remove, > + .freeze = virtio_pmem_freeze, > + .restore = virtio_pmem_restore, > }; > > module_virtio_driver(virtio_pmem_driver); > -- > 2.46.0.76.ge559c4bf1a-goog >
Philip Chen wrote: > Hi maintainers, > > Can anyone let me know if this patch makes sense? > Any comment/feedback is appreciated. > Thanks in advance! I'm not an expert on virtio but the code looks ok on the surface. I've discussed this with Dan a bit and virtio-pmem is not heavily tested. Based on our discussion [1] I wonder if there is a way we can recreate this with QEMU to incorporate into our testing? Ira [1] https://lore.kernel.org/lkml/CA+cxXhnb2i5O7_BiOfKLth5Zwp5T62d6F6c39vnuT53cUkU_uw@mail.gmail.com/ > > On Wed, Aug 14, 2024 at 5:46 PM Philip Chen <philipchen@chromium.org> wrote: > > > > Add basic freeze/restore PM callbacks to support hibernation (S4): > > - On freeze, delete vq and quiesce the device to prepare for > > snapshotting. > > - On restore, re-init vq and mark DRIVER_OK. > > > > Signed-off-by: Philip Chen <philipchen@chromium.org> > > --- > > drivers/nvdimm/virtio_pmem.c | 24 ++++++++++++++++++++++++ > > 1 file changed, 24 insertions(+) > > > > diff --git a/drivers/nvdimm/virtio_pmem.c b/drivers/nvdimm/virtio_pmem.c > > index c9b97aeabf85..2396d19ce549 100644 > > --- a/drivers/nvdimm/virtio_pmem.c > > +++ b/drivers/nvdimm/virtio_pmem.c > > @@ -143,6 +143,28 @@ static void virtio_pmem_remove(struct virtio_device *vdev) > > virtio_reset_device(vdev); > > } > > > > +static int virtio_pmem_freeze(struct virtio_device *vdev) > > +{ > > + vdev->config->del_vqs(vdev); > > + virtio_reset_device(vdev); > > + > > + return 0; > > +} > > + > > +static int virtio_pmem_restore(struct virtio_device *vdev) > > +{ > > + int ret; > > + > > + ret = init_vq(vdev->priv); > > + if (ret) { > > + dev_err(&vdev->dev, "failed to initialize virtio pmem's vq\n"); > > + return ret; > > + } > > + virtio_device_ready(vdev); > > + > > + return 0; > > +} > > + > > static unsigned int features[] = { > > VIRTIO_PMEM_F_SHMEM_REGION, > > }; > > @@ -155,6 +177,8 @@ static struct virtio_driver virtio_pmem_driver = { > > .validate = virtio_pmem_validate, > > .probe = virtio_pmem_probe, > > .remove = virtio_pmem_remove, > > + .freeze = virtio_pmem_freeze, > > + .restore = virtio_pmem_restore, > > }; > > > > module_virtio_driver(virtio_pmem_driver); > > -- > > 2.46.0.76.ge559c4bf1a-goog > >
Hi On Wed, Sep 4, 2024 at 10:54 AM Ira Weiny <ira.weiny@intel.com> wrote: > > Philip Chen wrote: > > Hi maintainers, > > > > Can anyone let me know if this patch makes sense? > > Any comment/feedback is appreciated. > > Thanks in advance! > > I'm not an expert on virtio but the code looks ok on the surface. I've > discussed this with Dan a bit and virtio-pmem is not heavily tested. Thanks for your comments. I think this specific patch is not heavily involved with virtio spec details. This patch simply provides the basic freeze/restore PM callbacks for virtio_pmem, like people already did for the other virtio drivers. > > Based on our discussion [1] I wonder if there is a way we can recreate this > with QEMU to incorporate into our testing? Yes, these are how I test on crosvm, but I believe the same steps can be applied to QEMU: (1) Set pm_test_level to TEST_PLATFORM (build time change) (2) Write something to pmem (3) Make the device go through a freeze/restore cycle by writing `disk` to `/sys/power/state` (4) Validate the data written to pmem in (2) is still preserved Note: (a) The freeze/restore PM routines are sometimes called as the backup for suspend/resume PM routines in a suspend/resume cycle. In this case, we can also test freeze/restore PM routines with suspend/resume: i.e. skip (1) and write `mem` to `sys/power/state` in (3). (b) I also tried to set up QEMU for testing. But QEMU crashes when I try to freeze the device even without applying this patch. Since the issue seems to be irrelevant to pmem and most likely a QEMU setup problem on my end, I didn't spend more time enabling QEMU. > > Ira > > [1] https://lore.kernel.org/lkml/CA+cxXhnb2i5O7_BiOfKLth5Zwp5T62d6F6c39vnuT53cUkU_uw@mail.gmail.com/ > > > > > On Wed, Aug 14, 2024 at 5:46 PM Philip Chen <philipchen@chromium.org> wrote: > > > > > > Add basic freeze/restore PM callbacks to support hibernation (S4): > > > - On freeze, delete vq and quiesce the device to prepare for > > > snapshotting. > > > - On restore, re-init vq and mark DRIVER_OK. > > > > > > Signed-off-by: Philip Chen <philipchen@chromium.org> > > > --- > > > drivers/nvdimm/virtio_pmem.c | 24 ++++++++++++++++++++++++ > > > 1 file changed, 24 insertions(+) > > > > > > diff --git a/drivers/nvdimm/virtio_pmem.c b/drivers/nvdimm/virtio_pmem.c > > > index c9b97aeabf85..2396d19ce549 100644 > > > --- a/drivers/nvdimm/virtio_pmem.c > > > +++ b/drivers/nvdimm/virtio_pmem.c > > > @@ -143,6 +143,28 @@ static void virtio_pmem_remove(struct virtio_device *vdev) > > > virtio_reset_device(vdev); > > > } > > > > > > +static int virtio_pmem_freeze(struct virtio_device *vdev) > > > +{ > > > + vdev->config->del_vqs(vdev); > > > + virtio_reset_device(vdev); > > > + > > > + return 0; > > > +} > > > + > > > +static int virtio_pmem_restore(struct virtio_device *vdev) > > > +{ > > > + int ret; > > > + > > > + ret = init_vq(vdev->priv); > > > + if (ret) { > > > + dev_err(&vdev->dev, "failed to initialize virtio pmem's vq\n"); > > > + return ret; > > > + } > > > + virtio_device_ready(vdev); > > > + > > > + return 0; > > > +} > > > + > > > static unsigned int features[] = { > > > VIRTIO_PMEM_F_SHMEM_REGION, > > > }; > > > @@ -155,6 +177,8 @@ static struct virtio_driver virtio_pmem_driver = { > > > .validate = virtio_pmem_validate, > > > .probe = virtio_pmem_probe, > > > .remove = virtio_pmem_remove, > > > + .freeze = virtio_pmem_freeze, > > > + .restore = virtio_pmem_restore, > > > }; > > > > > > module_virtio_driver(virtio_pmem_driver); > > > -- > > > 2.46.0.76.ge559c4bf1a-goog > > > > >
Hi maintainers, Are there any other concerns I should address for this patch? On Mon, Sep 9, 2024 at 4:52 PM Philip Chen <philipchen@chromium.org> wrote: > > Hi > > On Wed, Sep 4, 2024 at 10:54 AM Ira Weiny <ira.weiny@intel.com> wrote: > > > > Philip Chen wrote: > > > Hi maintainers, > > > > > > Can anyone let me know if this patch makes sense? > > > Any comment/feedback is appreciated. > > > Thanks in advance! > > > > I'm not an expert on virtio but the code looks ok on the surface. I've > > discussed this with Dan a bit and virtio-pmem is not heavily tested. > > Thanks for your comments. > I think this specific patch is not heavily involved with virtio spec details. > This patch simply provides the basic freeze/restore PM callbacks for > virtio_pmem, like people already did for the other virtio drivers. > > > > > Based on our discussion [1] I wonder if there is a way we can recreate this > > with QEMU to incorporate into our testing? > > Yes, these are how I test on crosvm, but I believe the same steps can > be applied to QEMU: > (1) Set pm_test_level to TEST_PLATFORM (build time change) > (2) Write something to pmem > (3) Make the device go through a freeze/restore cycle by writing > `disk` to `/sys/power/state` > (4) Validate the data written to pmem in (2) is still preserved > > Note: > (a) The freeze/restore PM routines are sometimes called as the backup > for suspend/resume PM routines in a suspend/resume cycle. > In this case, we can also test freeze/restore PM routines with > suspend/resume: i.e. skip (1) and write `mem` to `sys/power/state` in > (3). > (b) I also tried to set up QEMU for testing. But QEMU crashes when I > try to freeze the device even without applying this patch. > Since the issue seems to be irrelevant to pmem and most likely a QEMU > setup problem on my end, I didn't spend more time enabling QEMU. > > > > > > > Ira > > > > [1] https://lore.kernel.org/lkml/CA+cxXhnb2i5O7_BiOfKLth5Zwp5T62d6F6c39vnuT53cUkU_uw@mail.gmail.com/ > > > > > > > > On Wed, Aug 14, 2024 at 5:46 PM Philip Chen <philipchen@chromium.org> wrote: > > > > > > > > Add basic freeze/restore PM callbacks to support hibernation (S4): > > > > - On freeze, delete vq and quiesce the device to prepare for > > > > snapshotting. > > > > - On restore, re-init vq and mark DRIVER_OK. > > > > > > > > Signed-off-by: Philip Chen <philipchen@chromium.org> > > > > --- > > > > drivers/nvdimm/virtio_pmem.c | 24 ++++++++++++++++++++++++ > > > > 1 file changed, 24 insertions(+) > > > > > > > > diff --git a/drivers/nvdimm/virtio_pmem.c b/drivers/nvdimm/virtio_pmem.c > > > > index c9b97aeabf85..2396d19ce549 100644 > > > > --- a/drivers/nvdimm/virtio_pmem.c > > > > +++ b/drivers/nvdimm/virtio_pmem.c > > > > @@ -143,6 +143,28 @@ static void virtio_pmem_remove(struct virtio_device *vdev) > > > > virtio_reset_device(vdev); > > > > } > > > > > > > > +static int virtio_pmem_freeze(struct virtio_device *vdev) > > > > +{ > > > > + vdev->config->del_vqs(vdev); > > > > + virtio_reset_device(vdev); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int virtio_pmem_restore(struct virtio_device *vdev) > > > > +{ > > > > + int ret; > > > > + > > > > + ret = init_vq(vdev->priv); > > > > + if (ret) { > > > > + dev_err(&vdev->dev, "failed to initialize virtio pmem's vq\n"); > > > > + return ret; > > > > + } > > > > + virtio_device_ready(vdev); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > static unsigned int features[] = { > > > > VIRTIO_PMEM_F_SHMEM_REGION, > > > > }; > > > > @@ -155,6 +177,8 @@ static struct virtio_driver virtio_pmem_driver = { > > > > .validate = virtio_pmem_validate, > > > > .probe = virtio_pmem_probe, > > > > .remove = virtio_pmem_remove, > > > > + .freeze = virtio_pmem_freeze, > > > > + .restore = virtio_pmem_restore, > > > > }; > > > > > > > > module_virtio_driver(virtio_pmem_driver); > > > > -- > > > > 2.46.0.76.ge559c4bf1a-goog > > > > > > > >
+CC MST > > Philip Chen wrote: > > > Hi maintainers, > > > > > > Can anyone let me know if this patch makes sense? > > > Any comment/feedback is appreciated. > > > Thanks in advance! > > > > I'm not an expert on virtio but the code looks ok on the surface. I've > > discussed this with Dan a bit and virtio-pmem is not heavily tested. > > Thanks for your comments. > I think this specific patch is not heavily involved with virtio spec details. > This patch simply provides the basic freeze/restore PM callbacks for > virtio_pmem, like people already did for the other virtio drivers. > > > > > Based on our discussion [1] I wonder if there is a way we can recreate this > > with QEMU to incorporate into our testing? > > Yes, these are how I test on crosvm, but I believe the same steps can > be applied to QEMU: > (1) Set pm_test_level to TEST_PLATFORM (build time change) > (2) Write something to pmem > (3) Make the device go through a freeze/restore cycle by writing > `disk` to `/sys/power/state` > (4) Validate the data written to pmem in (2) is still preserved > > Note: > (a) The freeze/restore PM routines are sometimes called as the backup > for suspend/resume PM routines in a suspend/resume cycle. > In this case, we can also test freeze/restore PM routines with > suspend/resume: i.e. skip (1) and write `mem` to `sys/power/state` in > (3). > (b) I also tried to set up QEMU for testing. But QEMU crashes when I > try to freeze the device even without applying this patch. > Since the issue seems to be irrelevant to pmem and most likely a QEMU > setup problem on my end, I didn't spend more time enabling QEMU. Thanks for the work! Did you check why QEMU was crashing, maybe because we did not support freeze/restore before so some missing NULL check? Any back trace you got? If it works in crossVM, it should work in Qemu as well? Thanks, Pankaj > > > > > > > Ira > > > > [1] https://lore.kernel.org/lkml/CA+cxXhnb2i5O7_BiOfKLth5Zwp5T62d6F6c39vnuT53cUkU_uw@mail.gmail.com/ > > > > > > > > On Wed, Aug 14, 2024 at 5:46 PM Philip Chen <philipchen@chromium.org> wrote: > > > > > > > > Add basic freeze/restore PM callbacks to support hibernation (S4): > > > > - On freeze, delete vq and quiesce the device to prepare for > > > > snapshotting. > > > > - On restore, re-init vq and mark DRIVER_OK. > > > > > > > > Signed-off-by: Philip Chen <philipchen@chromium.org> > > > > --- > > > > drivers/nvdimm/virtio_pmem.c | 24 ++++++++++++++++++++++++ > > > > 1 file changed, 24 insertions(+) > > > > > > > > diff --git a/drivers/nvdimm/virtio_pmem.c b/drivers/nvdimm/virtio_pmem.c > > > > index c9b97aeabf85..2396d19ce549 100644 > > > > --- a/drivers/nvdimm/virtio_pmem.c > > > > +++ b/drivers/nvdimm/virtio_pmem.c > > > > @@ -143,6 +143,28 @@ static void virtio_pmem_remove(struct virtio_device *vdev) > > > > virtio_reset_device(vdev); > > > > } > > > > > > > > +static int virtio_pmem_freeze(struct virtio_device *vdev) > > > > +{ > > > > + vdev->config->del_vqs(vdev); > > > > + virtio_reset_device(vdev); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int virtio_pmem_restore(struct virtio_device *vdev) > > > > +{ > > > > + int ret; > > > > + > > > > + ret = init_vq(vdev->priv); > > > > + if (ret) { > > > > + dev_err(&vdev->dev, "failed to initialize virtio pmem's vq\n"); > > > > + return ret; > > > > + } > > > > + virtio_device_ready(vdev); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > static unsigned int features[] = { > > > > VIRTIO_PMEM_F_SHMEM_REGION, > > > > }; > > > > @@ -155,6 +177,8 @@ static struct virtio_driver virtio_pmem_driver = { > > > > .validate = virtio_pmem_validate, > > > > .probe = virtio_pmem_probe, > > > > .remove = virtio_pmem_remove, > > > > + .freeze = virtio_pmem_freeze, > > > > + .restore = virtio_pmem_restore, > > > > }; > > > > > > > > module_virtio_driver(virtio_pmem_driver); > > > > -- > > > > 2.46.0.76.ge559c4bf1a-goog > > > > > > > >
diff --git a/drivers/nvdimm/virtio_pmem.c b/drivers/nvdimm/virtio_pmem.c index c9b97aeabf85..2396d19ce549 100644 --- a/drivers/nvdimm/virtio_pmem.c +++ b/drivers/nvdimm/virtio_pmem.c @@ -143,6 +143,28 @@ static void virtio_pmem_remove(struct virtio_device *vdev) virtio_reset_device(vdev); } +static int virtio_pmem_freeze(struct virtio_device *vdev) +{ + vdev->config->del_vqs(vdev); + virtio_reset_device(vdev); + + return 0; +} + +static int virtio_pmem_restore(struct virtio_device *vdev) +{ + int ret; + + ret = init_vq(vdev->priv); + if (ret) { + dev_err(&vdev->dev, "failed to initialize virtio pmem's vq\n"); + return ret; + } + virtio_device_ready(vdev); + + return 0; +} + static unsigned int features[] = { VIRTIO_PMEM_F_SHMEM_REGION, }; @@ -155,6 +177,8 @@ static struct virtio_driver virtio_pmem_driver = { .validate = virtio_pmem_validate, .probe = virtio_pmem_probe, .remove = virtio_pmem_remove, + .freeze = virtio_pmem_freeze, + .restore = virtio_pmem_restore, }; module_virtio_driver(virtio_pmem_driver);
Add basic freeze/restore PM callbacks to support hibernation (S4): - On freeze, delete vq and quiesce the device to prepare for snapshotting. - On restore, re-init vq and mark DRIVER_OK. Signed-off-by: Philip Chen <philipchen@chromium.org> --- drivers/nvdimm/virtio_pmem.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)