From: Halil Pasic <pasic@linux.ibm.com>
To: Cornelia Huck <cohuck@redhat.com>
Cc: 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, bfu@redhat.com,
Vineeth Vijayan <vneethv@linux.ibm.com>,
Halil Pasic <pasic@linux.ibm.com>
Subject: Re: [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown
Date: Thu, 16 Sep 2021 15:18:35 +0200 [thread overview]
Message-ID: <20210916151835.4ab512b2.pasic@linux.ibm.com> (raw)
In-Reply-To: <87pmt8hp5o.fsf@redhat.com>
On Thu, 16 Sep 2021 10:59:15 +0200
Cornelia Huck <cohuck@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@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.
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
next prev parent reply other threads:[~2021-09-16 13:18 UTC|newest]
Thread overview: 15+ 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 22:00 ` Halil Pasic
2021-09-16 8:59 ` Cornelia Huck
2021-09-16 13:18 ` Halil Pasic [this message]
2021-09-17 8:40 ` Cornelia Huck
2021-09-19 22:39 ` Halil Pasic
2021-09-20 7:41 ` Vineeth Vijayan
2021-09-20 10:07 ` Cornelia Huck
2021-09-21 3:25 ` Halil Pasic
2021-09-21 12:09 ` Cornelia Huck
2021-09-21 13:31 ` Vineeth Vijayan
2021-09-21 16:52 ` Halil Pasic
2021-09-21 18:25 ` Vineeth Vijayan
2021-09-20 10:30 ` Cornelia Huck
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=20210916151835.4ab512b2.pasic@linux.ibm.com \
--to=pasic@linux.ibm.com \
--cc=bfu@redhat.com \
--cc=borntraeger@de.ibm.com \
--cc=cohuck@redhat.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=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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).