* [PATCH] tracing: Merge separate resize loops
@ 2012-05-18 20:29 Vaibhav Nagarnaik
2012-05-19 1:56 ` Steven Rostedt
2012-05-21 9:44 ` [tip:perf/core] ring-buffer: " tip-bot for Vaibhav Nagarnaik
0 siblings, 2 replies; 6+ messages in thread
From: Vaibhav Nagarnaik @ 2012-05-18 20:29 UTC (permalink / raw)
To: Steven Rostedt
Cc: Laurent Chavey, Justin Teravest, David Sharp, linux-kernel,
Vaibhav Nagarnaik
There are 2 separate loops to resize cpu buffers that are online and
offline. Merge them to make the code look better.
Also change the name from update_completion to update_done to allow
shorter lines.
Signed-off-by: Vaibhav Nagarnaik <vnagarnaik@google.com>
---
kernel/trace/ring_buffer.c | 42 ++++++++++++++++--------------------------
1 files changed, 16 insertions(+), 26 deletions(-)
diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index d4c458a..4a142b2 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -473,7 +473,7 @@ struct ring_buffer_per_cpu {
int nr_pages_to_update;
struct list_head new_pages; /* new pages to add */
struct work_struct update_pages_work;
- struct completion update_completion;
+ struct completion update_done;
};
struct ring_buffer {
@@ -1054,7 +1054,7 @@ rb_allocate_cpu_buffer(struct ring_buffer *buffer, int nr_pages, int cpu)
lockdep_set_class(&cpu_buffer->reader_lock, buffer->reader_lock_key);
cpu_buffer->lock = (arch_spinlock_t)__ARCH_SPIN_LOCK_UNLOCKED;
INIT_WORK(&cpu_buffer->update_pages_work, update_pages_handler);
- init_completion(&cpu_buffer->update_completion);
+ init_completion(&cpu_buffer->update_done);
bpage = kzalloc_node(ALIGN(sizeof(*bpage), cache_line_size()),
GFP_KERNEL, cpu_to_node(cpu));
@@ -1457,7 +1457,7 @@ static void update_pages_handler(struct work_struct *work)
struct ring_buffer_per_cpu *cpu_buffer = container_of(work,
struct ring_buffer_per_cpu, update_pages_work);
rb_update_pages(cpu_buffer);
- complete(&cpu_buffer->update_completion);
+ complete(&cpu_buffer->update_done);
}
/**
@@ -1530,45 +1530,36 @@ int ring_buffer_resize(struct ring_buffer *buffer, unsigned long size,
get_online_cpus();
/*
* Fire off all the required work handlers
- * Look out for offline CPUs
- */
- for_each_buffer_cpu(buffer, cpu) {
- cpu_buffer = buffer->buffers[cpu];
- if (!cpu_buffer->nr_pages_to_update ||
- !cpu_online(cpu))
- continue;
-
- schedule_work_on(cpu, &cpu_buffer->update_pages_work);
- }
- /*
- * This loop is for the CPUs that are not online.
- * We can't schedule anything on them, but it's not necessary
+ * We can't schedule on offline CPUs, but it's not necessary
* since we can change their buffer sizes without any race.
*/
for_each_buffer_cpu(buffer, cpu) {
cpu_buffer = buffer->buffers[cpu];
- if (!cpu_buffer->nr_pages_to_update ||
- cpu_online(cpu))
+ if (!cpu_buffer->nr_pages_to_update)
continue;
- rb_update_pages(cpu_buffer);
+ if (cpu_online(cpu))
+ schedule_work_on(cpu,
+ &cpu_buffer->update_pages_work);
+ else
+ rb_update_pages(cpu_buffer);
}
/* wait for all the updates to complete */
for_each_buffer_cpu(buffer, cpu) {
cpu_buffer = buffer->buffers[cpu];
- if (!cpu_buffer->nr_pages_to_update||
- !cpu_online(cpu))
+ if (!cpu_buffer->nr_pages_to_update)
continue;
- wait_for_completion(&cpu_buffer->update_completion);
- /* reset this value */
+ if (cpu_online(cpu))
+ wait_for_completion(&cpu_buffer->update_done);
cpu_buffer->nr_pages_to_update = 0;
}
put_online_cpus();
} else {
cpu_buffer = buffer->buffers[cpu_id];
+
if (nr_pages == cpu_buffer->nr_pages)
goto out;
@@ -1588,13 +1579,12 @@ int ring_buffer_resize(struct ring_buffer *buffer, unsigned long size,
if (cpu_online(cpu_id)) {
schedule_work_on(cpu_id,
&cpu_buffer->update_pages_work);
- wait_for_completion(&cpu_buffer->update_completion);
+ wait_for_completion(&cpu_buffer->update_done);
} else
rb_update_pages(cpu_buffer);
- put_online_cpus();
- /* reset this value */
cpu_buffer->nr_pages_to_update = 0;
+ put_online_cpus();
}
out:
--
1.7.7.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] tracing: Merge separate resize loops
2012-05-18 20:29 [PATCH] tracing: Merge separate resize loops Vaibhav Nagarnaik
@ 2012-05-19 1:56 ` Steven Rostedt
2012-05-19 3:00 ` Vaibhav Nagarnaik
2012-05-21 9:44 ` [tip:perf/core] ring-buffer: " tip-bot for Vaibhav Nagarnaik
1 sibling, 1 reply; 6+ messages in thread
From: Steven Rostedt @ 2012-05-19 1:56 UTC (permalink / raw)
To: Vaibhav Nagarnaik
Cc: Laurent Chavey, Justin Teravest, David Sharp, linux-kernel
On Fri, 2012-05-18 at 13:29 -0700, Vaibhav Nagarnaik wrote:
>
> /* wait for all the updates to complete */
> for_each_buffer_cpu(buffer, cpu) {
> cpu_buffer = buffer->buffers[cpu];
> - if (!cpu_buffer->nr_pages_to_update||
> - !cpu_online(cpu))
> + if (!cpu_buffer->nr_pages_to_update)
> continue;
>
Why did you make this change? As we only need to wait for completions.
> - wait_for_completion(&cpu_buffer->update_completion);
> - /* reset this value */
> + if (cpu_online(cpu))
> + wait_for_completion(&cpu_buffer->update_done);
> cpu_buffer->nr_pages_to_update = 0;
Or was the original patch buggy, where we never set nr_page_to_update to
zero for the offline case?
-- Steve
> }
>
> put_online_cpus();
> } else {
> cpu_buffer = buffer->buffers[cpu_id];
> +
> if (nr_pages == cpu_buffer->nr_pages)
> goto out;
>
> @@ -1588,13 +1579,12 @@ int ring_buffer_resize(struct ring_buffer *buffer, unsigned long size,
> if (cpu_online(cpu_id)) {
> schedule_work_on(cpu_id,
> &cpu_buffer->update_pages_work);
> - wait_for_completion(&cpu_buffer->update_completion);
> + wait_for_completion(&cpu_buffer->update_done);
> } else
> rb_update_pages(cpu_buffer);
>
> - put_online_cpus();
> - /* reset this value */
> cpu_buffer->nr_pages_to_update = 0;
> + put_online_cpus();
> }
>
> out:
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tracing: Merge separate resize loops
2012-05-19 1:56 ` Steven Rostedt
@ 2012-05-19 3:00 ` Vaibhav Nagarnaik
2012-05-19 3:19 ` Steven Rostedt
0 siblings, 1 reply; 6+ messages in thread
From: Vaibhav Nagarnaik @ 2012-05-19 3:00 UTC (permalink / raw)
To: Steven Rostedt; +Cc: Laurent Chavey, Justin Teravest, David Sharp, linux-kernel
On Fri, May 18, 2012 at 6:56 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Fri, 2012-05-18 at 13:29 -0700, Vaibhav Nagarnaik wrote:
>>
>> /* wait for all the updates to complete */
>> for_each_buffer_cpu(buffer, cpu) {
>> cpu_buffer = buffer->buffers[cpu];
>> - if (!cpu_buffer->nr_pages_to_update||
>> - !cpu_online(cpu))
>> + if (!cpu_buffer->nr_pages_to_update)
>> continue;
>>
>
> Why did you make this change? As we only need to wait for completions.
>
>> - wait_for_completion(&cpu_buffer->update_completion);
>> - /* reset this value */
>> + if (cpu_online(cpu))
>> + wait_for_completion(&cpu_buffer->update_done);
>> cpu_buffer->nr_pages_to_update = 0;
>
> Or was the original patch buggy, where we never set nr_page_to_update to
> zero for the offline case?
I don't see a bug, since at the start of the resize operation, we
always recalculate this value. It will be reset to 0, if there are no
updates. I set it to zero at the end just as a precautionary measure.
Vaibhav Nagarnaik
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tracing: Merge separate resize loops
2012-05-19 3:00 ` Vaibhav Nagarnaik
@ 2012-05-19 3:19 ` Steven Rostedt
2012-05-19 4:57 ` Vaibhav Nagarnaik
0 siblings, 1 reply; 6+ messages in thread
From: Steven Rostedt @ 2012-05-19 3:19 UTC (permalink / raw)
To: Vaibhav Nagarnaik
Cc: Laurent Chavey, Justin Teravest, David Sharp, linux-kernel
On Fri, 2012-05-18 at 20:00 -0700, Vaibhav Nagarnaik wrote:
> On Fri, May 18, 2012 at 6:56 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> > On Fri, 2012-05-18 at 13:29 -0700, Vaibhav Nagarnaik wrote:
> >>
> >> /* wait for all the updates to complete */
> >> for_each_buffer_cpu(buffer, cpu) {
> >> cpu_buffer = buffer->buffers[cpu];
> >> - if (!cpu_buffer->nr_pages_to_update||
> >> - !cpu_online(cpu))
> >> + if (!cpu_buffer->nr_pages_to_update)
> >> continue;
> >>
> >
> > Why did you make this change? As we only need to wait for completions.
> >
> >> - wait_for_completion(&cpu_buffer->update_completion);
> >> - /* reset this value */
> >> + if (cpu_online(cpu))
> >> + wait_for_completion(&cpu_buffer->update_done);
> >> cpu_buffer->nr_pages_to_update = 0;
> >
> > Or was the original patch buggy, where we never set nr_page_to_update to
> > zero for the offline case?
>
> I don't see a bug, since at the start of the resize operation, we
> always recalculate this value. It will be reset to 0, if there are no
> updates. I set it to zero at the end just as a precautionary measure.
But if there were updates on a offline CPU, then the original patch
would not have set this to zero at the end.
Or are you just saying that we don't need to set this to zero, as it
isn't used later on? And when we re-enter this function (where its the
only place, and what it calls, that uses nr_page_to_update), it gets
reset.
IOW, this reset is just a "clean up" of the nr_pages_to_update. Right?
-- Steve
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tracing: Merge separate resize loops
2012-05-19 3:19 ` Steven Rostedt
@ 2012-05-19 4:57 ` Vaibhav Nagarnaik
0 siblings, 0 replies; 6+ messages in thread
From: Vaibhav Nagarnaik @ 2012-05-19 4:57 UTC (permalink / raw)
To: Steven Rostedt; +Cc: Laurent Chavey, Justin Teravest, David Sharp, linux-kernel
On Fri, May 18, 2012 at 8:19 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> But if there were updates on a offline CPU, then the original patch
> would not have set this to zero at the end.
Right.
> Or are you just saying that we don't need to set this to zero, as it
> isn't used later on? And when we re-enter this function (where its the
> only place, and what it calls, that uses nr_page_to_update), it gets
> reset.
>
> IOW, this reset is just a "clean up" of the nr_pages_to_update. Right?
Yes, that's exactly right and a much better way to put it :)
It's just a reset which isn't strictly needed since nr_pages_to_update
gets initialized before it is used and isn't used after resize
operation.
Vaibhav Nagarnaik
^ permalink raw reply [flat|nested] 6+ messages in thread
* [tip:perf/core] ring-buffer: Merge separate resize loops
2012-05-18 20:29 [PATCH] tracing: Merge separate resize loops Vaibhav Nagarnaik
2012-05-19 1:56 ` Steven Rostedt
@ 2012-05-21 9:44 ` tip-bot for Vaibhav Nagarnaik
1 sibling, 0 replies; 6+ messages in thread
From: tip-bot for Vaibhav Nagarnaik @ 2012-05-21 9:44 UTC (permalink / raw)
To: linux-tip-commits
Cc: linux-kernel, vnagarnaik, hpa, mingo, dhsharp, rostedt, chavey,
tglx, teravest
Commit-ID: 05fdd70d2fe1e34d8b80ec56d6e3272d9293653e
Gitweb: http://git.kernel.org/tip/05fdd70d2fe1e34d8b80ec56d6e3272d9293653e
Author: Vaibhav Nagarnaik <vnagarnaik@google.com>
AuthorDate: Fri, 18 May 2012 13:29:51 -0700
Committer: Steven Rostedt <rostedt@goodmis.org>
CommitDate: Sat, 19 May 2012 08:28:50 -0400
ring-buffer: Merge separate resize loops
There are 2 separate loops to resize cpu buffers that are online and
offline. Merge them to make the code look better.
Also change the name from update_completion to update_done to allow
shorter lines.
Link: http://lkml.kernel.org/r/1337372991-14783-1-git-send-email-vnagarnaik@google.com
Cc: Laurent Chavey <chavey@google.com>
Cc: Justin Teravest <teravest@google.com>
Cc: David Sharp <dhsharp@google.com>
Signed-off-by: Vaibhav Nagarnaik <vnagarnaik@google.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
kernel/trace/ring_buffer.c | 41 +++++++++++++++--------------------------
1 files changed, 15 insertions(+), 26 deletions(-)
diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 68388f8..6420cda 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -473,7 +473,7 @@ struct ring_buffer_per_cpu {
int nr_pages_to_update;
struct list_head new_pages; /* new pages to add */
struct work_struct update_pages_work;
- struct completion update_completion;
+ struct completion update_done;
};
struct ring_buffer {
@@ -1058,7 +1058,7 @@ rb_allocate_cpu_buffer(struct ring_buffer *buffer, int nr_pages, int cpu)
lockdep_set_class(&cpu_buffer->reader_lock, buffer->reader_lock_key);
cpu_buffer->lock = (arch_spinlock_t)__ARCH_SPIN_LOCK_UNLOCKED;
INIT_WORK(&cpu_buffer->update_pages_work, update_pages_handler);
- init_completion(&cpu_buffer->update_completion);
+ init_completion(&cpu_buffer->update_done);
bpage = kzalloc_node(ALIGN(sizeof(*bpage), cache_line_size()),
GFP_KERNEL, cpu_to_node(cpu));
@@ -1461,7 +1461,7 @@ static void update_pages_handler(struct work_struct *work)
struct ring_buffer_per_cpu *cpu_buffer = container_of(work,
struct ring_buffer_per_cpu, update_pages_work);
rb_update_pages(cpu_buffer);
- complete(&cpu_buffer->update_completion);
+ complete(&cpu_buffer->update_done);
}
/**
@@ -1534,39 +1534,29 @@ int ring_buffer_resize(struct ring_buffer *buffer, unsigned long size,
get_online_cpus();
/*
* Fire off all the required work handlers
- * Look out for offline CPUs
- */
- for_each_buffer_cpu(buffer, cpu) {
- cpu_buffer = buffer->buffers[cpu];
- if (!cpu_buffer->nr_pages_to_update ||
- !cpu_online(cpu))
- continue;
-
- schedule_work_on(cpu, &cpu_buffer->update_pages_work);
- }
- /*
- * This loop is for the CPUs that are not online.
- * We can't schedule anything on them, but it's not necessary
+ * We can't schedule on offline CPUs, but it's not necessary
* since we can change their buffer sizes without any race.
*/
for_each_buffer_cpu(buffer, cpu) {
cpu_buffer = buffer->buffers[cpu];
- if (!cpu_buffer->nr_pages_to_update ||
- cpu_online(cpu))
+ if (!cpu_buffer->nr_pages_to_update)
continue;
- rb_update_pages(cpu_buffer);
+ if (cpu_online(cpu))
+ schedule_work_on(cpu,
+ &cpu_buffer->update_pages_work);
+ else
+ rb_update_pages(cpu_buffer);
}
/* wait for all the updates to complete */
for_each_buffer_cpu(buffer, cpu) {
cpu_buffer = buffer->buffers[cpu];
- if (!cpu_buffer->nr_pages_to_update ||
- !cpu_online(cpu))
+ if (!cpu_buffer->nr_pages_to_update)
continue;
- wait_for_completion(&cpu_buffer->update_completion);
- /* reset this value */
+ if (cpu_online(cpu))
+ wait_for_completion(&cpu_buffer->update_done);
cpu_buffer->nr_pages_to_update = 0;
}
@@ -1593,13 +1583,12 @@ int ring_buffer_resize(struct ring_buffer *buffer, unsigned long size,
if (cpu_online(cpu_id)) {
schedule_work_on(cpu_id,
&cpu_buffer->update_pages_work);
- wait_for_completion(&cpu_buffer->update_completion);
+ wait_for_completion(&cpu_buffer->update_done);
} else
rb_update_pages(cpu_buffer);
- put_online_cpus();
- /* reset this value */
cpu_buffer->nr_pages_to_update = 0;
+ put_online_cpus();
}
out:
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-05-21 9:45 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-05-18 20:29 [PATCH] tracing: Merge separate resize loops Vaibhav Nagarnaik
2012-05-19 1:56 ` Steven Rostedt
2012-05-19 3:00 ` Vaibhav Nagarnaik
2012-05-19 3:19 ` Steven Rostedt
2012-05-19 4:57 ` Vaibhav Nagarnaik
2012-05-21 9:44 ` [tip:perf/core] ring-buffer: " tip-bot for Vaibhav Nagarnaik
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox