Re: [kvm-ppc-devel] [PATCH 2 of 2] Add PowerPC KVM guest wait handling support

[Jerone Y. <jy...@us...>]

On Tue, 2008-04-15 at 13:56 -0500, Hollis Blanchard wrote:
> On Tuesday 15 April 2008 13:29:19 Jerone Young wrote:
> > On Tue, 2008-04-15 at 13:19 -0500, Hollis Blanchard wrote:
> > > On Tuesday 15 April 2008 11:14:52 Jerone Young wrote:
> > > > Actually there appears to be a real problem with preempt notify in 44x.
> > > > I had no gotten a chance to get back with you about it. But I did some
> > > > investigation into it last week. We are following the same code paths
> > > > (common code) as x86 for preempt initalization. But I ran some tests
> > > > using preempt_disable() & preempt_enable() around some places where it
> > > > would make since (places where we disable interrupts), but just using
> > > > these functions whould cause the kernel to dump sig #11.
> > >
> > > preempt_disable() and preempt_enable() are completely different, and I've
> > > called those in the past and had no problems.
> >
> > But it's not used anywhere in our code. And places where it can go ..
> > blows up big time.
> 
> We don't explicitly use preempt_disable() now, because we use 
> local_irq_disable() instead.
> 
> However, as I said before, when we *did* use preempt_disable() explicitly, it 
> didn't even blow up a little bit.
> 
> preempt_disable() IS NOT THE SAME as preempt_notify_unregister(). Please read 
> that again, because I think you're skimming over the function names. These 
> are very different things.
> 
> > > > The issue we have using function kvm_vcpu_block() that it is identical
> > > > to the code below, BUT it calls vcpu_put which then calls
> > > > preempt_notify_unregister() if it is called it will also sig #11.
> > >
> > > If preempt_notify_unregister() dereferences a bad pointer, that shouldn't
> > > be too difficult to track down.
> > >
> > > > I'm not sure if what is going on honestly. Based on what I found it
> > > > should "just work" as we are initializing everything like x86 (we are
> > > > calling preempt_notify_init() in the same place). But for 44x any
> > > > preempt notfication calls blow up. So it appears calling anything
> > > > preempt notify related just blows up.
> > > >
> > > > This is a much bigger issue. I'm not sure that we honest want to be
> > > > stuck on this for long periods of time just to have a function call in
> > > > a place where we honestly do not absolutely need to have it at this
> > > > time. Plus I'm no expert with these scheduling frameworks. But givin
> > > > what have read around the net what we have now "should" work. It just
> > > > doesn't.
> > > >
> > > > Something can come back to later. But for now we should just roll with
> > > > the working code.
> > >
> > > No, I don't want to commit the hack. Sorry if I didn't make that clear
> > > before.
> >
> > But this isn't a hack. You just want to use the common function. The
> > issue is there are a lot place we do not use the common function.
> 
> We use common code wherever it makes sense, and it makes a lot of sense here. 
> If you can point out other areas we could or should share code, I would be 
> interested in taking a look.
> 
> > Since 
> > we really do need to get functional code up and going this is something
> > that can be postponed to be fixed. I will (and have been) take a look at
> > this to change this. But if we put a ton of emphasis on something that
> > isn't needed to get us going, then we will never move forward to solve
> > the real issues left to get this stuff usable.
> >
> > > Accessing a bad pointer doesn't seem like a "much bigger issue" to me, so
> > > if it's more complicated than that please elaborate.
> >
> > The problem is the pointer is assigned by the frame work. I'm not
> > passing in any pointer to preempt. So something more is going on.
> 
> Obviously you're not passing a pointer in, yet sig11 means the code is 
> dereferencing a bad pointer. The backtrace probably indicates exactly what 
> pointer is causing the problem, and it's 100% reproducible, so it doesn't 
> sound like anything too subtle.
> 
> > I will 
> > have to look into this more after I finish the next task. I don't
> > honestly know all that much about the preempt frame work. But I have
> > spent time trying to figure it out.
> 
> Great, I will look forward to the corrected patch.

I would suggest that you include the current patch so that other can use
the functionality and it can be changed to use the common code function
once I figure out the preempt issue. Otherwise anyone trying to use our
source is still going to suffer from guest eating 100% cpu when idle.

>