Re: Regression :-) Re: [GIT PULL RESEND] x86/jumpmplabel changes forv3.12-rc1

[Konrad Rzeszutek Wilk]

On Wed, Sep 11, 2013 at 10:56:33AM -0400, Steven Rostedt wrote:
> On Wed, 11 Sep 2013 10:25:45 -0400
> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> wrote:
> 
> > > It seems to imply line 53 is the originating bug, so that would be:
> > > 
> > >  47         if (type == JUMP_LABEL_ENABLE) {
> > >  48                 /*
> > >  49                  * We are enabling this jump label. If it is not a nop
> > >  50                  * then something must have gone wrong.
> > >  51                  */
> > >  52                 if (unlikely(memcmp((void *)entry->code, ideal_nop, 5) != 0))
> > >  53                         bug_at((void *)entry->code, __LINE__);
> > > 
> > > But it is a NOP isn't it? The code is
> > > 
> > > Unexpected op at trace_clock_global+0x6b/0x120 [ffffffff8113a21b] (0f 1f 44 00 00) 53
> > > 
> > > Perhaps the ideal_nop has not been set yet?
> > > 
> > 
> > And this looks to fix it for me.
> 
> I'm trying to understand how this will fix it for you. Are you sure you
> removed 'xen_nopvspin'?

Yes.
> 
> If you are calling static_key_slow_inc() before jump_label_init(), then
> it should still fail. The static_key_slow_inc() eventually calls
> arch_jump_label_transform(), which calls __jump_label_transform() with
> init == 0.

Perhaps I am misreading the code, but I believe init is set to one.
That is due to us calling:

arch_jump_label_transform (.., JUMP_LABEL_ENABLE)

which calls __jump_label_transform(.., 1)
?

Perhaps the 'init' and 'enable' parameters have different meanings?

> 
> The below code looks to me that it would still compare the contents
> with the ideal_nop, which hasn't been set yet.

In the !init case - sure. 

In the init case - just with default_nop.

> 
> -- Steve
> 
> > 
> > diff --git a/arch/x86/kernel/jump_label.c b/arch/x86/kernel/jump_label.c
> > index ee11b7d..d688348 100644
> > --- a/arch/x86/kernel/jump_label.c
> > +++ b/arch/x86/kernel/jump_label.c
> > @@ -44,13 +44,20 @@ static void __jump_label_transform(struct jump_entry *entry,
> >  	union jump_code_union code;
> >  	const unsigned char *ideal_nop = ideal_nops[NOP_ATOMIC5];
> >  
> > +	if (init) {
> > +		const unsigned char default_nop[] = { STATIC_KEY_INIT_NOP };
> > +		if (unlikely(memcmp((void *)entry->code, default_nop, 5) != 0))
> > +			bug_at((void *)entry->code, __LINE__);
> > +	}
> >  	if (type == JUMP_LABEL_ENABLE) {
> >  		/*
> >  		 * We are enabling this jump label. If it is not a nop
> >  		 * then something must have gone wrong.
> >  		 */
> > -		if (unlikely(memcmp((void *)entry->code, ideal_nop, 5) != 0))
> > -			bug_at((void *)entry->code, __LINE__);
> > +		if (!init) {
> > +			if (unlikely(memcmp((void *)entry->code, ideal_nop, 5) != 0))
> > +				bug_at((void *)entry->code, __LINE__);
> > +		}
> >  
> >  		code.jump = 0xe9;
> >  		code.offset = entry->target -
> > @@ -62,11 +69,7 @@ static void __jump_label_transform(struct jump_entry *entry,
> >  		 * If this is the first initialization call, then we
> >  		 * are converting the default nop to the ideal nop.
> >  		 */
> > -		if (init) {
> > -			const unsigned char default_nop[] = { STATIC_KEY_INIT_NOP };
> > -			if (unlikely(memcmp((void *)entry->code, default_nop, 5) != 0))
> > -				bug_at((void *)entry->code, __LINE__);
> > -		} else {
> > +		if (!init) {
> >  			code.jump = 0xe9;
> >  			code.offset = entry->target -
> >  				(entry->code + JUMP_LABEL_NOP_SIZE);
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/