Re: [PATCH]porting lockless mce from x86_64 to i386

Re: [PATCH]porting lockless mce from x86_64 to i386

[Andi Kleen]

On Tue, Apr 26, 2005 at 01:27:30PM +0800, Yu, Luming wrote:
> 
> Forward a patch :

Some comments: 

The asmlinkage on x86-64 is not really needed. You can remove
the ifdef. fastcall is fine, although it is a nop.

The u64 tsc[NR CPUS] on the stack is a stack overflow with big
NR_CPUS. I have
a patch locally here to fix it, but you could just apply it
anyways when you move the code. Fix is to use kmalloc here.

-Andi

Re: [PATCH]porting lockless mce from x86_64 to i386

[Andrew Morton]

Andi Kleen <ak@muc.de> wrote:
>
> On Tue, Apr 26, 2005 at 01:27:30PM +0800, Yu, Luming wrote:
> > 
> > Forward a patch :
> 
> Some comments: 
> 
> The asmlinkage on x86-64 is not really needed. You can remove
> the ifdef. fastcall is fine, although it is a nop.
> 
> The u64 tsc[NR CPUS] on the stack is a stack overflow with big
> NR_CPUS. I have
> a patch locally here to fix it, but you could just apply it
> anyways when you move the code. Fix is to use kmalloc here.
> 

OK, thanks.  Luming, could you please reissue the second patch, including
the above fixes as well as the two warning fixes which we discussed?

Re: [PATCH]porting lockless mce from x86_64 to i386

[Andi Kleen]

On Wed, Apr 27, 2005 at 11:38:00AM -0700, Andrew Morton wrote:
> Andi Kleen <ak@muc.de> wrote:
> >
> > On Tue, Apr 26, 2005 at 01:27:30PM +0800, Yu, Luming wrote:
> > > 
> > > Forward a patch :
> > 
> > Some comments: 
> > 
> > The asmlinkage on x86-64 is not really needed. You can remove
> > the ifdef. fastcall is fine, although it is a nop.
> > 
> > The u64 tsc[NR CPUS] on the stack is a stack overflow with big
> > NR_CPUS. I have
> > a patch locally here to fix it, but you could just apply it
> > anyways when you move the code. Fix is to use kmalloc here.
> > 
> 
> OK, thanks.  Luming, could you please reissue the second patch, including
> the above fixes as well as the two warning fixes which we discussed?

Another comment: 

If Luming would not move the mce.c file from x86-64 to i386 then
his patch would be only 1/4 as big. I dont know why he does this
anyways, it seems completely pointless.


-Andi

> 

RE: [PATCH]porting lockless mce from x86_64 to i386

[Yu, Luming]

>
>Another comment: 
>
>If Luming would not move the mce.c file from x86-64 to i386 then
>his patch would be only 1/4 as big. I dont know why he does this
>anyways, it seems completely pointless.
>
Sounds reasonable. Then, mce.c will the first one linked into i386
Makefile. :-)
Racing, what do you think?

Thanks,
Luming

RE: [PATCH]porting lockless mce from x86_64 to i386

[Guo, Racing]

>
>If Luming would not move the mce.c file from x86-64 to i386 then
>his patch would be only 1/4 as big. I dont know why he does this
>anyways, it seems completely pointless.

mce.c mce.h and mce_intel.c are moved from x86_64 to i386. so the
patch is very big. The motivation is to share mce code between
x86_64 and i386 and avoid duplicate code in x86_64 and i386.
I don't know whether I completely understand what you point.
Correct me if I am wrong.

Re: [PATCH]porting lockless mce from x86_64 to i386

[Andi Kleen]

On Mon, May 02, 2005 at 09:01:53AM +0800, Guo, Racing wrote:
> >
> >If Luming would not move the mce.c file from x86-64 to i386 then
> >his patch would be only 1/4 as big. I dont know why he does this
> >anyways, it seems completely pointless.
> 
> mce.c mce.h and mce_intel.c are moved from x86_64 to i386. so the
> patch is very big. The motivation is to share mce code between
> x86_64 and i386 and avoid duplicate code in x86_64 and i386.
> I don't know whether I completely understand what you point.
> Correct me if I am wrong.

You can share code as well by linking it from x86-64 into i386,
not only the other way round.  This is already done for earlyprintk
for example.

-Andi

RE: [PATCH]porting lockless mce from x86_64 to i386

[Pallipadi, Venkatesh]

