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 <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: dingtianhong <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Zhangfei Gao <[email protected]>
Cc: Dan Carpenter <[email protected]>
Cc: [email protected]
---
drivers/net/ethernet/hisilicon/hip04_eth.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
Index: linux/drivers/net/ethernet/hisilicon/hip04_eth.c
===================================================================
--- linux.orig/drivers/net/ethernet/hisilicon/hip04_eth.c
+++ linux/drivers/net/ethernet/hisilicon/hip04_eth.c
@@ -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);
+}
+
static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
{
struct hip04_priv *priv = netdev_priv(ndev);
@@ -466,8 +475,7 @@ static int hip04_mac_start_xmit(struct s
}
} else if (!hrtimer_is_queued(&priv->tx_coalesce_timer)) {
/* cleanup not pending yet, start a new timer */
- hrtimer_start_expires(&priv->tx_coalesce_timer,
- HRTIMER_MODE_REL);
+ hip04_start_tx_timer(priv);
}
return NETDEV_TX_OK;
@@ -549,7 +557,7 @@ done:
/* clean up tx descriptors and start a new timer if necessary */
tx_remaining = hip04_tx_reclaim(ndev, false);
if (rx < budget && tx_remaining)
- hrtimer_start_expires(&priv->tx_coalesce_timer, HRTIMER_MODE_REL);
+ hip04_start_tx_timer(priv);
return rx;
}
@@ -809,7 +817,6 @@ static int hip04_mac_probe(struct platfo
struct hip04_priv *priv;
struct resource *res;
unsigned int irq;
- ktime_t txtime;
int ret;
ndev = alloc_etherdev(sizeof(struct hip04_priv));
@@ -846,9 +853,6 @@ static int hip04_mac_probe(struct platfo
*/
priv->tx_coalesce_frames = TX_DESC_NUM * 3 / 4;
priv->tx_coalesce_usecs = 200;
- /* allow timer to fire after half the time at the earliest */
- txtime = ktime_set(0, priv->tx_coalesce_usecs * NSEC_PER_USEC / 2);
- hrtimer_set_expires_range(&priv->tx_coalesce_timer, txtime, txtime);
priv->tx_coalesce_timer.function = tx_done;
priv->map = syscon_node_to_regmap(arg.np);
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 <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: dingtianhong <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Zhangfei Gao <[email protected]>
> Cc: Dan Carpenter <[email protected]>
> Cc: [email protected]
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
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 <[email protected]>
> > Cc: "David S. Miller" <[email protected]>
> > Cc: dingtianhong <[email protected]>
> > Cc: Arnd Bergmann <[email protected]>
> > Cc: Zhangfei Gao <[email protected]>
> > Cc: Dan Carpenter <[email protected]>
> > Cc: [email protected]
>
> 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
On Monday 13 April 2015 23:42:03 Thomas Gleixner wrote:
> >
> > 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);
Ah, good. I have to admit that I'd probably make the same mistake
again if I was to do this for another driver and you hadn't sent
the fix. The hrtimer_set_expires_range() function just looked like
it had been designed for the use case I was interested in ;-).
Any idea how to prevent the next person from making the same mistake?
Arnd
On Tue, 14 Apr 2015, Arnd Bergmann wrote:
> On Monday 13 April 2015 23:42:03 Thomas Gleixner wrote:
> > >
> > > 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);
>
> Ah, good. I have to admit that I'd probably make the same mistake
> again if I was to do this for another driver and you hadn't sent
> the fix. The hrtimer_set_expires_range() function just looked like
> it had been designed for the use case I was interested in ;-).
>
> Any idea how to prevent the next person from making the same mistake?
Yes. Documentation :)
Thanks,
tglx
On 2015/4/14 6:08, Thomas Gleixner wrote:
> On Tue, 14 Apr 2015, Arnd Bergmann wrote:
>> On Monday 13 April 2015 23:42:03 Thomas Gleixner wrote:
>>>>
>>>> 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);
>>
>> Ah, good. I have to admit that I'd probably make the same mistake
>> again if I was to do this for another driver and you hadn't sent
>> the fix. The hrtimer_set_expires_range() function just looked like
>> it had been designed for the use case I was interested in ;-).
>>
>> Any idea how to prevent the next person from making the same mistake?
>
> Yes. Documentation :)
>
Looks good to me, thanks everyone.
Ding
> Thanks,
>
> tglx
>
> .
>
From: Thomas Gleixner <[email protected]>
Date: Tue, 14 Apr 2015 00:08:23 +0200 (CEST)
> On Tue, 14 Apr 2015, Arnd Bergmann wrote:
>> On Monday 13 April 2015 23:42:03 Thomas Gleixner wrote:
>> > >
>> > > 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);
>>
>> Ah, good. I have to admit that I'd probably make the same mistake
>> again if I was to do this for another driver and you hadn't sent
>> the fix. The hrtimer_set_expires_range() function just looked like
>> it had been designed for the use case I was interested in ;-).
>>
>> Any idea how to prevent the next person from making the same mistake?
>
> Yes. Documentation :)
Can I get a respin of this patch with the above?
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 <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: dingtianhong <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Zhangfei Gao <[email protected]>
Cc: Dan Carpenter <[email protected]>
Cc: [email protected]
---
drivers/net/ethernet/hisilicon/hip04_eth.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
Index: linux/drivers/net/ethernet/hisilicon/hip04_eth.c
===================================================================
--- linux.orig/drivers/net/ethernet/hisilicon/hip04_eth.c
+++ linux/drivers/net/ethernet/hisilicon/hip04_eth.c
@@ -413,6 +413,15 @@ out:
return count;
}
+static void hip04_start_tx_timer(struct hip04_priv *priv)
+{
+ unsigned long ns = priv->tx_coalesce_usecs * NSEC_PER_USEC / 2;
+
+ /* allow timer to fire after half the time at the earliest */
+ hrtimer_start_range_ns(&priv->tx_coalesce_timer, ns_to_ktime(ns),
+ ns, HRTIMER_MODE_REL);
+}
+
static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
{
struct hip04_priv *priv = netdev_priv(ndev);
@@ -466,8 +475,7 @@ static int hip04_mac_start_xmit(struct s
}
} else if (!hrtimer_is_queued(&priv->tx_coalesce_timer)) {
/* cleanup not pending yet, start a new timer */
- hrtimer_start_expires(&priv->tx_coalesce_timer,
- HRTIMER_MODE_REL);
+ hip04_start_tx_timer(priv);
}
return NETDEV_TX_OK;
@@ -549,7 +557,7 @@ done:
/* clean up tx descriptors and start a new timer if necessary */
tx_remaining = hip04_tx_reclaim(ndev, false);
if (rx < budget && tx_remaining)
- hrtimer_start_expires(&priv->tx_coalesce_timer, HRTIMER_MODE_REL);
+ hip04_start_tx_timer(priv);
return rx;
}
@@ -809,7 +817,6 @@ static int hip04_mac_probe(struct platfo
struct hip04_priv *priv;
struct resource *res;
unsigned int irq;
- ktime_t txtime;
int ret;
ndev = alloc_etherdev(sizeof(struct hip04_priv));
@@ -846,9 +853,6 @@ static int hip04_mac_probe(struct platfo
*/
priv->tx_coalesce_frames = TX_DESC_NUM * 3 / 4;
priv->tx_coalesce_usecs = 200;
- /* allow timer to fire after half the time at the earliest */
- txtime = ktime_set(0, priv->tx_coalesce_usecs * NSEC_PER_USEC / 2);
- hrtimer_set_expires_range(&priv->tx_coalesce_timer, txtime, txtime);
priv->tx_coalesce_timer.function = tx_done;
priv->map = syscon_node_to_regmap(arg.np);
On 2015/4/15 3:42, 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.
>
Acked-by: Ding Tianhong <[email protected]>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: dingtianhong <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Zhangfei Gao <[email protected]>
> Cc: Dan Carpenter <[email protected]>
> Cc: [email protected]
> ---
> drivers/net/ethernet/hisilicon/hip04_eth.c | 18 +++++++++++-------
> 1 file changed, 11 insertions(+), 7 deletions(-)
>
> Index: linux/drivers/net/ethernet/hisilicon/hip04_eth.c
> ===================================================================
> --- linux.orig/drivers/net/ethernet/hisilicon/hip04_eth.c
> +++ linux/drivers/net/ethernet/hisilicon/hip04_eth.c
> @@ -413,6 +413,15 @@ out:
> return count;
> }
>
> +static void hip04_start_tx_timer(struct hip04_priv *priv)
> +{
> + unsigned long ns = priv->tx_coalesce_usecs * NSEC_PER_USEC / 2;
> +
> + /* allow timer to fire after half the time at the earliest */
> + hrtimer_start_range_ns(&priv->tx_coalesce_timer, ns_to_ktime(ns),
> + ns, HRTIMER_MODE_REL);
> +}
> +
> static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> {
> struct hip04_priv *priv = netdev_priv(ndev);
> @@ -466,8 +475,7 @@ static int hip04_mac_start_xmit(struct s
> }
> } else if (!hrtimer_is_queued(&priv->tx_coalesce_timer)) {
> /* cleanup not pending yet, start a new timer */
> - hrtimer_start_expires(&priv->tx_coalesce_timer,
> - HRTIMER_MODE_REL);
> + hip04_start_tx_timer(priv);
> }
>
> return NETDEV_TX_OK;
> @@ -549,7 +557,7 @@ done:
> /* clean up tx descriptors and start a new timer if necessary */
> tx_remaining = hip04_tx_reclaim(ndev, false);
> if (rx < budget && tx_remaining)
> - hrtimer_start_expires(&priv->tx_coalesce_timer, HRTIMER_MODE_REL);
> + hip04_start_tx_timer(priv);
>
> return rx;
> }
> @@ -809,7 +817,6 @@ static int hip04_mac_probe(struct platfo
> struct hip04_priv *priv;
> struct resource *res;
> unsigned int irq;
> - ktime_t txtime;
> int ret;
>
> ndev = alloc_etherdev(sizeof(struct hip04_priv));
> @@ -846,9 +853,6 @@ static int hip04_mac_probe(struct platfo
> */
> priv->tx_coalesce_frames = TX_DESC_NUM * 3 / 4;
> priv->tx_coalesce_usecs = 200;
> - /* allow timer to fire after half the time at the earliest */
> - txtime = ktime_set(0, priv->tx_coalesce_usecs * NSEC_PER_USEC / 2);
> - hrtimer_set_expires_range(&priv->tx_coalesce_timer, txtime, txtime);
> priv->tx_coalesce_timer.function = tx_done;
>
> priv->map = syscon_node_to_regmap(arg.np);
>
> .
>
On Tuesday 14 April 2015 21:42:42 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 <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: dingtianhong <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Zhangfei Gao <[email protected]>
> Cc: Dan Carpenter <[email protected]>
> Cc: [email protected]
>
Acked-by: Arnd Bergmann <[email protected]>
From: Thomas Gleixner <[email protected]>
Date: Tue, 14 Apr 2015 21:42:42 +0200 (CEST)
> 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 <[email protected]>
Applied, thanks Thomas.