Re: [PATCH] mm: filemap: Avoid unnecessary barries and waitqueue lookups in unlock_page fastpath v5

[Peter Zijlstra]

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/