From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753855AbbDMVZ0 (ORCPT ); Mon, 13 Apr 2015 17:25:26 -0400 Received: from mout.kundenserver.de ([212.227.126.131]:59552 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751826AbbDMVZT (ORCPT ); Mon, 13 Apr 2015 17:25:19 -0400 From: Arnd Bergmann To: Thomas Gleixner Cc: LKML , Peter Zijlstra , Ingo Molnar , "David S. Miller" , dingtianhong , Zhangfei Gao , Dan Carpenter , netdev@vger.kernel.org Subject: Re: [patch 4/5] net: hip04: Make tx coalesce timer actually work Date: Mon, 13 Apr 2015 23:24:58 +0200 Message-ID: <9614284.QjZ4xls6zV@wuerfel> User-Agent: KMail/4.11.5 (Linux/3.16.0-10-generic; KDE/4.11.5; x86_64; ; ) In-Reply-To: <20150413210035.274411419@linutronix.de> References: <20150413210009.682000343@linutronix.de> <20150413210035.274411419@linutronix.de> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Provags-ID: V03:K0:FSl4YJRhcuG1hWJpUkQTqLv8rmW62kpo+zE5VJbqQ73OqsrFEWj BMZ+158xoyJhxd0PiUye4mhHEVaLZWtvDBFQlYS/M2amapwGLiefz0x27aAK552rOIk4O83 ETi3JrClbgXUffMTQ6ty056Yec99HODj0FfIPnxOfHh6zx6a9FTz7naez3nSPq5zTxNYj7a QrV6248RCk6qov6NqCMZA== X-UI-Out-Filterresults: notjunk:1; Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Monday 13 April 2015 21:02:23 Thomas Gleixner wrote: > The code sets the expiry value of the timer to a relative value and > starts it with hrtimer_start_expires. That's fine, but that only works > once. The timer is started in relative mode, so the expiry value gets > overwritten with the absolut expiry time (now + expiry). > > So once the timer expired, a new call to hrtimer_start_expires results > in an immidiately expired timer, because the expiry value is > already in the past. > > Use the proper mechanisms to (re)start the timer in the intended way. > > Signed-off-by: Thomas Gleixner > Cc: "David S. Miller" > Cc: dingtianhong > Cc: Arnd Bergmann > Cc: Zhangfei Gao > Cc: Dan Carpenter > Cc: netdev@vger.kernel.org Thanks a lot for the fix. The mistake was clearly mine, as I had sent a patch to introduce the tx coalesce timer without access to hardware or a way to test that what I did was correct. There are other known problems in the version of the driver that got merged, and I believe that someone is now looking at them. What I think we really want here is a way for user space to configure both the minimum and maximum coalesce timer separately rather than assuming half the time is what we want. Arnd > @@ -413,6 +413,15 @@ out: > return count; > } > > +static void hip04_start_tx_timer(struct hip04_priv *priv) > +{ > + ktime_t t; > + > + /* allow timer to fire after half the time at the earliest */ > + t = ktime_set(0, priv->tx_coalesce_usecs * NSEC_PER_USEC / 2); > + hrtimer_start(&priv->tx_coalesce_timer, t, HRTIMER_MODE_REL); > +} Question: this looks to me like it sets both the minimum and maximum time to priv->tx_coalesce_usecs/2, when the intention was to set the minimum to priv->tx_coalesce_usecs/2 and the maximum to priv->tx_coalesce_usecs. Am I missing something subtle here, or did you just misread my original intention from the botched code? Arnd