>-----Original Message-----
>From: linux-kernel-owner@vger.kernel.org 
>[mailto:linux-kernel-owner@vger.kernel.org] On Behalf Of Guo, Racing
>Sent: Sunday, May 01, 2005 6:02 PM
>To: Andi Kleen; Andrew Morton
>Cc: Yu, Luming; linux-kernel@vger.kernel.org
>Subject: RE: [PATCH]porting lockless mce from x86_64 to i386
>
>>
>>If Luming would not move the mce.c file from x86-64 to i386 then
>>his patch would be only 1/4 as big. I dont know why he does this
>>anyways, it seems completely pointless.
>
>mce.c mce.h and mce_intel.c are moved from x86_64 to i386. so the
>patch is very big. The motivation is to share mce code between
>x86_64 and i386 and avoid duplicate code in x86_64 and i386.
>I don't know whether I completely understand what you point.
>Correct me if I am wrong.

I think what Andi meant was that instead of copying code from x86-64 
to i386 and making x86-64 link to this i386 copy, you can leave the 
code in x86-64 and link it from i386 part of the tree. 

Doing it either way should be OK with this mce code. But I feel, 
despite of the patch size, it is better to keep all the shared 
code in i386 tree and link it from x86-64. Otherwise, it may become 
kind of messy in future, with various links between i386 and x86-64.
Andi/Andrew: What do you suggest here?

Thanks,
Venki

Re: [PATCH]porting lockless mce from x86_64 to i386

[Andi Kleen]

On Mon, May 02, 2005 at 09:15:07AM -0700, Pallipadi, Venkatesh wrote:
> >-----Original Message-----
> >From: linux-kernel-owner@vger.kernel.org 
> >[mailto:linux-kernel-owner@vger.kernel.org] On Behalf Of Guo, Racing
> >Sent: Sunday, May 01, 2005 6:02 PM
> >To: Andi Kleen; Andrew Morton
> >Cc: Yu, Luming; linux-kernel@vger.kernel.org
> >Subject: RE: [PATCH]porting lockless mce from x86_64 to i386
> >
> >>
> >>If Luming would not move the mce.c file from x86-64 to i386 then
> >>his patch would be only 1/4 as big. I dont know why he does this
> >>anyways, it seems completely pointless.
> >
> >mce.c mce.h and mce_intel.c are moved from x86_64 to i386. so the
> >patch is very big. The motivation is to share mce code between
> >x86_64 and i386 and avoid duplicate code in x86_64 and i386.
> >I don't know whether I completely understand what you point.
> >Correct me if I am wrong.
> 
> I think what Andi meant was that instead of copying code from x86-64 
> to i386 and making x86-64 link to this i386 copy, you can leave the 
> code in x86-64 and link it from i386 part of the tree. 

Yep.

> 
> Doing it either way should be OK with this mce code. But I feel, 
> despite of the patch size, it is better to keep all the shared 
> code in i386 tree and link it from x86-64. Otherwise, it may become 
> kind of messy in future, with various links between i386 and x86-64.

i386 already uses code from x86-64 (earlyprintk.c) - it is nothing 
new.


-Andi

Re: [PATCH]porting lockless mce from x86_64 to i386

[Andrew Morton]

Andi Kleen <ak@muc.de> wrote:
>
>  > 
>  > Doing it either way should be OK with this mce code. But I feel, 
>  > despite of the patch size, it is better to keep all the shared 
>  > code in i386 tree and link it from x86-64. Otherwise, it may become 
>  > kind of messy in future, with various links between i386 and x86-64.
> 
>  i386 already uses code from x86-64 (earlyprintk.c) - it is nothing 
>  new.

I must say I don't like the bidirectional sharing either.

But I guess it'll be simple enough to fix up if it causes any real problems
in the future.

Re: [PATCH]porting lockless mce from x86_64 to i386

[Andi Kleen]

On Mon, May 02, 2005 at 11:31:25AM -0700, Andrew Morton wrote:
> Andi Kleen <ak@muc.de> wrote:
> >
> >  > 
> >  > Doing it either way should be OK with this mce code. But I feel, 
> >  > despite of the patch size, it is better to keep all the shared 
> >  > code in i386 tree and link it from x86-64. Otherwise, it may become 
> >  > kind of messy in future, with various links between i386 and x86-64.
> > 
> >  i386 already uses code from x86-64 (earlyprintk.c) - it is nothing 
> >  new.
> 
> I must say I don't like the bidirectional sharing either.

