On Wed, May 21, 2014 at 01:15:01PM +0100, Mel Gorman wrote:
> Andrew had suggested dropping v4 of the patch entirely as the numbers were
> marginal and the complexity was high. However, even on a relatively small
> machine running simple workloads the overhead of page_waitqueue and wakeup
> functions is around 5% of system CPU time. That's quite high for basic
> operations so I felt it was worth another shot. The performance figures
> are better with this version than they were for v4 and overall the patch
> should be more comprehensible.
Simpler patch and better performance, yay!
> This patch introduces a new page flag for 64-bit capable machines,
> PG_waiters, to signal there are processes waiting on PG_lock and uses it to
> avoid memory barriers and waitqueue hash lookup in the unlock_page fastpath.
The patch seems to also explicitly use it for PG_writeback, yet no
mention of that here.
> diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c
> index 0ffa20a..f829e73 100644
> --- a/kernel/sched/wait.c
> +++ b/kernel/sched/wait.c
> @@ -167,31 +167,39 @@ EXPORT_SYMBOL_GPL(__wake_up_sync); /* For internal use only */
> * stops them from bleeding out - it would still allow subsequent
> * loads to move into the critical region).
> */
> +static inline void
Make that __always_inline, that way we're guaranteed to optimize the
build time constant .page=NULL cases.
> +__prepare_to_wait(wait_queue_head_t *q, wait_queue_t *wait,
> + struct page *page, int state, bool exclusive)
> {
> unsigned long flags;
>
> + if (page && !PageWaiters(page))
> + SetPageWaiters(page);
> + if (list_empty(&wait->task_list)) {
> + if (exclusive) {
> + wait->flags |= WQ_FLAG_EXCLUSIVE;
> + __add_wait_queue_tail(q, wait);
> + } else {
I'm fairly sure we've just initialized the wait thing to 0, so clearing
the bit would be superfluous.
> + wait->flags &= ~WQ_FLAG_EXCLUSIVE;
> + __add_wait_queue(q, wait);
> + }
> + }
> set_current_state(state);
> spin_unlock_irqrestore(&q->lock, flags);
> }
> +
> +void
> +prepare_to_wait(wait_queue_head_t *q, wait_queue_t *wait, int state)
> +{
> + return __prepare_to_wait(q, wait, NULL, state, false);
> +}
> EXPORT_SYMBOL(prepare_to_wait);
>
> void
> prepare_to_wait_exclusive(wait_queue_head_t *q, wait_queue_t *wait, int state)
> {
> + return __prepare_to_wait(q, wait, NULL, state, true);
> }
> EXPORT_SYMBOL(prepare_to_wait_exclusive);
>
> @@ -228,7 +236,8 @@ EXPORT_SYMBOL(prepare_to_wait_event);
> * the wait descriptor from the given waitqueue if still
> * queued.
> */
> +static inline void __finish_wait(wait_queue_head_t *q, wait_queue_t *wait,
> + struct page *page)
> {
Same thing, make that __always_inline.
> unsigned long flags;
>
> @@ -249,9 +258,16 @@ void finish_wait(wait_queue_head_t *q, wait_queue_t *wait)
> if (!list_empty_careful(&wait->task_list)) {
> spin_lock_irqsave(&q->lock, flags);
> list_del_init(&wait->task_list);
> + if (page && !waitqueue_active(q))
> + ClearPageWaiters(page);
> spin_unlock_irqrestore(&q->lock, flags);
> }
> }
> +
> +void finish_wait(wait_queue_head_t *q, wait_queue_t *wait)
> +{
> + return __finish_wait(q, wait, NULL);
> +}
> EXPORT_SYMBOL(finish_wait);
>
> /**
> @@ -374,6 +427,19 @@ int __sched out_of_line_wait_on_bit_lock(void *word, int bit,
> }
> EXPORT_SYMBOL(out_of_line_wait_on_bit_lock);
>
> +void __wake_up_page_bit(wait_queue_head_t *wqh, struct page *page, void *word, int bit)
> +{
> + struct wait_bit_key key = __WAIT_BIT_KEY_INITIALIZER(word, bit);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&wqh->lock, flags);
> + if (waitqueue_active(wqh))
> + __wake_up_common(wqh, TASK_NORMAL, 1, 0, &key);
> + else
> + ClearPageWaiters(page);
> + spin_unlock_irqrestore(&wqh->lock, flags);
> +}
Seeing how word is always going to be &page->flags, might it make sense
to remove that argument?
Anyway, looks good in principle. Oleg?
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
