[PATCH] ring_bufer: fix BUF_PAGE_SIZE

[PATCH] ring_bufer: fix BUF_PAGE_SIZE

[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 <laijs@cn.fujitsu.com>
---
diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 668bbb5..0cf6caf 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -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,

Re: [PATCH] ring_bufer: fix BUF_PAGE_SIZE

[Ingo Molnar]

* Lai Jiangshan <laijs@cn.fujitsu.com> wrote:

> 
> 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 <laijs@cn.fujitsu.com>
> ---
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 668bbb5..0cf6caf 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -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);

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

	Ingo

Re: [PATCH] ring_bufer: fix BUF_PAGE_SIZE

[Lai Jiangshan]

Ingo Molnar wrote:
> * Lai Jiangshan <laijs@cn.fujitsu.com> wrote:
> 
>> 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.
>>
[...]
> 
> 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

Re: [PATCH] ring_bufer: fix BUF_PAGE_SIZE

[Ingo Molnar]

* Lai Jiangshan <laijs@cn.fujitsu.com> wrote:

> Ingo Molnar wrote:
> > * Lai Jiangshan <laijs@cn.fujitsu.com> wrote:
> > 
> >> 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.
> >>
> [...]
> > 
> > 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.

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

	Ingo

Re: [PATCH] ring_bufer: fix BUF_PAGE_SIZE

[Steven Rostedt]

On Sat, 20 Dec 2008, Lai Jiangshan wrote:

> Ingo Molnar wrote:
> > * Lai Jiangshan <laijs@cn.fujitsu.com> wrote:
> > 
> >> 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.
> >>
> [...]
> > 
> > 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.

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

Re: [PATCH] ring_bufer: fix BUF_PAGE_SIZE

[Steven Rostedt]

On Sun, 21 Dec 2008, Ingo Molnar wrote:
> * Lai Jiangshan <laijs@cn.fujitsu.com> wrote:

> > 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