ring_bufer: fix BUF_PAGE_SIZE

PATCH In review Lai Jiangshan 3 participants 6 messages Dec 17, 2008 – Dec 22, 2008

Subsystems (2)

Tracing Tracing ring buffer

lore

Share & tools

lore

Lai Jiangshan

author

2008-12-17 09:52 UTC #0 lore raw

PATCH 1/1 ring_bufer: fix BUF_PAGE_SIZE

impact: make BUF_PAGE_SIZE changeable.

Except allocating/freeing page and the code using PAGE_MASK, all code expect buffer_page's length is BUF_PAGE_SIZE.

This patch make this behavior more concordant.

Signed-off-by Lai Jiangshan <[email protected]>

---

kernel/trace/ring_buffer.c +11 -9

index 668bbb5..0cf6caf 100644
@@ -158,6 +158,10 @@ struct buffer_page {
 	void *page;			/* Actual data page */
 };
 
+#define BUF_PAGE_ORDER 0
+#define BUF_PAGE_SIZE (PAGE_SIZE << BUF_PAGE_ORDER)
+#define BUF_PAGE_MASK (~(BUF_PAGE_SIZE - 1))
+
 /*
  * Also stolen from mm/slob.c. Thanks to Mathieu Desnoyers for pointing
  * this issue out.

@@ -165,7 +169,7 @@ struct buffer_page {
 static inline void free_buffer_page(struct buffer_page *bpage)
 {
 	if (bpage->page)
-		free_page((unsigned long)bpage->page);
+		free_pages((unsigned long)bpage->page, BUF_PAGE_ORDER);
 	kfree(bpage);
 }

@@ -179,8 +183,6 @@ static inline int test_time_stamp(u64 delta)
 	return 0;
 }
 
-#define BUF_PAGE_SIZE PAGE_SIZE
-
 /*
  * head_page == tail_page && head == tail then buffer is empty.
  */

@@ -289,7 +291,7 @@ static int rb_allocate_pages(struct ring_buffer_per_cpu *cpu_buffer,
 			goto free_pages;
 		list_add(&page->list, &pages);
 
-		addr = __get_free_page(GFP_KERNEL);
+		addr = __get_free_pages(GFP_KERNEL, BUF_PAGE_ORDER);
 		if (!addr)
 			goto free_pages;
 		page->page = (void *)addr;

@@ -333,7 +335,7 @@ rb_allocate_cpu_buffer(struct ring_buffer *buffer, int cpu)
 		goto fail_free_buffer;
 
 	cpu_buffer->reader_page = page;
-	addr = __get_free_page(GFP_KERNEL);
+	addr = __get_free_pages(GFP_KERNEL, BUF_PAGE_ORDER);
 	if (!addr)
 		goto fail_free_reader;
 	page->page = (void *)addr;

@@ -592,7 +594,7 @@ int ring_buffer_resize(struct ring_buffer *buffer, unsigned long size)
 			if (!page)
 				goto free_pages;
 			list_add(&page->list, &pages);
-			addr = __get_free_page(GFP_KERNEL);
+			addr = __get_free_pages(GFP_KERNEL, BUF_PAGE_ORDER);
 			if (!addr)
 				goto free_pages;
 			page->page = (void *)addr;

@@ -718,7 +720,7 @@ rb_event_index(struct ring_buffer_event *event)
 {
 	unsigned long addr = (unsigned long)event;
 
-	return (addr & ~PAGE_MASK) - (PAGE_SIZE - BUF_PAGE_SIZE);
+	return addr & ~BUF_PAGE_MASK;
 }
 
 static inline int

@@ -729,7 +731,7 @@ rb_is_commit(struct ring_buffer_per_cpu *cpu_buffer,
 	unsigned long index;
 
 	index = rb_event_index(event);
-	addr &= PAGE_MASK;
+	addr &= BUF_PAGE_MASK;
 
 	return cpu_buffer->commit_page->page == (void *)addr &&
 		rb_commit_index(cpu_buffer) == index;

@@ -743,7 +745,7 @@ rb_set_commit_event(struct ring_buffer_per_cpu *cpu_buffer,
 	unsigned long index;
 
 	index = rb_event_index(event);
-	addr &= PAGE_MASK;
+	addr &= BUF_PAGE_MASK;
 
 	while (cpu_buffer->commit_page->page != (void *)addr) {
 		RB_WARN_ON(cpu_buffer,

Ingo Molnar

2008-12-18 12:48 UTC #1 lore raw

Quoted 26 lines · Lai Jiangshan

impact: make BUF_PAGE_SIZE changeable.
Except allocating/freeing page and the code using PAGE_MASK, all code expect buffer_page's length is BUF_PAGE_SIZE.
This patch make this behavior more concordant.
Signed-off-by Lai Jiangshan <[email protected]>
---
kernel/trace/ring_buffer.c +4 -0
index 668bbb5..0cf6caf 100644
@@ -158,6 +158,10 @@ struct buffer_page {
 	void *page;			/* Actual data page */
 };
 
+#define BUF_PAGE_ORDER 0
+#define BUF_PAGE_SIZE (PAGE_SIZE << BUF_PAGE_ORDER)
+#define BUF_PAGE_MASK (~(BUF_PAGE_SIZE - 1))
+
 /*
  * Also stolen from mm/slob.c. Thanks to Mathieu Desnoyers for pointing
  * this issue out.

kernel/trace/ring_buffer.c struct buffer_page { · line 172

Quoted 5 lines

kernel/trace/ring_buffer.c +1 -1

@@ -165,7 +169,7 @@ struct buffer_page {
 static inline void free_buffer_page(struct buffer_page *bpage)
 {
 	if (bpage->page)
-		free_page((unsigned long)bpage->page);
+		free_pages((unsigned long)bpage->page, BUF_PAGE_ORDER);

hm, why? Non-order-0 allocations are pretty evil - why would we ever want to do them?

Ingo

Lai Jiangshan

author

2008-12-20 09:35 UTC #2 lore raw

Quoted 9 lines · Ingo Molnar

Quoted 7 lines · Lai Jiangshan
impact: make BUF_PAGE_SIZE changeable.
Except allocating/freeing page and the code using PAGE_MASK, all code expect buffer_page's length is BUF_PAGE_SIZE.
This patch make this behavior more concordant.

[...]

Quoted 6 lines

hm, why? Non-order-0 allocations are pretty evil - why would we ever want to do them?
Ingo

I think since we introduce BUF_PAGE_SIZE instead of PAGE_SIZE for buffer_page, we should make it changeable. We can use Non-order-0 allocations, but it doesn't mean we have to use Non-order-0 allocations.

In the old codes, these lines confuse me:
return (addr & ~PAGE_MASK) - (PAGE_SIZE - BUF_PAGE_SIZE); addr &= PAGE_MASK;
This patch mostly make the codes concordant.

Lai

Ingo Molnar

2008-12-21 08:56 UTC #3 lore raw

Quoted 18 lines · Lai Jiangshan

Quoted 9 lines · Ingo Molnar
Quoted 7 lines · Lai Jiangshan
impact: make BUF_PAGE_SIZE changeable.
Except allocating/freeing page and the code using PAGE_MASK, all code expect buffer_page's length is BUF_PAGE_SIZE.
This patch make this behavior more concordant.
[...]
Quoted 6 lines
hm, why? Non-order-0 allocations are pretty evil - why would we ever want to do them?
Ingo

Quoted 8 lines

I think since we introduce BUF_PAGE_SIZE instead of PAGE_SIZE for buffer_page, we should make it changeable. We can use Non-order-0 allocations, but it doesn't mean we have to use Non-order-0 allocations.
In the old codes, these lines confuse me:
return (addr & ~PAGE_MASK) - (PAGE_SIZE - BUF_PAGE_SIZE); addr &= PAGE_MASK;
This patch mostly make the codes concordant.

ah, okay. Steve, any strong feelings against the patch? And we might just go for removing BUF_PAGE_SIZE itself instead.

Ingo

Steven Rostedt

maintainer

2008-12-22 18:37 UTC #4 lore raw

Quoted 9 lines · Ingo Molnar

Quoted 4 lines · Lai Jiangshan
In the old codes, these lines confuse me:
return (addr & ~PAGE_MASK) - (PAGE_SIZE - BUF_PAGE_SIZE); addr &= PAGE_MASK;
This patch mostly make the codes concordant.
ah, okay. Steve, any strong feelings against the patch? And we might just go for removing BUF_PAGE_SIZE itself instead.

I'll need to rename BUF_PAGE_SIZE to BUF_SIZE since it is really the size of a buffer per page. The pages will now have some header information on it to let things like splice take the page away and still be able to translate what is on the page.

-- Steve

Steven Rostedt

maintainer

2008-12-22 17:57 UTC #5 lore raw

In reply to Lai Jiangshan

Quoted 18 lines · Lai Jiangshan

Quoted 9 lines · Ingo Molnar
Quoted 7 lines · Lai Jiangshan
impact: make BUF_PAGE_SIZE changeable.
Except allocating/freeing page and the code using PAGE_MASK, all code expect buffer_page's length is BUF_PAGE_SIZE.
This patch make this behavior more concordant.
[...]
Quoted 6 lines
hm, why? Non-order-0 allocations are pretty evil - why would we ever want to do them?
Ingo

Quoted 8 lines

I think since we introduce BUF_PAGE_SIZE instead of PAGE_SIZE for buffer_page, we should make it changeable. We can use Non-order-0 allocations, but it doesn't mean we have to use Non-order-0 allocations.
In the old codes, these lines confuse me:
return (addr & ~PAGE_MASK) - (PAGE_SIZE - BUF_PAGE_SIZE); addr &= PAGE_MASK;
This patch mostly make the codes concordant.

I need to rename the BUF_PAGE_SIZE to BUF_SIZE, since it is not really a page. I'm moving to keep a header on each page so that I can use splice and still be able to process the pages elsewhere. The BUF_SIZE is the size of the PAGE - that header.

I think it is too early to move the ringbuffer to bigger than a page sub buffers.

-- Steve