On Saturday 17 March 2007, YAMAMOTO Takashi wrote:

> because it isn't the first time and i already know how it's hard to
> explain you what's wrong...
> and, actually, i explained what's wrong here.  ie. the iteration is
> completely bogus.

Ok, I know that is bogus. But that fixed all my tests. If you know a
better way to fix this, why don't you say that? or at least point me at
the correct location of the code to understand it.

If I use curcpu() or the struct cpu_info pointer passed to the function 
callback, sometimes it won't assing the value in ci_msr_rvalue in all CPUs,
this is why I added the second iteration and it worked.

In my understanding of the code, msr_read_ipi will be called via 
x86_broadcast_ipi(IPI_READ_MSR), looking the bit defined by
X86_IPI_READ_MSR (0x00000080) in machine/intrdefs.h.

x86_broadcast_ipi iterates over all cpu_info struct, and IT skips curcpu(),
so it passes to next one, and ci_msr_rvalue is not assigned.

Am I wrong?

> you don't need to be a MASTER of UNIVERSE, but you need to understand
> relevant code before commiting something.  don't you agree?

I agree. But you never are helping me trying to develop something. You are
always attacking me, saying "bah, you don't know how the code works, don't 
commit or do it yourself, I'm telling you it's wrong, but not any suggestion 
in how to fix it).

> you want to use ipi in the first place because you want to run these
> operations on each cpus.  it doesn't make sense unless the callback
> only operates the cpu on which it's running.

Well, ok.

> > > you did it because you thought it was necessary for some reasons,
> > > didn't you?
> >
> > Andrew told me that it's necessary to run the ipi handler in the primary
> > cpu, this is what I'm doing.
>
> i guess he told you about a cpu calling x86_broadcast_ipi,
> not a CPU_IS_PRIMARY cpu.

Perhaps, can't remember now.

> > I iter over cii, and ci points to the primary cpu; I assign
> > ci->ci_msr_rvalue the value of the msr_type.
>
> so?  do you mean a cpu on which msr_cpu_broadcast_read is running
> should be a primary cpu, in the sense of CPU_IS_PRIMARY?  if so, why?

Well, maybe CPU_IS_PRIMARY is not what I want, but don't worry,
I'll fix it without any help, is that what you want right?

> > > > > - what's the point of ci_msr_rvalue?
> > > >
> > > > Just to know what's the returned value of the MSR in cpuN.
> > >
> > > to me, no one in your patch seems to use it in a meaningful way.
> >
> > Erm, why? ci_msr_rvalue could have a different value of the MSR,
> > it's necessary to know that it has the same value.
>
> why?  i don't know.  it's your code.
> please grep ci_msr_rvalue in your patch and see how they are used.

I know it's my code, and I know how I'm using ci_msr_rvalue.

Maybe I should remove some parts of msr_cpu_broadcast_read, concretly
the part you are mentioning.

I'm working on it, don't get too excited about attacking me. And this
time I'll commit it (do you remember bufq-on-the-fly? you did not help me 
there and you committed the code before I finished it, even the log message
did not mention my name!)

So thanks for your attacks. Anyway, I think you could be more friendly in
the future with people trying to help problems in NetBSD, for FREE!
-- 

http://plog.xtrarom.org/
Juan RP's blog - NetBSD/pkgsrc news in Spanish