Prev: Mtd: fixed coding style indent errors
Next: Use xvmalloc to store compressed chunks
From: Nick Piggin on 22 Jul 2010 05:00
On Thu, Jul 22, 2010 at 11:02:19AM +0300, Artem Bityutskiy wrote:
> On Thu, 2010-07-22 at 18:05 +1000, Nick Piggin wrote:
> > I can see what you mean, but I think the designs in core code should
> > be made as efficient as possible _unless_ there is some complication
> > in doing otherwise (not the other way around).
> >
> > This is producing 2 unrequired context switches, so I really would
> > like to see it done properly. Setting up a timer is really pretty
> > simple (or if you would care to implement a delayed process wakeup
> > API, I think that would be useful -- I'm surprised there isn't one
> > already).
>
> OK, NP, I'll work on this.

Thanks.


> The only problem I see is that it will involve more maintainers and
> trees (I guess Ingo?), and make it even more difficult for me to reach
> upstream :-) But let's try and see!

I wouldn't worry about that. It's so simple that if you end up coding
the helper function to do a timer delayed wakeup, just send it to Jens
in your series, cc Ingo on it if you'd like.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Christoph Hellwig on 22 Jul 2010 05:10
On Thu, Jul 22, 2010 at 10:41:55AM +1000, Dave Chinner wrote:
> if (wakeup_bdi) {
> trace_writeback_wakeup(bdi)
> spin_lock(&bdi->wb_lock);
> if (!bdi->wb.task) {{
> trace_writeback_wakeup_nothread(bdi);
> wake_up_process(default_backing_dev_info.wb.task);
> } else
> wake_up_process(bdi->wb.task);
> spin_unlock(&bdi->wb_lock);

That gives us duplicate traces for the first case, what about:

if (wakeup_bdi) {
spin_lock(&bdi->wb_lock);
if (bdi->wb.task) {
trace_writeback_wake_thread(bdi);
wake_up_process(bdi->wb.task);
} else {
trace_writeback_wake_forker_thread(bdi);
wake_up_process(default_backing_dev_info.wb.task);
}
spin_unlock(&bdi->wb_lock);
}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Artem Bityutskiy on 22 Jul 2010 05:40
On Thu, 2010-07-22 at 05:00 -0400, Christoph Hellwig wrote:
> if (wakeup_bdi) {
> spin_lock(&bdi->wb_lock);
> if (bdi->wb.task) {
> trace_writeback_wake_thread(bdi);
> wake_up_process(bdi->wb.task);
> } else {
> trace_writeback_wake_forker_thread(bdi);
> wake_up_process(default_backing_dev_info.wb.task);
> }
> spin_unlock(&bdi->wb_lock);
> }

Side note: I've noticed you've made the optimistic "if" condition first,
which is better, and I'll also amend this in v3.

--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Artem Bityutskiy on 22 Jul 2010 06:00
On Thu, 2010-07-22 at 18:05 +1000, Nick Piggin wrote:
> This is producing 2 unrequired context switches, so I really would
> like to see it done properly. Setting up a timer is really pretty
> simple (or if you would care to implement a delayed process wakeup
> API, I think that would be useful -- I'm surprised there isn't one
> already).

OK, a generic 'wake_up_process_timeout()' would need a timer, and it
cannot set it up on stack because it has to return immediately, this is
why we do not have such a generic helper, I think.

--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Artem Bityutskiy on 22 Jul 2010 09:40
On Thu, 2010-07-22 at 05:00 -0400, Christoph Hellwig wrote:
> On Thu, Jul 22, 2010 at 10:41:55AM +1000, Dave Chinner wrote:
> > if (wakeup_bdi) {
> > trace_writeback_wakeup(bdi)
> > spin_lock(&bdi->wb_lock);
> > if (!bdi->wb.task) {{
> > trace_writeback_wakeup_nothread(bdi);
> > wake_up_process(default_backing_dev_info.wb.task);
> > } else
> > wake_up_process(bdi->wb.task);
> > spin_unlock(&bdi->wb_lock);
>
> That gives us duplicate traces for the first case, what about:
>
> if (wakeup_bdi) {
> spin_lock(&bdi->wb_lock);
> if (bdi->wb.task) {
> trace_writeback_wake_thread(bdi);
> wake_up_process(bdi->wb.task);
> } else {
> trace_writeback_wake_forker_thread(bdi);
> wake_up_process(default_backing_dev_info.wb.task);
> }
> spin_unlock(&bdi->wb_lock);
> }

But Dave's version is what we have in 'bdi_queue_work()' as well. I it
is OK - first trace point is triggered every time the bdi thread is
_wanted_ to be woken up.

Then if it does not exist, we need to do something special to wake it up
(ask the forker thread which to create it). This is a different event
and we use a different trace point.

--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
First  |  Prev  |  Next  |  Last
Pages: 1 2 3 4
Prev: Mtd: fixed coding style indent errors
Next: Use xvmalloc to store compressed chunks