[PATCH 1/1] virtio/s390: fix vritio-ccw device teardown

[PATCH 1/1] virtio/s390: fix vritio-ccw device teardown

[Halil Pasic]

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.
* 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.
* 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?
 
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.

---
 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

[PATCH 1/1] virtio/s390: fix vritio-ccw device teardown

[Halil Pasic]

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.
* 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.
* 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?
 
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.

---
 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

Re: [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown

[Halil Pasic]

s/vritio/virtio/
(subject)

[..]

On Wed, 15 Sep 2021 23:57:42 +0200
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).
> 

[..]

Re: [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown

[Halil Pasic]

s/vritio/virtio/
(subject)

[..]

On Wed, 15 Sep 2021 23:57:42 +0200
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).
> 

[..]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown

[Cornelia Huck]

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

Re: [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown

[Cornelia Huck]

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

Re: [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown

[Halil Pasic]

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

Re: [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown

[Halil Pasic]

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown

[Cornelia Huck]

On Thu, Sep 16 2021, Halil Pasic <pasic@linux.ibm.com> wrote:

> 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.

I agree. Let's do that via a separate patch.

>
>> 
>> > * 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.

Not 100% foolproof, but "don't online a device that is in the progress
of going away" seems pretty basic to me.

>
>> >  
>> > 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.

What do you mean with "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.

I'm not sure what you're getting at here. Regardless of the lifetime of
the dma memory, it depends on the presence of the ccw device to which it
is tied. This means that the ccw device must not be released while the
dma memory is alive. We can use the approach in your patch here due to
the lifetime of the dma memory that virtio-ccw allocates when we start
using the device and frees when we stop using the device, or we can use
get/put with every allocate/release dma memory pair, which should be
safe for everyone?

Re: [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown

[Cornelia Huck]

On Thu, Sep 16 2021, Halil Pasic <pasic@linux.ibm.com> wrote:

> 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.

I agree. Let's do that via a separate patch.

>
>> 
>> > * 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.

Not 100% foolproof, but "don't online a device that is in the progress
of going away" seems pretty basic to me.

>
>> >  
>> > 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.

What do you mean with "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.

I'm not sure what you're getting at here. Regardless of the lifetime of
the dma memory, it depends on the presence of the ccw device to which it
is tied. This means that the ccw device must not be released while the
dma memory is alive. We can use the approach in your patch here due to
the lifetime of the dma memory that virtio-ccw allocates when we start
using the device and frees when we stop using the device, or we can use
get/put with every allocate/release dma memory pair, which should be
safe for everyone?

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown

[Halil Pasic]

On Fri, 17 Sep 2021 10:40:20 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Thu, Sep 16 2021, Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > 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.  
> 
> I agree. Let's do that via a separate patch.
> 

I understand you would like us to finish the discussion on the alternate
approach before giving an r-b for this patch, right?

> >  
> >>   
> >> > * 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.  
> 
> Not 100% foolproof, but "don't online a device that is in the progress
> of going away" seems pretty basic to me.
> 

I hope Vineeth will chime in on this.

> >  
> >> >  
> >> > 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.  
> 
> What do you mean with "was bad before"?

Using an already invalid pointer to the ccw device is always bad. I'm
not sure what prevented this from happening before commit 48720ba56891.
I'm aware of the fact that virtio_ccw_release_dev() didn't use to
deference the vcdev->cdev before that commit, so we didn't have this
exact problem. Can you tell me, how did we use to ensure that all
dereferences of vcdev->cdev are legit, i.e. happened while the
ccw device is still fully alive before commit 48720ba56891?

> 
> > 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.  
> 
> I'm not sure what you're getting at here. Regardless of the lifetime of
> the dma memory, it depends on the presence of the ccw device to which it
> is tied. This means that the ccw device must not be released while the
> dma memory is alive. We can use the approach in your patch here due to
> the lifetime of the dma memory that virtio-ccw allocates when we start
> using the device and frees when we stop using the device, or we can use
> get/put with every allocate/release dma memory pair, which should be
> safe for everyone?
> 

What I mean is that ccw_device_dma_[zalloc,free]() take a pointer to the
ccw_device. If we get/put in those we can ensure that, provided the
alloc and the free calls are properly paired, the device will be still
alive (and the pointer valid) for the free, if it was valid for the
alloc. But it does not ensure that each and every call to alloc is with
a valid pointer, or that other uses of the pointer are OK. So I don't
think it is completely safe for everyone, because we could try to use
a pointer to a ccw device when not having any dma memory allocated from
its pool.

This patch takes reference to cdev before the pointer is published via
vcdev->cdev and drops the reference after *vcdev is freed. The idea is
that the pointee basically outlives the pointer. (Without having a full
understanding of how things are synchronized).

Regards,
Halil

Re: [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown

[Halil Pasic]

On Fri, 17 Sep 2021 10:40:20 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Thu, Sep 16 2021, Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > 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.  
> 
> I agree. Let's do that via a separate patch.
> 

I understand you would like us to finish the discussion on the alternate
approach before giving an r-b for this patch, right?

> >  
> >>   
> >> > * 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.  
> 
> Not 100% foolproof, but "don't online a device that is in the progress
> of going away" seems pretty basic to me.
> 

I hope Vineeth will chime in on this.

> >  
> >> >  
> >> > 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.  
> 
> What do you mean with "was bad before"?

Using an already invalid pointer to the ccw device is always bad. I'm
not sure what prevented this from happening before commit 48720ba56891.
I'm aware of the fact that virtio_ccw_release_dev() didn't use to
deference the vcdev->cdev before that commit, so we didn't have this
exact problem. Can you tell me, how did we use to ensure that all
dereferences of vcdev->cdev are legit, i.e. happened while the
ccw device is still fully alive before commit 48720ba56891?

> 
> > 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.  
> 
> I'm not sure what you're getting at here. Regardless of the lifetime of
> the dma memory, it depends on the presence of the ccw device to which it
> is tied. This means that the ccw device must not be released while the
> dma memory is alive. We can use the approach in your patch here due to
> the lifetime of the dma memory that virtio-ccw allocates when we start
> using the device and frees when we stop using the device, or we can use
> get/put with every allocate/release dma memory pair, which should be
> safe for everyone?
> 

What I mean is that ccw_device_dma_[zalloc,free]() take a pointer to the
ccw_device. If we get/put in those we can ensure that, provided the
alloc and the free calls are properly paired, the device will be still
alive (and the pointer valid) for the free, if it was valid for the
alloc. But it does not ensure that each and every call to alloc is with
a valid pointer, or that other uses of the pointer are OK. So I don't
think it is completely safe for everyone, because we could try to use
a pointer to a ccw device when not having any dma memory allocated from
its pool.

This patch takes reference to cdev before the pointer is published via
vcdev->cdev and drops the reference after *vcdev is freed. The idea is
that the pointee basically outlives the pointer. (Without having a full
understanding of how things are synchronized).

Regards,
Halil
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown

[Vineeth Vijayan]

On Mon, 2021-09-20 at 00:39 +0200, Halil Pasic wrote:
> On Fri, 17 Sep 2021 10:40:20 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
...snip...
> > > 
> > > 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.  
> > 
> > Not 100% foolproof, but "don't online a device that is in the
> > progress
> > of going away" seems pretty basic to me.
> > 
> 
> I hope Vineeth will chime in on this.
Considering the online/offline processing, 
The ccw_device_set_offline function or the online/offline is handled
inside device_lock. Also, the online_store function takes care of
avoiding multiple online/offline processing. 

Now, when we consider the unconditional remove of the device,
I am not familiar with the virtio_ccw driver. My assumptions are based
on how CIO/dasd drivers works. If i understand correctly, the dasd
driver sets different flags to make sure that a device_open is getting
prevented while the the device is in progress of offline-ing. 

> 
> > >  
> > > > >  
> > > > > The main addresse of these questions is Conny ;).  
> > > 

...snip...

Re: [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown

[Cornelia Huck]

On Mon, Sep 20 2021, Vineeth Vijayan <vneethv@linux.ibm.com> wrote:

> On Mon, 2021-09-20 at 00:39 +0200, Halil Pasic wrote:
>> On Fri, 17 Sep 2021 10:40:20 +0200
>> Cornelia Huck <cohuck@redhat.com> wrote:
>> 
> ...snip...
>> > > 
>> > > 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.  
>> > 
>> > Not 100% foolproof, but "don't online a device that is in the
>> > progress
>> > of going away" seems pretty basic to me.
>> > 
>> 
>> I hope Vineeth will chime in on this.
> Considering the online/offline processing, 
> The ccw_device_set_offline function or the online/offline is handled
> inside device_lock. Also, the online_store function takes care of
> avoiding multiple online/offline processing. 
>
> Now, when we consider the unconditional remove of the device,
> I am not familiar with the virtio_ccw driver. My assumptions are based
> on how CIO/dasd drivers works. If i understand correctly, the dasd
> driver sets different flags to make sure that a device_open is getting
> prevented while the the device is in progress of offline-ing. 

Hm, if we are invoking the online/offline callbacks under the device
lock already, how would that affect the remove callback? Shouldn't they
be serialized under the device lock already? I think we are fine.

For dasd, I think they also need to deal with the block device
lifetimes. For virtio-ccw, we are basically a transport that does not
know about devices further down the chain (that are associated with the
virtio device, whose lifetime is tied to online/offline processing.) I'd
presume that the serialization above would be enough.

Re: [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown

[Cornelia Huck]

On Mon, Sep 20 2021, Vineeth Vijayan <vneethv@linux.ibm.com> wrote:

> On Mon, 2021-09-20 at 00:39 +0200, Halil Pasic wrote:
>> On Fri, 17 Sep 2021 10:40:20 +0200
>> Cornelia Huck <cohuck@redhat.com> wrote:
>> 
> ...snip...
>> > > 
>> > > 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.  
>> > 
>> > Not 100% foolproof, but "don't online a device that is in the
>> > progress
>> > of going away" seems pretty basic to me.
>> > 
>> 
>> I hope Vineeth will chime in on this.
> Considering the online/offline processing, 
> The ccw_device_set_offline function or the online/offline is handled
> inside device_lock. Also, the online_store function takes care of
> avoiding multiple online/offline processing. 
>
> Now, when we consider the unconditional remove of the device,
> I am not familiar with the virtio_ccw driver. My assumptions are based
> on how CIO/dasd drivers works. If i understand correctly, the dasd
> driver sets different flags to make sure that a device_open is getting
> prevented while the the device is in progress of offline-ing. 

Hm, if we are invoking the online/offline callbacks under the device
lock already, how would that affect the remove callback? Shouldn't they
be serialized under the device lock already? I think we are fine.

For dasd, I think they also need to deal with the block device
lifetimes. For virtio-ccw, we are basically a transport that does not
know about devices further down the chain (that are associated with the
virtio device, whose lifetime is tied to online/offline processing.) I'd
presume that the serialization above would be enough.

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown

[Cornelia Huck]

On Mon, Sep 20 2021, Halil Pasic <pasic@linux.ibm.com> wrote:

> On Fri, 17 Sep 2021 10:40:20 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
>
>> On Thu, Sep 16 2021, Halil Pasic <pasic@linux.ibm.com> wrote:
>> 
>> > 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.  
>> 
>> I agree. Let's do that via a separate patch.
>> 
>
> I understand you would like us to finish the discussion on the alternate
> approach before giving an r-b for this patch, right?

Yes, exactly.

(...)

>> >> > 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.  
>> 
>> What do you mean with "was bad before"?
>
> Using an already invalid pointer to the ccw device is always bad. I'm
> not sure what prevented this from happening before commit 48720ba56891.
> I'm aware of the fact that virtio_ccw_release_dev() didn't use to
> deference the vcdev->cdev before that commit, so we didn't have this
> exact problem. Can you tell me, how did we use to ensure that all
> dereferences of vcdev->cdev are legit, i.e. happened while the
> ccw device is still fully alive before commit 48720ba56891?

I'm not sure what that commit is having to do with lifetimes, it did not
change anything, only added the extra interaction for the dma buffer.

Basically, the vcdev is supposed to be around while the ccw device is
online (with a tail end until references have been given up, of course.)
It embeds a virtio device that has the ccw device as a parent, which
will give us a reference on the ccw device as long as the virtio device
is alive. Any interactions with the ccw device (except freeing the dma
buffer) are limited to the time where we still have a reference to it
via the virtio device.

>
>> 
>> > 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.  
>> 
>> I'm not sure what you're getting at here. Regardless of the lifetime of
>> the dma memory, it depends on the presence of the ccw device to which it
>> is tied. This means that the ccw device must not be released while the
>> dma memory is alive. We can use the approach in your patch here due to
>> the lifetime of the dma memory that virtio-ccw allocates when we start
>> using the device and frees when we stop using the device, or we can use
>> get/put with every allocate/release dma memory pair, which should be
>> safe for everyone?
>> 
>
> What I mean is that ccw_device_dma_[zalloc,free]() take a pointer to the
> ccw_device. If we get/put in those we can ensure that, provided the
> alloc and the free calls are properly paired, the device will be still
> alive (and the pointer valid) for the free, if it was valid for the
> alloc. But it does not ensure that each and every call to alloc is with
> a valid pointer, or that other uses of the pointer are OK. So I don't
> think it is completely safe for everyone, because we could try to use
> a pointer to a ccw device when not having any dma memory allocated from
> its pool.

But the problem is the dma memory, right? Also, it is the same issue for
any potential caller of the ccw_device_dma_* interfaces.

>
> This patch takes reference to cdev before the pointer is published via
> vcdev->cdev and drops the reference after *vcdev is freed. The idea is
> that the pointee basically outlives the pointer. (Without having a full
> understanding of how things are synchronized).

I don't think we have to care about accessing ->cdev (see above.) Plus,
as we give up the dma memory at the very last point, we would also give
up the reference via that memory at the very last point, so I'm not sure
what additional problems could come up.

Re: [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown

[Cornelia Huck]

On Mon, Sep 20 2021, Halil Pasic <pasic@linux.ibm.com> wrote:

> On Fri, 17 Sep 2021 10:40:20 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
>
>> On Thu, Sep 16 2021, Halil Pasic <pasic@linux.ibm.com> wrote:
>> 
>> > 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.  
>> 
>> I agree. Let's do that via a separate patch.
>> 
>
> I understand you would like us to finish the discussion on the alternate
> approach before giving an r-b for this patch, right?

Yes, exactly.

(...)

>> >> > 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.  
>> 
>> What do you mean with "was bad before"?
>
> Using an already invalid pointer to the ccw device is always bad. I'm
> not sure what prevented this from happening before commit 48720ba56891.
> I'm aware of the fact that virtio_ccw_release_dev() didn't use to
> deference the vcdev->cdev before that commit, so we didn't have this
> exact problem. Can you tell me, how did we use to ensure that all
> dereferences of vcdev->cdev are legit, i.e. happened while the
> ccw device is still fully alive before commit 48720ba56891?

I'm not sure what that commit is having to do with lifetimes, it did not
change anything, only added the extra interaction for the dma buffer.

Basically, the vcdev is supposed to be around while the ccw device is
online (with a tail end until references have been given up, of course.)
It embeds a virtio device that has the ccw device as a parent, which
will give us a reference on the ccw device as long as the virtio device
is alive. Any interactions with the ccw device (except freeing the dma
buffer) are limited to the time where we still have a reference to it
via the virtio device.

>
>> 
>> > 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.  
>> 
>> I'm not sure what you're getting at here. Regardless of the lifetime of
>> the dma memory, it depends on the presence of the ccw device to which it
>> is tied. This means that the ccw device must not be released while the
>> dma memory is alive. We can use the approach in your patch here due to
>> the lifetime of the dma memory that virtio-ccw allocates when we start
>> using the device and frees when we stop using the device, or we can use
>> get/put with every allocate/release dma memory pair, which should be
>> safe for everyone?
>> 
>
> What I mean is that ccw_device_dma_[zalloc,free]() take a pointer to the
> ccw_device. If we get/put in those we can ensure that, provided the
> alloc and the free calls are properly paired, the device will be still
> alive (and the pointer valid) for the free, if it was valid for the
> alloc. But it does not ensure that each and every call to alloc is with
> a valid pointer, or that other uses of the pointer are OK. So I don't
> think it is completely safe for everyone, because we could try to use
> a pointer to a ccw device when not having any dma memory allocated from
> its pool.

But the problem is the dma memory, right? Also, it is the same issue for
any potential caller of the ccw_device_dma_* interfaces.

>
> This patch takes reference to cdev before the pointer is published via
> vcdev->cdev and drops the reference after *vcdev is freed. The idea is
> that the pointee basically outlives the pointer. (Without having a full
> understanding of how things are synchronized).

I don't think we have to care about accessing ->cdev (see above.) Plus,
as we give up the dma memory at the very last point, we would also give
up the reference via that memory at the very last point, so I'm not sure
what additional problems could come up.

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown

[Halil Pasic]

On Mon, 20 Sep 2021 12:30:39 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Mon, Sep 20 2021, Halil Pasic <pasic@linux.ibm.com> wrote:
[..]
> 
> Basically, the vcdev is supposed to be around while the ccw device is
> online (with a tail end until references have been given up, of course.)
> It embeds a virtio device that has the ccw device as a parent, which
> will give us a reference on the ccw device as long as the virtio device
> is alive. Any interactions with the ccw device (except freeing the dma
> buffer) are limited to the time where we still have a reference to it
> via the virtio device.
>

I didn't remember that device_add() takes a reference to the parent, and
that device_del() before device_put(dev) and remove callback.


> >  
> >>   
> >> > 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.    
> >> 
> >> I'm not sure what you're getting at here. Regardless of the lifetime of
> >> the dma memory, it depends on the presence of the ccw device to which it
> >> is tied. This means that the ccw device must not be released while the
> >> dma memory is alive. We can use the approach in your patch here due to
> >> the lifetime of the dma memory that virtio-ccw allocates when we start
> >> using the device and frees when we stop using the device, or we can use
> >> get/put with every allocate/release dma memory pair, which should be
> >> safe for everyone?
> >>   
> >
> > What I mean is that ccw_device_dma_[zalloc,free]() take a pointer to the
> > ccw_device. If we get/put in those we can ensure that, provided the
> > alloc and the free calls are properly paired, the device will be still
> > alive (and the pointer valid) for the free, if it was valid for the
> > alloc. But it does not ensure that each and every call to alloc is with
> > a valid pointer, or that other uses of the pointer are OK. So I don't
> > think it is completely safe for everyone, because we could try to use
> > a pointer to a ccw device when not having any dma memory allocated from
> > its pool.  
> 
> But the problem is the dma memory, right? Also, it is the same issue for
> any potential caller of the ccw_device_dma_* interfaces.

I tend to agree, my argument was based on the assumption that we did not
use to take a reference to the ccw device in virtio_ccw_online(), but we
do via register_virtio_device(). This reference however gets dropped
right before virtio_ccw_release_dev() is called.
> 
> >
> > This patch takes reference to cdev before the pointer is published via
> > vcdev->cdev and drops the reference after *vcdev is freed. The idea is
> > that the pointee basically outlives the pointer. (Without having a full
> > understanding of how things are synchronized).  
> 
> I don't think we have to care about accessing ->cdev (see above.) Plus,
> as we give up the dma memory at the very last point, we would also give
> up the reference via that memory at the very last point, so I'm not sure
> what additional problems could come up.

I understand now. Let me think about it some more. I'm wonderning about
leafs. Will come back at you shortly.

Regards,
Halil

Re: [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown

[Halil Pasic]

On Mon, 20 Sep 2021 12:30:39 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Mon, Sep 20 2021, Halil Pasic <pasic@linux.ibm.com> wrote:
[..]
> 
> Basically, the vcdev is supposed to be around while the ccw device is
> online (with a tail end until references have been given up, of course.)
> It embeds a virtio device that has the ccw device as a parent, which
> will give us a reference on the ccw device as long as the virtio device
> is alive. Any interactions with the ccw device (except freeing the dma
> buffer) are limited to the time where we still have a reference to it
> via the virtio device.
>

I didn't remember that device_add() takes a reference to the parent, and
that device_del() before device_put(dev) and remove callback.


> >  
> >>   
> >> > 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.    
> >> 
> >> I'm not sure what you're getting at here. Regardless of the lifetime of
> >> the dma memory, it depends on the presence of the ccw device to which it
> >> is tied. This means that the ccw device must not be released while the
> >> dma memory is alive. We can use the approach in your patch here due to
> >> the lifetime of the dma memory that virtio-ccw allocates when we start
> >> using the device and frees when we stop using the device, or we can use
> >> get/put with every allocate/release dma memory pair, which should be
> >> safe for everyone?
> >>   
> >
> > What I mean is that ccw_device_dma_[zalloc,free]() take a pointer to the
> > ccw_device. If we get/put in those we can ensure that, provided the
> > alloc and the free calls are properly paired, the device will be still
> > alive (and the pointer valid) for the free, if it was valid for the
> > alloc. But it does not ensure that each and every call to alloc is with
> > a valid pointer, or that other uses of the pointer are OK. So I don't
> > think it is completely safe for everyone, because we could try to use
> > a pointer to a ccw device when not having any dma memory allocated from
> > its pool.  
> 
> But the problem is the dma memory, right? Also, it is the same issue for
> any potential caller of the ccw_device_dma_* interfaces.

I tend to agree, my argument was based on the assumption that we did not
use to take a reference to the ccw device in virtio_ccw_online(), but we
do via register_virtio_device(). This reference however gets dropped
right before virtio_ccw_release_dev() is called.
> 
> >
> > This patch takes reference to cdev before the pointer is published via
> > vcdev->cdev and drops the reference after *vcdev is freed. The idea is
> > that the pointee basically outlives the pointer. (Without having a full
> > understanding of how things are synchronized).  
> 
> I don't think we have to care about accessing ->cdev (see above.) Plus,
> as we give up the dma memory at the very last point, we would also give
> up the reference via that memory at the very last point, so I'm not sure
> what additional problems could come up.

I understand now. Let me think about it some more. I'm wonderning about
leafs. Will come back at you shortly.

Regards,
Halil
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown

[Halil Pasic]

On Mon, 20 Sep 2021 12:07:23 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Mon, Sep 20 2021, Vineeth Vijayan <vneethv@linux.ibm.com> wrote:
> 
> > On Mon, 2021-09-20 at 00:39 +0200, Halil Pasic wrote:  
> >> On Fri, 17 Sep 2021 10:40:20 +0200
> >> Cornelia Huck <cohuck@redhat.com> wrote:
> >>   
> > ...snip...  
> >> > > 
> >> > > 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.    
> >> > 
> >> > Not 100% foolproof, but "don't online a device that is in the
> >> > progress
> >> > of going away" seems pretty basic to me.
> >> >   
> >> 
> >> I hope Vineeth will chime in on this.  
> > Considering the online/offline processing, 
> > The ccw_device_set_offline function or the online/offline is handled
> > inside device_lock. Also, the online_store function takes care of
> > avoiding multiple online/offline processing. 
> >
> > Now, when we consider the unconditional remove of the device,
> > I am not familiar with the virtio_ccw driver. My assumptions are based
> > on how CIO/dasd drivers works. If i understand correctly, the dasd
> > driver sets different flags to make sure that a device_open is getting
> > prevented while the the device is in progress of offline-ing.   
> 
> Hm, if we are invoking the online/offline callbacks under the device
> lock already, 

I believe we have a misunderstanding here. I believe that Vineeth is
trying to tell us, that online_store_handle_offline() and
online_store_handle_offline() are called under the a device lock of
the ccw device. Right, Vineeth?

Conny, I believe, by online/offline callbacks, you mean
virtio_ccw_online() and virtio_ccw_offline(), right?

But the thing is that virtio_ccw_online() may get called (and is
typically called, AFAICT) with no locks held via:
virtio_ccw_probe() --> async_schedule(virtio_ccw_auto_online, cdev)
-*-> virtio_ccw_auto_online(cdev) --> ccw_device_set_online(cdev) -->
virtio_ccw_online()

Furthermore after a closer look, I believe because we don't take
a reference to the cdev in probe, we may get virtio_ccw_auto_online()
called with an invalid pointer (the pointer is guaranteed to be valid
in probe, but because of async we have no guarantee that it will be
called in the context of probe).

Shouldn't we take a reference to the cdev in probe? BTW what is the
reason for the async?


> how would that affect the remove callback?

I believe dev->bus->remove(dev) is called by 
bus_remove_device() with the device lock held. I.e. I believe that means
that virtio_ccw_remove() is called with the ccw devices device lock
held. Vineeth can you confirm that?


The thing is, both virtio_ccw_remove() and virtio_ccw_offline() are
very similar, with the notable exception that offline assumes we are
online() at the moment, while remove() does the same only if it
decides based on vcdev && cdev->online that we are online.


> Shouldn't they
> be serialized under the device lock already? I think we are fine.

AFAICT virtio_ccw_remove() and virtio_ccw_offline() are serialized
against each other under the device lock. And also against
virtio_ccw_online() iff it was initiated via the sysfs, and not via
the auto-online mechanism.

Thus I don't think we are fine at the moment.

> 
> For dasd, I think they also need to deal with the block device
> lifetimes. For virtio-ccw, we are basically a transport that does not
> know about devices further down the chain (that are associated with the
> virtio device, whose lifetime is tied to online/offline processing.) I'd
> presume that the serialization above would be enough.
> 

I don't know about dasd that much. For the reasons stated above, I don't
think the serialization we have right now is entirely sufficient.

Regards,
Halil
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown

[Halil Pasic]

On Mon, 20 Sep 2021 12:07:23 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Mon, Sep 20 2021, Vineeth Vijayan <vneethv@linux.ibm.com> wrote:
> 
> > On Mon, 2021-09-20 at 00:39 +0200, Halil Pasic wrote:  
> >> On Fri, 17 Sep 2021 10:40:20 +0200
> >> Cornelia Huck <cohuck@redhat.com> wrote:
> >>   
> > ...snip...  
> >> > > 
> >> > > 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.    
> >> > 
> >> > Not 100% foolproof, but "don't online a device that is in the
> >> > progress
> >> > of going away" seems pretty basic to me.
> >> >   
> >> 
> >> I hope Vineeth will chime in on this.  
> > Considering the online/offline processing, 
> > The ccw_device_set_offline function or the online/offline is handled
> > inside device_lock. Also, the online_store function takes care of
> > avoiding multiple online/offline processing. 
> >
> > Now, when we consider the unconditional remove of the device,
> > I am not familiar with the virtio_ccw driver. My assumptions are based
> > on how CIO/dasd drivers works. If i understand correctly, the dasd
> > driver sets different flags to make sure that a device_open is getting
> > prevented while the the device is in progress of offline-ing.   
> 
> Hm, if we are invoking the online/offline callbacks under the device
> lock already, 

I believe we have a misunderstanding here. I believe that Vineeth is
trying to tell us, that online_store_handle_offline() and
online_store_handle_offline() are called under the a device lock of
the ccw device. Right, Vineeth?

Conny, I believe, by online/offline callbacks, you mean
virtio_ccw_online() and virtio_ccw_offline(), right?

But the thing is that virtio_ccw_online() may get called (and is
typically called, AFAICT) with no locks held via:
virtio_ccw_probe() --> async_schedule(virtio_ccw_auto_online, cdev)
-*-> virtio_ccw_auto_online(cdev) --> ccw_device_set_online(cdev) -->
virtio_ccw_online()

Furthermore after a closer look, I believe because we don't take
a reference to the cdev in probe, we may get virtio_ccw_auto_online()
called with an invalid pointer (the pointer is guaranteed to be valid
in probe, but because of async we have no guarantee that it will be
called in the context of probe).

Shouldn't we take a reference to the cdev in probe? BTW what is the
reason for the async?


> how would that affect the remove callback?

I believe dev->bus->remove(dev) is called by 
bus_remove_device() with the device lock held. I.e. I believe that means
that virtio_ccw_remove() is called with the ccw devices device lock
held. Vineeth can you confirm that?


The thing is, both virtio_ccw_remove() and virtio_ccw_offline() are
very similar, with the notable exception that offline assumes we are
online() at the moment, while remove() does the same only if it
decides based on vcdev && cdev->online that we are online.


> Shouldn't they
> be serialized under the device lock already? I think we are fine.

AFAICT virtio_ccw_remove() and virtio_ccw_offline() are serialized
against each other under the device lock. And also against
virtio_ccw_online() iff it was initiated via the sysfs, and not via
the auto-online mechanism.

Thus I don't think we are fine at the moment.

> 
> For dasd, I think they also need to deal with the block device
> lifetimes. For virtio-ccw, we are basically a transport that does not
> know about devices further down the chain (that are associated with the
> virtio device, whose lifetime is tied to online/offline processing.) I'd
> presume that the serialization above would be enough.
> 

I don't know about dasd that much. For the reasons stated above, I don't
think the serialization we have right now is entirely sufficient.

Regards,
Halil

Re: [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown

[Cornelia Huck]

On Tue, Sep 21 2021, Halil Pasic <pasic@linux.ibm.com> wrote:

> On Mon, 20 Sep 2021 12:07:23 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
>
>> On Mon, Sep 20 2021, Vineeth Vijayan <vneethv@linux.ibm.com> wrote:
>> 
>> > On Mon, 2021-09-20 at 00:39 +0200, Halil Pasic wrote:  
>> >> On Fri, 17 Sep 2021 10:40:20 +0200
>> >> Cornelia Huck <cohuck@redhat.com> wrote:
>> >>   
>> > ...snip...  
>> >> > > 
>> >> > > 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.    
>> >> > 
>> >> > Not 100% foolproof, but "don't online a device that is in the
>> >> > progress
>> >> > of going away" seems pretty basic to me.
>> >> >   
>> >> 
>> >> I hope Vineeth will chime in on this.  
>> > Considering the online/offline processing, 
>> > The ccw_device_set_offline function or the online/offline is handled
>> > inside device_lock. Also, the online_store function takes care of
>> > avoiding multiple online/offline processing. 
>> >
>> > Now, when we consider the unconditional remove of the device,
>> > I am not familiar with the virtio_ccw driver. My assumptions are based
>> > on how CIO/dasd drivers works. If i understand correctly, the dasd
>> > driver sets different flags to make sure that a device_open is getting
>> > prevented while the the device is in progress of offline-ing.   
>> 
>> Hm, if we are invoking the online/offline callbacks under the device
>> lock already, 
>
> I believe we have a misunderstanding here. I believe that Vineeth is
> trying to tell us, that online_store_handle_offline() and
> online_store_handle_offline() are called under the a device lock of
> the ccw device. Right, Vineeth?
>
> Conny, I believe, by online/offline callbacks, you mean
> virtio_ccw_online() and virtio_ccw_offline(), right?

Whatever the common I/O layer invokes.

>
> But the thing is that virtio_ccw_online() may get called (and is
> typically called, AFAICT) with no locks held via:
> virtio_ccw_probe() --> async_schedule(virtio_ccw_auto_online, cdev)
> -*-> virtio_ccw_auto_online(cdev) --> ccw_device_set_online(cdev) -->
> virtio_ccw_online()

That's the common I/O layer in there again?

>
> Furthermore after a closer look, I believe because we don't take
> a reference to the cdev in probe, we may get virtio_ccw_auto_online()
> called with an invalid pointer (the pointer is guaranteed to be valid
> in probe, but because of async we have no guarantee that it will be
> called in the context of probe).
>
> Shouldn't we take a reference to the cdev in probe? BTW what is the
> reason for the async?

I don't know.

>
>
>> how would that affect the remove callback?
>
> I believe dev->bus->remove(dev) is called by 
> bus_remove_device() with the device lock held. I.e. I believe that means
> that virtio_ccw_remove() is called with the ccw devices device lock
> held. Vineeth can you confirm that?
>
>
> The thing is, both virtio_ccw_remove() and virtio_ccw_offline() are
> very similar, with the notable exception that offline assumes we are
> online() at the moment, while remove() does the same only if it
> decides based on vcdev && cdev->online that we are online.
>
>
>> Shouldn't they
>> be serialized under the device lock already? I think we are fine.
>
> AFAICT virtio_ccw_remove() and virtio_ccw_offline() are serialized
> against each other under the device lock. And also against
> virtio_ccw_online() iff it was initiated via the sysfs, and not via
> the auto-online mechanism.
>
> Thus I don't think we are fine at the moment.

I don't understand this, sorry.

>
>> 
>> For dasd, I think they also need to deal with the block device
>> lifetimes. For virtio-ccw, we are basically a transport that does not
>> know about devices further down the chain (that are associated with the
>> virtio device, whose lifetime is tied to online/offline processing.) I'd
>> presume that the serialization above would be enough.
>> 
>
> I don't know about dasd that much. For the reasons stated above, I don't
> think the serialization we have right now is entirely sufficient.

I'm not sure it makes sense to discuss this further right now, I feel I
currently can't really provide any meaningful contribution.

Re: [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown

[Cornelia Huck]

On Tue, Sep 21 2021, Halil Pasic <pasic@linux.ibm.com> wrote:

> On Mon, 20 Sep 2021 12:07:23 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
>
>> On Mon, Sep 20 2021, Vineeth Vijayan <vneethv@linux.ibm.com> wrote:
>> 
>> > On Mon, 2021-09-20 at 00:39 +0200, Halil Pasic wrote:  
>> >> On Fri, 17 Sep 2021 10:40:20 +0200
>> >> Cornelia Huck <cohuck@redhat.com> wrote:
>> >>   
>> > ...snip...  
>> >> > > 
>> >> > > 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.    
>> >> > 
>> >> > Not 100% foolproof, but "don't online a device that is in the
>> >> > progress
>> >> > of going away" seems pretty basic to me.
>> >> >   
>> >> 
>> >> I hope Vineeth will chime in on this.  
>> > Considering the online/offline processing, 
>> > The ccw_device_set_offline function or the online/offline is handled
>> > inside device_lock. Also, the online_store function takes care of
>> > avoiding multiple online/offline processing. 
>> >
>> > Now, when we consider the unconditional remove of the device,
>> > I am not familiar with the virtio_ccw driver. My assumptions are based
>> > on how CIO/dasd drivers works. If i understand correctly, the dasd
>> > driver sets different flags to make sure that a device_open is getting
>> > prevented while the the device is in progress of offline-ing.   
>> 
>> Hm, if we are invoking the online/offline callbacks under the device
>> lock already, 
>
> I believe we have a misunderstanding here. I believe that Vineeth is
> trying to tell us, that online_store_handle_offline() and
> online_store_handle_offline() are called under the a device lock of
> the ccw device. Right, Vineeth?
>
> Conny, I believe, by online/offline callbacks, you mean
> virtio_ccw_online() and virtio_ccw_offline(), right?

Whatever the common I/O layer invokes.

>
> But the thing is that virtio_ccw_online() may get called (and is
> typically called, AFAICT) with no locks held via:
> virtio_ccw_probe() --> async_schedule(virtio_ccw_auto_online, cdev)
> -*-> virtio_ccw_auto_online(cdev) --> ccw_device_set_online(cdev) -->
> virtio_ccw_online()

That's the common I/O layer in there again?

>
> Furthermore after a closer look, I believe because we don't take
> a reference to the cdev in probe, we may get virtio_ccw_auto_online()
> called with an invalid pointer (the pointer is guaranteed to be valid
> in probe, but because of async we have no guarantee that it will be
> called in the context of probe).
>
> Shouldn't we take a reference to the cdev in probe? BTW what is the
> reason for the async?

I don't know.

>
>
>> how would that affect the remove callback?
>
> I believe dev->bus->remove(dev) is called by 
> bus_remove_device() with the device lock held. I.e. I believe that means
> that virtio_ccw_remove() is called with the ccw devices device lock
> held. Vineeth can you confirm that?
>
>
> The thing is, both virtio_ccw_remove() and virtio_ccw_offline() are
> very similar, with the notable exception that offline assumes we are
> online() at the moment, while remove() does the same only if it
> decides based on vcdev && cdev->online that we are online.
>
>
>> Shouldn't they
>> be serialized under the device lock already? I think we are fine.
>
> AFAICT virtio_ccw_remove() and virtio_ccw_offline() are serialized
> against each other under the device lock. And also against
> virtio_ccw_online() iff it was initiated via the sysfs, and not via
> the auto-online mechanism.
>
> Thus I don't think we are fine at the moment.

I don't understand this, sorry.

>
>> 
>> For dasd, I think they also need to deal with the block device
>> lifetimes. For virtio-ccw, we are basically a transport that does not
>> know about devices further down the chain (that are associated with the
>> virtio device, whose lifetime is tied to online/offline processing.) I'd
>> presume that the serialization above would be enough.
>> 
>
> I don't know about dasd that much. For the reasons stated above, I don't
> think the serialization we have right now is entirely sufficient.

I'm not sure it makes sense to discuss this further right now, I feel I
currently can't really provide any meaningful contribution.

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown

[Vineeth Vijayan]

On Tue, 2021-09-21 at 05:25 +0200, Halil Pasic wrote:
> On Mon, 20 Sep 2021 12:07:23 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Mon, Sep 20 2021, Vineeth Vijayan <vneethv@linux.ibm.com> wrote:
> > 
> > > On Mon, 2021-09-20 at 00:39 +0200, Halil Pasic wrote:  
> > > > On Fri, 17 Sep 2021 10:40:20 +0200
> > > > Cornelia Huck <cohuck@redhat.com> wrote:
> > > >   
> > > ...snip...  
> > > > > > 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.    
> > > > > 
> > > > > Not 100% foolproof, but "don't online a device that is in the
> > > > > progress
> > > > > of going away" seems pretty basic to me.
> > > > >   
> > > > 
> > > > I hope Vineeth will chime in on this.  
> > > Considering the online/offline processing, 
> > > The ccw_device_set_offline function or the online/offline is
> > > handled
> > > inside device_lock. Also, the online_store function takes care of
> > > avoiding multiple online/offline processing. 
> > > 
> > > Now, when we consider the unconditional remove of the device,
> > > I am not familiar with the virtio_ccw driver. My assumptions are
> > > based
> > > on how CIO/dasd drivers works. If i understand correctly, the
> > > dasd
> > > driver sets different flags to make sure that a device_open is
> > > getting
> > > prevented while the the device is in progress of offline-ing.   
> > 
> > Hm, if we are invoking the online/offline callbacks under the
> > device
> > lock already, 
> 
> I believe we have a misunderstanding here. I believe that Vineeth is
> trying to tell us, that online_store_handle_offline() and
> online_store_handle_offline() are called under the a device lock of
> the ccw device. Right, Vineeth?
Yes. I wanted to bring-out both the scenario.The set_offline/_online()
calls and the unconditional-remove call. 
For the set_online The virtio_ccw_online() also invoked with ccwlock
held. (ref: ccw_device_set_online)
> 
> Conny, I believe, by online/offline callbacks, you mean
> virtio_ccw_online() and virtio_ccw_offline(), right?
> 
> But the thing is that virtio_ccw_online() may get called (and is
> typically called, AFAICT) with no locks held via:
> virtio_ccw_probe() --> async_schedule(virtio_ccw_auto_online, cdev)
> -*-> virtio_ccw_auto_online(cdev) --> ccw_device_set_online(cdev) -->
> virtio_ccw_online()
> 
> Furthermore after a closer look, I believe because we don't take
> a reference to the cdev in probe, we may get virtio_ccw_auto_online()
> called with an invalid pointer (the pointer is guaranteed to be valid
> in probe, but because of async we have no guarantee that it will be
> called in the context of probe).
> 
> Shouldn't we take a reference to the cdev in probe?
We just had a quick look at the virtio_ccw_probe() function.
Did you mean to have a get_device() during the probe() and put_device()
just after the virtio_ccw_auto_online() ?

> reason for the async?
> 
> 
> > how would that affect the remove callback?
> 
> I believe dev->bus->remove(dev) is called by 
> bus_remove_device() with the device lock held. I.e. I believe that
> means
> that virtio_ccw_remove() is called with the ccw devices device lock
> held. Vineeth can you confirm that?
This is what my understanding too.
When we disconnect a working/online device, the CIO layer gets a CRW
which indicates this disconnection. Then the subchannel driver endup
un-registering the ccw-device. This ccw_device_unregister() then
invokes device_del(), which invokes the bus->driver->remove calls which
is called with @dev-lock held.
> 
> 
> The thing is, both virtio_ccw_remove() and virtio_ccw_offline() are
> very similar, with the notable exception that offline assumes we are
> online() at the moment, while remove() does the same only if it
> decides based on vcdev && cdev->online that we are online.
> 
> 
> > Shouldn't they
> > be serialized under the device lock already? I think we are fine.
> 
> AFAICT virtio_ccw_remove() and virtio_ccw_offline() are serialized
> against each other under the device lock. And also against
> virtio_ccw_online() iff it was initiated via the sysfs, and not via
> the auto-online mechanism.
> 
> Thus I don't think we are fine at the moment.
> 
> > For dasd, I think they also need to deal with the block device
> > lifetimes. For virtio-ccw, we are basically a transport that does
> > not
> > know about devices further down the chain (that are associated with
> > the
> > virtio device, whose lifetime is tied to online/offline
> > processing.) I'd
> > presume that the serialization above would be enough.
> > 
> 
> I don't know about dasd that much. For the reasons stated above, I
> don't
> think the serialization we have right now is entirely sufficient.
> 
> Regards,
> Halil

Re: [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown

[Halil Pasic]

On Tue, 21 Sep 2021 15:31:03 +0200
Vineeth Vijayan <vneethv@linux.ibm.com> wrote:

> On Tue, 2021-09-21 at 05:25 +0200, Halil Pasic wrote:
> > On Mon, 20 Sep 2021 12:07:23 +0200
> > Cornelia Huck <cohuck@redhat.com> wrote:
> >   
> > > On Mon, Sep 20 2021, Vineeth Vijayan <vneethv@linux.ibm.com> wrote:
> > >   
> > > > On Mon, 2021-09-20 at 00:39 +0200, Halil Pasic wrote:    
> > > > > On Fri, 17 Sep 2021 10:40:20 +0200
> > > > > Cornelia Huck <cohuck@redhat.com> wrote:
> > > > >     
> > > > ...snip...    
> > > > > > > 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.      
> > > > > > 
> > > > > > Not 100% foolproof, but "don't online a device that is in the
> > > > > > progress
> > > > > > of going away" seems pretty basic to me.
> > > > > >     
> > > > > 
> > > > > I hope Vineeth will chime in on this.    
> > > > Considering the online/offline processing, 
> > > > The ccw_device_set_offline function or the online/offline is
> > > > handled
> > > > inside device_lock. Also, the online_store function takes care of
> > > > avoiding multiple online/offline processing. 
> > > > 
> > > > Now, when we consider the unconditional remove of the device,
> > > > I am not familiar with the virtio_ccw driver. My assumptions are
> > > > based
> > > > on how CIO/dasd drivers works. If i understand correctly, the
> > > > dasd
> > > > driver sets different flags to make sure that a device_open is
> > > > getting
> > > > prevented while the the device is in progress of offline-ing.     
> > > 
> > > Hm, if we are invoking the online/offline callbacks under the
> > > device
> > > lock already,   
> > 
> > I believe we have a misunderstanding here. I believe that Vineeth is
> > trying to tell us, that online_store_handle_offline() and
> > online_store_handle_offline() are called under the a device lock of
> > the ccw device. Right, Vineeth?  
> Yes. I wanted to bring-out both the scenario.The set_offline/_online()
> calls and the unconditional-remove call.

I don't understand the paragraph above. I can't map the terms
set_offline/_online() and unconditional-remove call to chunks of code.
:( 

> For the set_online The virtio_ccw_online() also invoked with ccwlock
> held. (ref: ccw_device_set_online)

I don't think virtio_ccw_online() is invoked with the ccwlock held. I
think we call virtio_ccw_online() in this line:
https://elixir.bootlin.com/linux/v5.15-rc2/source/drivers/s390/cio/device.c#L394
and we have released the cdev->ccwlock literally 2 lines above.


> > 
> > Conny, I believe, by online/offline callbacks, you mean
> > virtio_ccw_online() and virtio_ccw_offline(), right?
> > 
> > But the thing is that virtio_ccw_online() may get called (and is
> > typically called, AFAICT) with no locks held via:
> > virtio_ccw_probe() --> async_schedule(virtio_ccw_auto_online, cdev)
> > -*-> virtio_ccw_auto_online(cdev) --> ccw_device_set_online(cdev) -->
> > virtio_ccw_online()
> > 
> > Furthermore after a closer look, I believe because we don't take
> > a reference to the cdev in probe, we may get virtio_ccw_auto_online()
> > called with an invalid pointer (the pointer is guaranteed to be valid
> > in probe, but because of async we have no guarantee that it will be
> > called in the context of probe).
> > 
> > Shouldn't we take a reference to the cdev in probe?  
> We just had a quick look at the virtio_ccw_probe() function.
> Did you mean to have a get_device() during the probe() and put_device()
> just after the virtio_ccw_auto_online() ?

Yes, that would ensure that cdev pointer is still valid when
virtio_ccw_auto_online() is executed, and that things are cleaned up
properly, I guess. But I'm not 100% sure about all the interactions.
AFAIR ccw_device_set_online(cdev) would bail out if !drv. But then
we have the case where we already assigned it to a new driver (e.g.
vfio for dasd).

BTW I believe if we have a problem here, the dasd driver has the same
problem as well. The code looks very, very similar.

And shouldn't this auto-online be common CIO functionality? What is the
reason the char devices don't seem to have it?

Regards,
Halil

Re: [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown

[Halil Pasic]

On Tue, 21 Sep 2021 15:31:03 +0200
Vineeth Vijayan <vneethv@linux.ibm.com> wrote:

> On Tue, 2021-09-21 at 05:25 +0200, Halil Pasic wrote:
> > On Mon, 20 Sep 2021 12:07:23 +0200
> > Cornelia Huck <cohuck@redhat.com> wrote:
> >   
> > > On Mon, Sep 20 2021, Vineeth Vijayan <vneethv@linux.ibm.com> wrote:
> > >   
> > > > On Mon, 2021-09-20 at 00:39 +0200, Halil Pasic wrote:    
> > > > > On Fri, 17 Sep 2021 10:40:20 +0200
> > > > > Cornelia Huck <cohuck@redhat.com> wrote:
> > > > >     
> > > > ...snip...    
> > > > > > > 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.      
> > > > > > 
> > > > > > Not 100% foolproof, but "don't online a device that is in the
> > > > > > progress
> > > > > > of going away" seems pretty basic to me.
> > > > > >     
> > > > > 
> > > > > I hope Vineeth will chime in on this.    
> > > > Considering the online/offline processing, 
> > > > The ccw_device_set_offline function or the online/offline is
> > > > handled
> > > > inside device_lock. Also, the online_store function takes care of
> > > > avoiding multiple online/offline processing. 
> > > > 
> > > > Now, when we consider the unconditional remove of the device,
> > > > I am not familiar with the virtio_ccw driver. My assumptions are
> > > > based
> > > > on how CIO/dasd drivers works. If i understand correctly, the
> > > > dasd
> > > > driver sets different flags to make sure that a device_open is
> > > > getting
> > > > prevented while the the device is in progress of offline-ing.     
> > > 
> > > Hm, if we are invoking the online/offline callbacks under the
> > > device
> > > lock already,   
> > 
> > I believe we have a misunderstanding here. I believe that Vineeth is
> > trying to tell us, that online_store_handle_offline() and
> > online_store_handle_offline() are called under the a device lock of
> > the ccw device. Right, Vineeth?  
> Yes. I wanted to bring-out both the scenario.The set_offline/_online()
> calls and the unconditional-remove call.

I don't understand the paragraph above. I can't map the terms
set_offline/_online() and unconditional-remove call to chunks of code.
:( 

> For the set_online The virtio_ccw_online() also invoked with ccwlock
> held. (ref: ccw_device_set_online)

I don't think virtio_ccw_online() is invoked with the ccwlock held. I
think we call virtio_ccw_online() in this line:
https://elixir.bootlin.com/linux/v5.15-rc2/source/drivers/s390/cio/device.c#L394
and we have released the cdev->ccwlock literally 2 lines above.


> > 
> > Conny, I believe, by online/offline callbacks, you mean
> > virtio_ccw_online() and virtio_ccw_offline(), right?
> > 
> > But the thing is that virtio_ccw_online() may get called (and is
> > typically called, AFAICT) with no locks held via:
> > virtio_ccw_probe() --> async_schedule(virtio_ccw_auto_online, cdev)
> > -*-> virtio_ccw_auto_online(cdev) --> ccw_device_set_online(cdev) -->
> > virtio_ccw_online()
> > 
> > Furthermore after a closer look, I believe because we don't take
> > a reference to the cdev in probe, we may get virtio_ccw_auto_online()
> > called with an invalid pointer (the pointer is guaranteed to be valid
> > in probe, but because of async we have no guarantee that it will be
> > called in the context of probe).
> > 
> > Shouldn't we take a reference to the cdev in probe?  
> We just had a quick look at the virtio_ccw_probe() function.
> Did you mean to have a get_device() during the probe() and put_device()
> just after the virtio_ccw_auto_online() ?

Yes, that would ensure that cdev pointer is still valid when
virtio_ccw_auto_online() is executed, and that things are cleaned up
properly, I guess. But I'm not 100% sure about all the interactions.
AFAIR ccw_device_set_online(cdev) would bail out if !drv. But then
we have the case where we already assigned it to a new driver (e.g.
vfio for dasd).

BTW I believe if we have a problem here, the dasd driver has the same
problem as well. The code looks very, very similar.

And shouldn't this auto-online be common CIO functionality? What is the
reason the char devices don't seem to have it?

Regards,
Halil
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown

[Vineeth Vijayan]

On Tue, 2021-09-21 at 18:52 +0200, Halil Pasic wrote:
> > > > lock already,   
> > > 
> > > I believe we have a misunderstanding here. I believe that Vineeth
> > > is
> > > trying to tell us, that online_store_handle_offline() and
> > > online_store_handle_offline() are called under the a device lock
> > > of
> > > the ccw device. Right, Vineeth?  
> > Yes. I wanted to bring-out both the scenario.The
> > set_offline/_online()
> > calls and the unconditional-remove call.
> 
> I don't understand the paragraph above. I can't map the terms
> set_offline/_online() and unconditional-remove call to chunks of
> code.
> :( 
online_store() function can be used to set_online/set_offline manually
from the sysfs entry.
And an unconditional-remove call, for CIO, starts with a CRW which
indicates there is a subchannel_event which needs to be taken care.
This sch_event() (in device.c) then try to find the reason for this CRW
and act accordingly. This would lead to device_del and end up calling
the remove function of the driver.

> > For the set_online The virtio_ccw_online() also invoked with
> > ccwlock
> > held. (ref: ccw_device_set_online)
> 
> I don't think virtio_ccw_online() is invoked with the ccwlock held. I
> think we call virtio_ccw_online() in this line:
> https://elixir.bootlin.com/linux/v5.15-rc2/source/drivers/s390/cio/device.c#L394
> and we have released the cdev->ccwlock literally 2 lines above.
My bad. I overlooked it! 
> 
> 
> > > Conny, I believe, by online/offline callbacks, you mean
> > > virtio_ccw_online() and virtio_ccw_offline(), right?
> > > 
> > > But the thing is that virtio_ccw_online() may get called (and is
> > > typically called, AFAICT) with no locks held via:
> > > virtio_ccw_probe() --> async_schedule(virtio_ccw_auto_online,
> > > cdev)
> > > -*-> virtio_ccw_auto_online(cdev) --> ccw_device_set_online(cdev)
> > > -->
> > > virtio_ccw_online()
> > > 
> > > Furthermore after a closer look, I believe because we don't take
> > > a reference to the cdev in probe, we may get
> > > virtio_ccw_auto_online()
> > > called with an invalid pointer (the pointer is guaranteed to be
> > > valid
> > > in probe, but because of async we have no guarantee that it will
> > > be
> > > called in the context of probe).
> > > 
> > > Shouldn't we take a reference to the cdev in probe?  
> > We just had a quick look at the virtio_ccw_probe() function.
> > Did you mean to have a get_device() during the probe() and
> > put_device()
> > just after the virtio_ccw_auto_online() ?
> 
> Yes, that would ensure that cdev pointer is still valid when
> virtio_ccw_auto_online() is executed, and that things are cleaned up
> properly, I guess. But I'm not 100% sure about all the interactions.
> AFAIR ccw_device_set_online(cdev) would bail out if !drv. But then
> we have the case where we already assigned it to a new driver (e.g.
> vfio for dasd).
> 
> BTW I believe if we have a problem here, the dasd driver has the same
> problem as well. The code looks very, very similar.
You are right about that. I am trying to recreate that issue with DASD
now. And working on the patch as well.
> 
> And shouldn't this auto-online be common CIO functionality? What is
> the
> reason the char devices don't seem to have it?
I am not sure about that. I dont understand why it should be a CIO
functionality. 
> 
> Regards,
> Halil