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

[Hollis B. <ho...@us...>]

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.

-- 
Hollis Blanchard
IBM Linux Technology Center