Why exactly? X86-64 is not a "slave" of i386, they are equal peers; 
free to share from each other, but none better than the other ... ,-) 

-Andi (fighting for the rights of the repressed architectures ;-)
> 
> But I guess it'll be simple enough to fix up if it causes any real problems
> in the future.

The only complaint I heard so far was from a kernel hacker who deleted
all  non i386 architectures in his kernel trees, but I was not 
very sympathetic to that one. In fact I think it is better
when people have full trees around so when they change something
globally grep finds really all instances.

-Andi


> 

Re: [PATCH]porting lockless mce from x86_64 to i386

[Andrew Morton]

Andi Kleen <ak@muc.de> wrote:
>
> On Mon, May 02, 2005 at 11:31:25AM -0700, Andrew Morton wrote:
>  > Andi Kleen <ak@muc.de> wrote:
>  > >
>  > >  > 
>  > >  > Doing it either way should be OK with this mce code. But I feel, 
>  > >  > despite of the patch size, it is better to keep all the shared 
>  > >  > code in i386 tree and link it from x86-64. Otherwise, it may become 
>  > >  > kind of messy in future, with various links between i386 and x86-64.
>  > > 
>  > >  i386 already uses code from x86-64 (earlyprintk.c) - it is nothing 
>  > >  new.
>  > 
>  > I must say I don't like the bidirectional sharing either.
> 
>  Why exactly?

One reason is that it makes it harder to locate the code.  I ctag each of
my architecture trees only with stuff from ./arch/that-architecture to reduce
duplicate hits.  So I end up with some x86 functions being unlocatable in
the x86 tree.  We end up with both x86_64 and x86 being broken in this
regard.

But that's a relatively minor point.  The major point is that it gives me
the creeps in hard-to-define ways ;)

Re: [PATCH]porting lockless mce from x86_64 to i386

[Adrian Bunk]

On Mon, May 02, 2005 at 09:11:59PM +0200, Andi Kleen wrote:
> On Mon, May 02, 2005 at 11:31:25AM -0700, Andrew Morton wrote:
> > Andi Kleen <ak@muc.de> wrote:
> > >
> > >  > 
> > >  > Doing it either way should be OK with this mce code. But I feel, 
> > >  > despite of the patch size, it is better to keep all the shared 
> > >  > code in i386 tree and link it from x86-64. Otherwise, it may become 
> > >  > kind of messy in future, with various links between i386 and x86-64.
> > > 
> > >  i386 already uses code from x86-64 (earlyprintk.c) - it is nothing 
> > >  new.
> > 
> > I must say I don't like the bidirectional sharing either.
> 
> Why exactly? X86-64 is not a "slave" of i386, they are equal peers; 
> free to share from each other, but none better than the other ... ,-) 
>...

When grep'ing whether a patch I send might break something, it's quite 
handy to see what belongs to which architecture.

Perhaps some day someone might want to put some ACPI code under 
arch/ia64 and let i386 and x86_64 use it from there...

What about some kind of arch/i386-x86_64-shared/ that contains the 
shared code?

The fact that x86_64 defines CONFIG_X86 while i386 doesn't define 
CONFIG_X86_64 unambiguously defines an ordering, and if we really need 
this sharing, there's no good reason to make the chaos bigger than it is 
already with unidirectional sharing.

> -Andi

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH]porting lockless mce from x86_64 to i386

[Bill Davidsen]

Pallipadi, Venkatesh wrote:

> I think what Andi meant was that instead of copying code from x86-64 
> to i386 and making x86-64 link to this i386 copy, you can leave the 
> code in x86-64 and link it from i386 part of the tree. 
> 
> Doing it either way should be OK with this mce code. But I feel, 
> despite of the patch size, it is better to keep all the shared 
> code in i386 tree and link it from x86-64. Otherwise, it may become 
> kind of messy in future, with various links between i386 and x86-64.
> Andi/Andrew: What do you suggest here?

Have you considered having a tree just for the shared code and links 
where appropriate? If nothing else that would make it blindingly obvious 
  that the code was shared, and avoid having someone do something 
unsharable because s/he didn't know there was a pointer to the code 
elsewhere.

I know it's slightly more complex, but also slightly safer.

-- 
    -bill davidsen (davidsen@tmr.com)
"The secret to procrastination is to put things off until the
  last possible moment - but no longer"  -me