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 <[email protected]>
---
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
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:
On Fri, May 18, 2012 at 6:56 PM, Steven Rostedt <[email protected]> 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
On Fri, 2012-05-18 at 20:00 -0700, Vaibhav Nagarnaik wrote:
> On Fri, May 18, 2012 at 6:56 PM, Steven Rostedt <[email protected]> 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
On Fri, May 18, 2012 at 8:19 PM, Steven Rostedt <[email protected]> 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
Commit-ID: 05fdd70d2fe1e34d8b80ec56d6e3272d9293653e
Gitweb: http://git.kernel.org/tip/05fdd70d2fe1e34d8b80ec56d6e3272d9293653e
Author: Vaibhav Nagarnaik <[email protected]>
AuthorDate: Fri, 18 May 2012 13:29:51 -0700
Committer: Steven Rostedt <[email protected]>
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/[email protected]
Cc: Laurent Chavey <[email protected]>
Cc: Justin Teravest <[email protected]>
Cc: David Sharp <[email protected]>
Signed-off-by: Vaibhav Nagarnaik <[email protected]>
Signed-off-by: Steven Rostedt <[email protected]>
---
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: