On 29.6.2012 5:23, Adam Hraska wrote:
> 1) APIC: waiting for completion
> -------------------------------
> APIC has a bit that indicates if an IPI is in progress.
> Should we not wait for the dispatch of the previous IPI
> to complete before we send another one? Note that I don't
> know if l_apic[ICRlo].delivs is CPU local (so a simple
> while(l_apic[ICRlo].delivs == DELIVS_PENDING){} would
> suffice) or global for the system (then we'd need a lock).
> IPIs are sent via l_apic_broadcast_custom_ipi in
> arch/ia32/src/smp/apic.c:
>
> int l_apic_broadcast_custom_ipi(uint8_t vector)
> {
> icr_t icr;
>
> // Wait here for icr.delivs != DELIVS_PENDING ?
>
> icr.lo = l_apic[ICRlo];
> icr.delmod = DELMOD_FIXED;
> icr.destmod = DESTMOD_LOGIC;
> icr.level = LEVEL_ASSERT;
> icr.shorthand = SHORTHAND_ALL_EXCL;
> icr.trigger_mode = TRIGMOD_LEVEL;
> icr.vector = vector;
>
> l_apic[ICRlo] = icr.lo;
>
> icr.lo = l_apic[ICRlo];
> if (icr.delivs == DELIVS_PENDING) {
> #ifdef CONFIG_DEBUG
> printf("IPI is pending.\n");
> #endif
> }
>
> return apic_poll_errors();
> }
Hm, I remember seeing "IPI is pending" messages when running HelenOS on
my laptop, which suggests this is a real issue :-) Looks like a good
idea to wait for some time. Does waiting indefinitely here look like a
good idea?
> 2) sparc64/sun4U: while-loop cond
> ---------------------------------
> arch/sparc64/src/smp/sun4u/ipi.c sends IPIs via cross_call().
> It has a suspicious while loop end condition. I don't
> know enough to decide if it is a bug or not.
>
> static void cross_call(int mid, void (* func)(void))
> {
> uint64_t status;
> bool done;
>
> /* snip */
>
> do {
> // snip
> if (!done) {
> /*
> * Prevent deadlock.
> */
> (void) interrupts_enable();
> delay(20 + (tick_read() & 0xff));
> (void) interrupts_disable();
> }
>
> } while (done); // <-- instead of "while (!done)" ?!
>
> preemption_enable();
> }
Indeed, this looks wrong. The idea should be to loop until the other CPU
acknowledges the IPI. I wonder this has gone unnoticed for so long. I
will reverse the condition and test on the Ultra 60.
> 3) sparc64/sun4v: cpu->arch.id vs cpu->id
> -----------------------------------------
> In arch/sparc64/src/smp/sun4v/ipi.c, ipi_broadcast_arch()
> seems to be mixing cpu->arch.id with cpu->id as if they
> were the same (even if they are guaranteed to match, it
> is just not obvious from the code):
>
> void ipi_broadcast_arch(int ipi)
> {
> void (* func)(void);
> /* snip */
> unsigned int i;
> unsigned idx = 0;
> for (i = 0; i < config.cpu_active; i++) {
> if (&cpus[i] == CPU) {
> continue;
> }
>
> // Was this supposed to be CPU->arch.id and
> // cpus[i].arch.id ??
> ipi_cpu_list[CPU->id][idx] = (uint16_t) cpus[i].id;
> idx++;
> }
>
> ipi_brodcast_to(func, ipi_cpu_list[CPU->arch.id], idx);
> }
Yeah, looks like it should be either cpu->id or cpu->arch.id, but not
both at the same time. We may want to consult this with Pavel.
Unfortunately, sun4v support is currently broken.
> Thanks to anyone with the proper knowledge for checking
> these inconsistencies.
Thanks for pointing them out. These things are kind of hard-to-spot if
you are not looking into the code trying to fix some particular problem.
So with every additional code review, the quality of code increases.
Again, I am quite surprised that some of theses bugs did not bite us
already.
Jakub
_______________________________________________
HelenOS-devel mailing list
[email protected]
http://lists.modry.cz/cgi-bin/listinfo/helenos-devel
