From: Cornelia Huck <cohuck@redhat.com> To: Halil Pasic <pasic@linux.ibm.com>, Halil Pasic <pasic@linux.ibm.com>, Heiko Carstens <hca@linux.ibm.com>, Vasily Gorbik <gor@linux.ibm.com>, Christian Borntraeger <borntraeger@de.ibm.com>, Pierre Morel <pmorel@linux.ibm.com>, Michael Mueller <mimu@linux.ibm.com>, linux-s390@vger.kernel.org, virtualization@lists.linux-foundation.org, kvm@vger.kernel.org, linux-kernel@vger.kernel.org Cc: bfu@redhat.com, Vineeth Vijayan <vneethv@linux.ibm.com> Subject: Re: [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown Date: Thu, 16 Sep 2021 10:59:15 +0200 [thread overview] Message-ID: <87pmt8hp5o.fsf@redhat.com> (raw) In-Reply-To: <20210915215742.1793314-1-pasic@linux.ibm.com> On Wed, Sep 15 2021, Halil Pasic <pasic@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@linux.ibm.com> > Fixes: 48720ba56891 ("virtio/s390: use DMA memory for ccw I/O and > classic notifiers") > Reported-by: bfu@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
WARNING: multiple messages have this Message-ID (diff)
From: Cornelia Huck <cohuck@redhat.com> To: Halil Pasic <pasic@linux.ibm.com>, Halil Pasic <pasic@linux.ibm.com>, Heiko Carstens <hca@linux.ibm.com>, Vasily Gorbik <gor@linux.ibm.com>, Christian Borntraeger <borntraeger@de.ibm.com>, Pierre Morel <pmorel@linux.ibm.com>, Michael Mueller <mimu@linux.ibm.com>, linux-s390@vger.kernel.org, virtualization@lists.linux-foundation.org, kvm@vger.kernel.org, linux-kernel@vger.kernel.org Cc: bfu@redhat.com, Vineeth Vijayan <vneethv@linux.ibm.com> Subject: Re: [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown Date: Thu, 16 Sep 2021 10:59:15 +0200 [thread overview] Message-ID: <87pmt8hp5o.fsf@redhat.com> (raw) In-Reply-To: <20210915215742.1793314-1-pasic@linux.ibm.com> On Wed, Sep 15 2021, Halil Pasic <pasic@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@linux.ibm.com> > Fixes: 48720ba56891 ("virtio/s390: use DMA memory for ccw I/O and > classic notifiers") > Reported-by: bfu@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 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
next prev parent reply other threads:[~2021-09-16 8:59 UTC|newest] Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-09-15 21:57 [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown Halil Pasic 2021-09-15 21:57 ` Halil Pasic 2021-09-15 22:00 ` Halil Pasic 2021-09-15 22:00 ` Halil Pasic 2021-09-16 8:59 ` Cornelia Huck [this message] 2021-09-16 8:59 ` Cornelia Huck 2021-09-16 13:18 ` Halil Pasic 2021-09-16 13:18 ` Halil Pasic 2021-09-17 8:40 ` Cornelia Huck 2021-09-17 8:40 ` Cornelia Huck 2021-09-19 22:39 ` Halil Pasic 2021-09-19 22:39 ` Halil Pasic 2021-09-20 7:41 ` Vineeth Vijayan 2021-09-20 10:07 ` Cornelia Huck 2021-09-20 10:07 ` Cornelia Huck 2021-09-21 3:25 ` Halil Pasic 2021-09-21 3:25 ` Halil Pasic 2021-09-21 12:09 ` Cornelia Huck 2021-09-21 12:09 ` Cornelia Huck 2021-09-21 13:31 ` Vineeth Vijayan 2021-09-21 16:52 ` Halil Pasic 2021-09-21 16:52 ` Halil Pasic 2021-09-21 18:25 ` Vineeth Vijayan 2021-09-20 10:30 ` Cornelia Huck 2021-09-20 10:30 ` Cornelia Huck 2021-09-20 13:27 ` Halil Pasic 2021-09-20 13:27 ` Halil Pasic
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87pmt8hp5o.fsf@redhat.com \ --to=cohuck@redhat.com \ --cc=bfu@redhat.com \ --cc=borntraeger@de.ibm.com \ --cc=gor@linux.ibm.com \ --cc=hca@linux.ibm.com \ --cc=kvm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-s390@vger.kernel.org \ --cc=mimu@linux.ibm.com \ --cc=pasic@linux.ibm.com \ --cc=pmorel@linux.ibm.com \ --cc=virtualization@lists.linux-foundation.org \ --cc=vneethv@linux.ibm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.