Cornelia Huck
2021-Sep-16 08:59 UTC
[PATCH 1/1] virtio/s390: fix vritio-ccw device teardown
On Wed, Sep 15 2021, Halil Pasic <pasic at linux.ibm.com> wrote:> Since commit 48720ba56891 ("virtio/s390: use DMA memory for ccw I/O and > classic notifiers") we were supposed to make sure that > virtio_ccw_release_dev() completes before the ccw device, and the > attached dma pool are torn down, but unfortunately we did not. > Before that commit it used to be OK to delay cleaning up the memory > allocated by virtio-ccw indefinitely (which isn't really intuitive for > guys used to destruction happens in reverse construction order). > > To accomplish this let us take a reference on the ccw device before we > allocate the dma_area and give it up after dma_area was freed. > > Signed-off-by: Halil Pasic <pasic at linux.ibm.com> > Fixes: 48720ba56891 ("virtio/s390: use DMA memory for ccw I/O and > classic notifiers") > Reported-by: bfu at redhat.com > --- > > I'm not certain this is the only hot-unplug and teardonw related problem > with virtio-ccw. > > Some things that are not perfectly clear to me: > * What would happen if we observed an hot-unplug while we are doing > wait_event() in ccw_io_helper()? Do we get stuck? I don't thin we > are guaranteed to receive an irq for a subchannel that is gone.Hm. I think we may need to do a wake_up during remove handling.> * cdev->online seems to be manipulated under cdev->ccwlock, but > in virtio_ccw_remove() we look at it to decide should we clean up > or not. What is the idea there? I guess we want to avoid doing > if nothing is there or twice. But I don't understand how stuff > interlocks.We only created the virtio device when we onlined the ccw device. Do you have a better idea how to check for that? (And yes, I'm not sure the locking is correct.)> * Can virtio_ccw_remove() get called while !cdev->online and > virtio_ccw_online() is running on a different cpu? If yes, what would > happen then?All of the remove/online/... etc. callbacks are invoked via the ccw bus code. We have to trust that it gets it correct :) (Or have the common I/O layer maintainers double-check it.)> > The main addresse of these questions is Conny ;). > > An alternative to this approach would be to inc and dec the refcount > in ccw_device_dma_zalloc() and ccw_device_dma_free() respectively.Yeah, I also thought about that. This would give us more get/put operations, but might be the safer option.> > --- > drivers/s390/virtio/virtio_ccw.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c > index d35e7a3f7067..99141df3259b 100644 > --- a/drivers/s390/virtio/virtio_ccw.c > +++ b/drivers/s390/virtio/virtio_ccw.c > @@ -1006,10 +1006,12 @@ static void virtio_ccw_release_dev(struct device *_d) > { > struct virtio_device *dev = dev_to_virtio(_d); > struct virtio_ccw_device *vcdev = to_vc_device(dev); > + struct ccw_device *cdev = READ_ONCE(vcdev->cdev); > > ccw_device_dma_free(vcdev->cdev, vcdev->dma_area, > sizeof(*vcdev->dma_area)); > kfree(vcdev); > + put_device(&cdev->dev); > } > > static int irb_is_error(struct irb *irb) > @@ -1262,6 +1264,7 @@ static int virtio_ccw_online(struct ccw_device *cdev) > struct virtio_ccw_device *vcdev; > unsigned long flags; > > + get_device(&cdev->dev); > vcdev = kzalloc(sizeof(*vcdev), GFP_KERNEL); > if (!vcdev) { > dev_warn(&cdev->dev, "Could not get memory for virtio\n"); > @@ -1315,6 +1318,7 @@ static int virtio_ccw_online(struct ccw_device *cdev) > sizeof(*vcdev->dma_area)); > } > kfree(vcdev); > + put_device(&cdev->dev); > return ret; > } > > > base-commit: 3ca706c189db861b2ca2019a0901b94050ca49d8 > -- > 2.25.1
On Thu, 16 Sep 2021 10:59:15 +0200 Cornelia Huck <cohuck at redhat.com> wrote:> > Since commit 48720ba56891 ("virtio/s390: use DMA memory for ccw I/O and > > classic notifiers") we were supposed to make sure that > > virtio_ccw_release_dev() completes before the ccw device, and the > > attached dma pool are torn down, but unfortunately we did not. > > Before that commit it used to be OK to delay cleaning up the memory > > allocated by virtio-ccw indefinitely (which isn't really intuitive for > > guys used to destruction happens in reverse construction order). > > > > To accomplish this let us take a reference on the ccw device before we > > allocate the dma_area and give it up after dma_area was freed. > > > > Signed-off-by: Halil Pasic <pasic at linux.ibm.com> > > Fixes: 48720ba56891 ("virtio/s390: use DMA memory for ccw I/O and > > classic notifiers") > > Reported-by: bfu at redhat.com > > --- > > > > I'm not certain this is the only hot-unplug and teardonw related problem > > with virtio-ccw. > > > > Some things that are not perfectly clear to me: > > * What would happen if we observed an hot-unplug while we are doing > > wait_event() in ccw_io_helper()? Do we get stuck? I don't thin we > > are guaranteed to receive an irq for a subchannel that is gone. > > Hm. I think we may need to do a wake_up during remove handling.My guess is that the BQL is saving us from ever seeing this with QEMU as the hypervisor-userspace. Nevertheless I don't think we should rely on that.> > > * cdev->online seems to be manipulated under cdev->ccwlock, but > > in virtio_ccw_remove() we look at it to decide should we clean up > > or not. What is the idea there? I guess we want to avoid doing > > if nothing is there or twice. But I don't understand how stuff > > interlocks. > > We only created the virtio device when we onlined the ccw device. Do you > have a better idea how to check for that? (And yes, I'm not sure the > locking is correct.) >Thanks, if I find time for it, I will try to understand this better and come back with my findings.> > * Can virtio_ccw_remove() get called while !cdev->online and > > virtio_ccw_online() is running on a different cpu? If yes, what would > > happen then? > > All of the remove/online/... etc. callbacks are invoked via the ccw bus > code. We have to trust that it gets it correct :) (Or have the common > I/O layer maintainers double-check it.) >Vineeth, what is your take on this? Are the struct ccw_driver virtio_ccw_remove and the virtio_ccw_online callbacks mutually exclusive. Please notice that we may initiate the onlining by calling ccw_device_set_online() from a workqueue. @Conny: I'm not sure what is your definition of 'it gets it correct'... I doubt CIO can make things 100% foolproof in this area.> > > > The main addresse of these questions is Conny ;).In any case, I think we can go step by step. I would like the issue this patch intends to address, addressed first. Then we can think about the rest.> > > > An alternative to this approach would be to inc and dec the refcount > > in ccw_device_dma_zalloc() and ccw_device_dma_free() respectively. > > Yeah, I also thought about that. This would give us more get/put > operations, but might be the safer option.My understanding is, that having the ccw device go away while in a middle of doing ccw stuff (about to submit, or waiting for a channel program, or whatever) was bad before. So my intuition tells me that drivers should manage explicitly. Yes virtio_ccw happens to have dma memory whose lifetime is more or less the lifetime of struct virtio_ccw, but that may not be always the case. Thanks for your comments! Regards, Halil