Re: simplify the mdev interface v6

From: Eric Farman <farman@linux.ibm.com>
To: Alex Williamson <alex.williamson@redhat.com>,
	Christoph Hellwig <hch@lst.de>
Cc: Kirti Wankhede <kwankhede@nvidia.com>,
	Tony Krowiak <akrowiak@linux.ibm.com>,
	Halil Pasic <pasic@linux.ibm.com>,
	Jason Herne <jjherne@linux.ibm.com>,
	Matthew Rosato <mjrosato@linux.ibm.com>,
	Zhenyu Wang <zhenyuw@linux.intel.com>,
	Zhi Wang <zhi.a.wang@intel.com>, Jason Gunthorpe <jgg@nvidia.com>,
	kvm@vger.kernel.org, linux-s390@vger.kernel.org,
	intel-gvt-dev@lists.freedesktop.org,
	Vineeth Vijayan <vneethv@linux.ibm.com>
Subject: Re: simplify the mdev interface v6
Date: Tue, 19 Jul 2022 22:41:49 -0400	[thread overview]
Message-ID: <29248eb6e20ef5990d3189ba5468fe4d8bada61a.camel@linux.ibm.com> (raw)
In-Reply-To: <20cba66846a011e2fe8885f15def6ec837d12d0b.camel@linux.ibm.com>

On Tue, 2022-07-19 at 13:49 -0400, Eric Farman wrote:
> On Tue, 2022-07-19 at 09:26 -0600, Alex Williamson wrote:
> > On Tue, 19 Jul 2022 16:49:28 +0200
> > Christoph Hellwig <hch@lst.de> wrote:
> > 
> > > On Mon, Jul 18, 2022 at 10:01:40PM -0400, Eric Farman wrote:
> > > > I'll get the problem with struct subchannel [1] sorted out in
> > > > the
> > > > next
> > > > couple of days. This series breaks vfio-ccw in its current form
> > > > (see
> > > > reply to patch 14), but even with that addressed the placement
> > > > of
> > > > all
> > > > these other mdev structs needs to be handled differently.  
> > > 
> > > Alex, any preference if I should just fix the number instances
> > > checking
> > > with either an incremental patch or a resend, or wait for this
> > > ccw
> > > rework?
> > 
> > Since it's the last patch, let's at least just respin that patch
> > rather
> > than break and fix.  I'd like to make sure Eric is ok to shift
> > around
> > structures as a follow-up or make a proposal how this series should
> > change though. 
> 
> I'd hoped to have that proposal today, but I don't have much
> confidence
> in it yet as this series (with the fix on the last patch) is still
> crashing my system. Will get something out as soon as I'm able.

The solution I envision thus far does two things:

 - Move the struct mdev_parent and its friends out of struct
subchannel, and into struct vfio_ccw_private. This struct is allocated
just prior to the call to mdev_register_device/_parent, and released
with the mdev_unregister. It's also a device-specific struct linked
from the device-agnostic subchannel.
 - Add a kref to struct vfio_ccw_private. The mdev_parent currently has
one, which is now unnecessary since it's embedded in another struct,
but it leaves vfio_ccw_private rather racy.

I suspect the second item (or something similar) is needed anyway,
because Alex' tree + this series crashes frequently in (usually)
mdev_remove. I haven't found an explanation for how we get in this
state, but admittedly didn't spent a lot of time on them since the
proposed changes to struct subchannel are a non-starter. The other
crashes were always in something that's almost certainly a victim of
something else, like kmalloc-related stuff in net/skbuff.

With the above, the crashes out of the vfio-ccw stack disappear, and
things work a bit better. But those random kmalloc-related crashes
persist. I guess I'll pick those up tomorrow.

Eric