From patchwork Sun Dec 2 01:11:57 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tejun Heo X-Patchwork-Id: 1829841 Return-Path: X-Original-To: patchwork-linux-wireless@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork2.kernel.org (Postfix) with ESMTP id D12BCDF264 for ; Sun, 2 Dec 2012 01:12:31 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753713Ab2LBBMG (ORCPT ); Sat, 1 Dec 2012 20:12:06 -0500 Received: from mail-pb0-f46.google.com ([209.85.160.46]:58310 "EHLO mail-pb0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753651Ab2LBBME (ORCPT ); Sat, 1 Dec 2012 20:12:04 -0500 Received: by mail-pb0-f46.google.com with SMTP id wy7so1247104pbc.19 for ; Sat, 01 Dec 2012 17:12:03 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=EijsHRvydUy/0LmQgoIvrRSAnO+PqXvP/CfZFm4FLZk=; b=QXq2I1+nENKKwvP9CfpET9siFKm4bq8bbDakdFFaGh2uVLFt7H+z8eAm5/tQP3sgfa 4e29BpbCdp8Hc+rRZm2SQZhCR1vBHP9NXMhBIYc6fZZASpl0GziI0qqz9s3BOKX/QoH4 xWZ/YtUkFcwqUhBtcgO2aHkB7PWXpp14G2cTJLt4gV9sYv17oqqvN7MgBvi4SXeg0XwL +lYKBUatuDB25jvVseoBpHbrzLyoAj6V3GPTSsM1nfzhTtO/oSEfPHEpYbAJ3cZ7PPpz vjAOCMv0ZmqcPd7kR0GT97+E8C3O3tVsVU6iHXGhlvawUpO2R3AXiPoBQw4vIYiv9iH5 v0ew== Received: by 10.68.235.2 with SMTP id ui2mr17386170pbc.163.1354410723167; Sat, 01 Dec 2012 17:12:03 -0800 (PST) Received: from htj.dyndns.org (c-69-181-251-227.hsd1.ca.comcast.net. [69.181.251.227]) by mx.google.com with ESMTPS id hs2sm5506854pbc.22.2012.12.01.17.12.00 (version=SSLv3 cipher=OTHER); Sat, 01 Dec 2012 17:12:01 -0800 (PST) Date: Sat, 1 Dec 2012 17:11:57 -0800 From: Tejun Heo To: Anders Kaseorg Cc: Herbert Xu , "John W. Linville" , netdev@vger.kernel.org, linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org, Zlatko Calusic , Joonsoo Kim Subject: [PATCH] workqueue: mod_delayed_work_on() shouldn't queue timer on 0 delay Message-ID: <20121202011157.GE2685@htj.dyndns.org> References: <20121130211435.GJ3873@htj.dyndns.org> <20121130225619.GD6021@htj.dyndns.org> <20121201143926.GB2685@htj.dyndns.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-wireless-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-wireless@vger.kernel.org From 8852aac25e79e38cc6529f20298eed154f60b574 Mon Sep 17 00:00:00 2001 From: Tejun Heo Date: Sat, 1 Dec 2012 16:23:42 -0800 8376fe22c7 ("workqueue: implement mod_delayed_work[_on]()") implemented mod_delayed_work[_on]() using the improved try_to_grab_pending(). The function is later used, among others, to replace [__]candel_delayed_work() + queue_delayed_work() combinations. Unfortunately, a delayed_work item w/ zero @delay is handled slightly differently by mod_delayed_work_on() compared to queue_delayed_work_on(). The latter skips timer altogether and directly queues it using queue_work_on() while the former schedules timer which will expire on the closest tick. This means, when @delay is zero, that [__]cancel_delayed_work() + queue_delayed_work_on() makes the target item immediately executable while mod_delayed_work_on() may induce delay of upto a full tick. This somewhat subtle difference breaks some of the converted users. e.g. block queue plugging uses delayed_work for deferred processing and uses mod_delayed_work_on() when the queue needs to be immediately unplugged. The above problem manifested as noticeably higher number of context switches under certain circumstances. The difference in behavior was caused by missing special case handling for 0 delay in mod_delayed_work_on() compared to queue_delayed_work_on(). Joonsoo Kim posted a patch to add it - ("workqueue: optimize mod_delayed_work_on() when @delay == 0")[1]. The patch was queued for 3.8 but it was described as optimization and I missed that it was a correctness issue. As both queue_delayed_work_on() and mod_delayed_work_on() use __queue_delayed_work() for queueing, it seems that the better approach is to move the 0 delay special handling to the function instead of duplicating it in mod_delayed_work_on(). Fix the problem by moving 0 delay special case handling from queue_delayed_work_on() to __queue_delayed_work(). This replaces Joonsoo's patch. [1] http://thread.gmane.org/gmane.linux.kernel/1379011/focus=1379012 Signed-off-by: Tejun Heo Reported-and-tested-by: Anders Kaseorg Reported-and-tested-by: Zlatko Calusic LKML-Reference: LKML-Reference: <50A78AA9.5040904@iskon.hr> Cc: Joonsoo Kim --- Applied to wq/for-3.7-fixes. Pull request sent. Thanks! kernel/workqueue.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/kernel/workqueue.c b/kernel/workqueue.c index ac25db1..084aa47 100644 --- a/kernel/workqueue.c +++ b/kernel/workqueue.c @@ -1364,6 +1364,17 @@ static void __queue_delayed_work(int cpu, struct workqueue_struct *wq, BUG_ON(timer_pending(timer)); BUG_ON(!list_empty(&work->entry)); + /* + * If @delay is 0, queue @dwork->work immediately. This is for + * both optimization and correctness. The earliest @timer can + * expire is on the closest next tick and delayed_work users depend + * on that there's no such delay when @delay is 0. + */ + if (!delay) { + __queue_work(cpu, wq, &dwork->work); + return; + } + timer_stats_timer_set_start_info(&dwork->timer); /* @@ -1417,9 +1428,6 @@ bool queue_delayed_work_on(int cpu, struct workqueue_struct *wq, bool ret = false; unsigned long flags; - if (!delay) - return queue_work_on(cpu, wq, &dwork->work); - /* read the comment in __queue_work() */ local_irq_save(flags);