On 5/4/21 12:04 PM, Jason Gunthorpe wrote:
On Tue, May 04, 2021 at 09:58:45AM -0400, Tony Krowiak wrote:
On 5/3/21 4:33 PM, Jason Gunthorpe wrote:
On Mon, May 03, 2021 at 04:14:43PM -0400, Tony Krowiak wrote:
This case will occur whenever a user removes the mdev
by echoing a '1' into the mdev's sysfs 'remove' attribute
file. I'm not sure it can be considered graceful to take away
all of the crypto devices from a guest while it is running,
but there is a way to process the remove callback without
leaving things in a "weird, half-dead state".
It is acceptable to just sleep here until whatever user controlled
condition is resolved.
Jason
I suppose we could do that, but the user that tried to remove
the mdev via its sysfs 'remove' attribute will be left sitting
there wondering why the operation didn't complete. That
could result in leaving the user hanging in perpetuity.
Yes.
If the driver can't implement a disconnection then that is
unavoidable. What it does today by leaking memory under user control
is not acceptable.
Based upon my observations of the behavior during a removal
of the mdev, memory is not leaked. If the fd for the mdev is
open when an attempt is made to remove it, the operation
will hang until the mdev fd is closed which happens when
either the guest is shut down or the mdev device is detached from
the guest. When the fd is closed, the mdev release callback is
invoked which nullifies the KVM pointer, so when the remove callback
is subsequently invoked, the mdev resources will be cleaned up.
Of course, I imagine there are other possibilities
for how an mdev can be removed, but in the normal course of
events, memory will not be leaked.
IMHO, the callback should continue to return an int and
the caller should display the error if a non-zero rc is
returned.
Nope, there is a reason removal is not allowed to fail.. sysfs remove
isn't the only reason the mdev driver could be destroyed, the
underlying physical device could be unplugged or other things.
That may be true with other devices, but the matrix device is
not a real, physical device. Its sole purpose in life is to provide
an anchor for the mdev devices used to provide AP resources
to a guest; however, I get your point.
Drivers need to implement a proper remove.
Jason
