From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751831AbbDMVl4 (ORCPT ); Mon, 13 Apr 2015 17:41:56 -0400 Received: from www.linutronix.de ([62.245.132.108]:33813 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751020AbbDMVly (ORCPT ); Mon, 13 Apr 2015 17:41:54 -0400 Date: Mon, 13 Apr 2015 23:42:03 +0200 (CEST) From: Thomas Gleixner To: Arnd Bergmann 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 In-Reply-To: <9614284.QjZ4xls6zV@wuerfel> Message-ID: References: <20150413210009.682000343@linutronix.de> <20150413210035.274411419@linutronix.de> <9614284.QjZ4xls6zV@wuerfel> User-Agent: Alpine 2.11 (DEB 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-Linutronix-Spam-Score: -1.0 X-Linutronix-Spam-Level: - X-Linutronix-Spam-Status: No , -1.0 points, 5.0 required, ALL_TRUSTED=-1,SHORTCIRCUIT=-0.0001 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 13 Apr 2015, Arnd Bergmann wrote: > 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? Yes, I missed that. Simple fix for this is: unsigned long t_ns = priv->tx_coalesce_usecs * NSEC_PER_USEC / 2; hrtimer_start_range_ns(&priv->tx_coalesce_timer, ns_to_ktime(t_ns), t_ns, HRTIMER_MODE_REL); Thanks, tglx