Skip to contentSkip to conversation

x86: bau_msg_head points to a_queue_first in uv_table_bases_finish()

PATCH In review Roel Kluin 4 participants 4 messages Jan 15, 2010 – Jan 18, 2010 x86

lore
Share & tools
lore

Roel Kluin

author
#0 lore raw

PATCH 1/1 x86: bau_msg_head points to a_queue_first in uv_table_bases_finish()

bau_msg_head should point to bau_msg_head

Signed-off-by Roel Kluin <[email protected]>

---
Is this correct?

arch/x86/kernel/tlb_uv.c +1 -1
index 364d015..3d66331 100644
@@ -691,7 +691,7 @@ uv_table_bases_finish(int blade,
 			continue;
 
 		bcp = (struct bau_control *)&per_cpu(bau_control, cpu);
-		bcp->bau_msg_head	= bau_tablesp->va_queue_first;
+		bcp->bau_msg_head	= bau_tablesp->bau_msg_head;
 		bcp->va_queue_first	= bau_tablesp->va_queue_first;
 		bcp->va_queue_last	= bau_tablesp->va_queue_last;
 		bcp->msg_statuses	= bau_tablesp->msg_statuses;
#1 lore raw
Quoted 11 lines · Roel Kluin

bau_msg_head should point to bau_msg_head

Signed-off-by Roel Kluin <[email protected]>

---
Is this correct?

arch/x86/kernel/tlb_uv.c +0 -0
index 364d015..3d66331 100644

arch/x86/kernel/tlb_uv.c uv_table_bases_finish(int blade, · line 697

Quoted 8 lines
arch/x86/kernel/tlb_uv.c +1 -1
@@ -691,7 +691,7 @@ uv_table_bases_finish(int blade,
 			continue;
 
 		bcp = (struct bau_control *)&per_cpu(bau_control, cpu);
-		bcp->bau_msg_head	= bau_tablesp->va_queue_first;
+		bcp->bau_msg_head	= bau_tablesp->bau_msg_head;
 		bcp->va_queue_first	= bau_tablesp->va_queue_first;
 		bcp->va_queue_last	= bau_tablesp->va_queue_last;
 		bcp->msg_statuses	= bau_tablesp->msg_statuses;

Seems like you either caught a real bug - or that there's a somewhat quirky initialization sequence here which should be commented upon in the source.

Cliff, Robin, what's your take on Roel's patch?

Thanks,

Ingo

#2 lore raw
Quoted 26 lines · Ingo Molnar
Quoted 11 lines · Roel Kluin

bau_msg_head should point to bau_msg_head

Signed-off-by Roel Kluin <[email protected]>

---
Is this correct?

arch/x86/kernel/tlb_uv.c +0 -0
index 364d015..3d66331 100644

arch/x86/kernel/tlb_uv.c uv_table_bases_finish(int blade, · line 697

Quoted 8 lines
arch/x86/kernel/tlb_uv.c +1 -1
@@ -691,7 +691,7 @@ uv_table_bases_finish(int blade,
 			continue;
 
 		bcp = (struct bau_control *)&per_cpu(bau_control, cpu);
-		bcp->bau_msg_head	= bau_tablesp->va_queue_first;
+		bcp->bau_msg_head	= bau_tablesp->bau_msg_head;
 		bcp->va_queue_first	= bau_tablesp->va_queue_first;
 		bcp->va_queue_last	= bau_tablesp->va_queue_last;
 		bcp->msg_statuses	= bau_tablesp->msg_statuses;

Seems like you either caught a real bug - or that there's a somewhat quirky initialization sequence here which should be commented upon in the source.

Quoted 1 line

Cliff, Robin, what's your take on Roel's patch?

I think I will work with Cliff on rewriting uv_payload_queue_init().

Currently, that does not set up bau_msg_head so the code above is functional. In my opinion, it should set up bau_msg_head. I am sure I can be convinced otherwise.

Some of the other cleanups:

1) Use the standard alignment macros instead of hand coding our own.
2) Replace the kmalloc with a kzalloc and remove the memset which currently is zeroing the unaligned allocation instead of the aligned allocation so it would miss the last (up to 31) bytes of the last payload.

Thanks,
Robin

#3 lore raw
In reply to Ingo Molnar

Hi Ingo,

Quoted 26 lines · Ingo Molnar
Quoted 11 lines · Roel Kluin

bau_msg_head should point to bau_msg_head

Signed-off-by Roel Kluin <[email protected]>

---
Is this correct?

arch/x86/kernel/tlb_uv.c +0 -0
index 364d015..3d66331 100644

arch/x86/kernel/tlb_uv.c uv_table_bases_finish(int blade, · line 697

Quoted 8 lines
arch/x86/kernel/tlb_uv.c +1 -1
@@ -691,7 +691,7 @@ uv_table_bases_finish(int blade,
 			continue;
 
 		bcp = (struct bau_control *)&per_cpu(bau_control, cpu);
-		bcp->bau_msg_head	= bau_tablesp->va_queue_first;
+		bcp->bau_msg_head	= bau_tablesp->bau_msg_head;
 		bcp->va_queue_first	= bau_tablesp->va_queue_first;
 		bcp->va_queue_last	= bau_tablesp->va_queue_last;
 		bcp->msg_statuses	= bau_tablesp->msg_statuses;

Seems like you either caught a real bug - or that there's a somewhat quirky initialization sequence here which should be commented upon in the source.

Quoted 5 lines

Cliff, Robin, what's your take on Roel's patch?

Thanks,

Ingo

The bau_tablesp->bau_msg_head doesn't seem to be initialized, so do not change the code this way.

bcp->bau_msg_head is correctly initialized to the head of the queue as it stands.

However I do agree that it is confusing, as there are redundant control structures, and needs improvement. I intend to submit a new version of tlb_uv.c soon.

-Cliff

Signature · 4 lines
Cliff Wickman
SGI
[email protected]
(651) 683-3824