* [PATCH net] xen-netback: use jiffies_64 value to calculate credit timeout
@ 2013-10-27 11:11 Wei Liu
2013-10-28 2:36 ` annie li
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Wei Liu @ 2013-10-27 11:11 UTC (permalink / raw)
To: xen-devel, netdev
Cc: david.vrabel, jbeulich, annie.li, Wei Liu, Ian Campbell, Jason Luan
time_after_eq() only works if the delta is < MAX_ULONG/2.
For a 32bit Dom0, if netfront sends packets at a very low rate, the time
between subsequent calls to tx_credit_exceeded() may exceed MAX_ULONG/2
and the test for timer_after_eq() will be incorrect. Credit will not be
replenished and the guest may become unable to send packets (e.g., if
prior to the long gap, all credit was exhausted).
Use jiffies_64 variant to mitigate this problem for 32bit Dom0.
Suggested-by: Jan Beulich <jbeulich@suse.com>
Suggested-by: David Vrabel <david.vrabel@citrix.com>
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Jason Luan <jianhai.luan@oracle.com>
---
drivers/net/xen-netback/common.h | 1 +
drivers/net/xen-netback/interface.c | 4 ++--
drivers/net/xen-netback/netback.c | 13 ++++++-------
3 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
index 5715318..400fea1 100644
--- a/drivers/net/xen-netback/common.h
+++ b/drivers/net/xen-netback/common.h
@@ -163,6 +163,7 @@ struct xenvif {
unsigned long credit_usec;
unsigned long remaining_credit;
struct timer_list credit_timeout;
+ u64 credit_window_start;
/* Statistics */
unsigned long rx_gso_checksum_fixup;
diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
index 01bb854..8c45b63 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c
@@ -312,8 +312,8 @@ struct xenvif *xenvif_alloc(struct device *parent, domid_t domid,
vif->credit_bytes = vif->remaining_credit = ~0UL;
vif->credit_usec = 0UL;
init_timer(&vif->credit_timeout);
- /* Initialize 'expires' now: it's used to track the credit window. */
- vif->credit_timeout.expires = jiffies;
+ /* credit window is tracked in credit_window_start */
+ vif->credit_window_start = get_jiffies_64();
dev->netdev_ops = &xenvif_netdev_ops;
dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO;
diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index f3e591c..1bc0688 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -1185,18 +1185,17 @@ out:
static bool tx_credit_exceeded(struct xenvif *vif, unsigned size)
{
- unsigned long now = jiffies;
- unsigned long next_credit =
- vif->credit_timeout.expires +
- msecs_to_jiffies(vif->credit_usec / 1000);
+ u64 now = get_jiffies_64();
+ u64 next_credit = vif->credit_window_start +
+ (u64)msecs_to_jiffies(vif->credit_usec / 1000);
/* Timer could already be pending in rare cases. */
if (timer_pending(&vif->credit_timeout))
return true;
/* Passed the point where we can replenish credit? */
- if (time_after_eq(now, next_credit)) {
- vif->credit_timeout.expires = now;
+ if (time_after_eq64(now, next_credit)) {
+ vif->credit_timeout.expires = (unsigned long)now;
tx_add_credit(vif);
}
@@ -1207,7 +1206,7 @@ static bool tx_credit_exceeded(struct xenvif *vif, unsigned size)
vif->credit_timeout.function =
tx_credit_callback;
mod_timer(&vif->credit_timeout,
- next_credit);
+ (unsigned long)next_credit);
return true;
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH net] xen-netback: use jiffies_64 value to calculate credit timeout
2013-10-27 11:11 [PATCH net] xen-netback: use jiffies_64 value to calculate credit timeout Wei Liu
@ 2013-10-28 2:36 ` annie li
2013-10-28 2:36 ` annie li
2013-10-28 8:21 ` David Vrabel
2 siblings, 0 replies; 13+ messages in thread
From: annie li @ 2013-10-28 2:36 UTC (permalink / raw)
To: Wei Liu
Cc: Ian Campbell, netdev, xen-devel, david.vrabel, jbeulich, Jason Luan
On 2013-10-27 19:11, Wei Liu wrote:
> time_after_eq() only works if the delta is < MAX_ULONG/2.
>
> For a 32bit Dom0, if netfront sends packets at a very low rate, the time
> between subsequent calls to tx_credit_exceeded() may exceed MAX_ULONG/2
> and the test for timer_after_eq() will be incorrect. Credit will not be
> replenished and the guest may become unable to send packets (e.g., if
> prior to the long gap, all credit was exhausted).
>
> Use jiffies_64 variant to mitigate this problem for 32bit Dom0.
>
> Suggested-by: Jan Beulich <jbeulich@suse.com>
> Suggested-by: David Vrabel <david.vrabel@citrix.com>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Jason Luan <jianhai.luan@oracle.com>
> ---
> drivers/net/xen-netback/common.h | 1 +
> drivers/net/xen-netback/interface.c | 4 ++--
> drivers/net/xen-netback/netback.c | 13 ++++++-------
> 3 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
> index 5715318..400fea1 100644
> --- a/drivers/net/xen-netback/common.h
> +++ b/drivers/net/xen-netback/common.h
> @@ -163,6 +163,7 @@ struct xenvif {
> unsigned long credit_usec;
> unsigned long remaining_credit;
> struct timer_list credit_timeout;
> + u64 credit_window_start;
>
> /* Statistics */
> unsigned long rx_gso_checksum_fixup;
> diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
> index 01bb854..8c45b63 100644
> --- a/drivers/net/xen-netback/interface.c
> +++ b/drivers/net/xen-netback/interface.c
> @@ -312,8 +312,8 @@ struct xenvif *xenvif_alloc(struct device *parent, domid_t domid,
> vif->credit_bytes = vif->remaining_credit = ~0UL;
> vif->credit_usec = 0UL;
> init_timer(&vif->credit_timeout);
> - /* Initialize 'expires' now: it's used to track the credit window. */
> - vif->credit_timeout.expires = jiffies;
> + /* credit window is tracked in credit_window_start */
> + vif->credit_window_start = get_jiffies_64();
>
> dev->netdev_ops = &xenvif_netdev_ops;
> dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO;
> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index f3e591c..1bc0688 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -1185,18 +1185,17 @@ out:
>
> static bool tx_credit_exceeded(struct xenvif *vif, unsigned size)
> {
> - unsigned long now = jiffies;
> - unsigned long next_credit =
> - vif->credit_timeout.expires +
> - msecs_to_jiffies(vif->credit_usec / 1000);
> + u64 now = get_jiffies_64();
> + u64 next_credit = vif->credit_window_start +
> + (u64)msecs_to_jiffies(vif->credit_usec / 1000);
You simply replace "credit_timeout.expires" with
"vif->credit_window_start" here, and never update
"vif->credit_window_start" in following code.
>
> /* Timer could already be pending in rare cases. */
> if (timer_pending(&vif->credit_timeout))
> return true;
>
> /* Passed the point where we can replenish credit? */
> - if (time_after_eq(now, next_credit)) {
> - vif->credit_timeout.expires = now;
> + if (time_after_eq64(now, next_credit)) {
> + vif->credit_timeout.expires = (unsigned long)now;
updates credit_window_start as following,
vif->credit_window_start = (unsigned long)now;
> tx_add_credit(vif);
> }
>
> @@ -1207,7 +1206,7 @@ static bool tx_credit_exceeded(struct xenvif *vif, unsigned size)
> vif->credit_timeout.function =
> tx_credit_callback;
> mod_timer(&vif->credit_timeout,
> - next_credit);
> + (unsigned long)next_credit);
vif->credit_timeout.expires is unsigned long, and this still causes
original issue on 32bit system, which works well on 64bit. Or rewriting
code to avoid uses of vif->credit_timeout.expires, but it is complex.
BTW, I prefer Luan's patch which is simple and clear.
Thanks
Annie
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net] xen-netback: use jiffies_64 value to calculate credit timeout
2013-10-27 11:11 [PATCH net] xen-netback: use jiffies_64 value to calculate credit timeout Wei Liu
2013-10-28 2:36 ` annie li
@ 2013-10-28 2:36 ` annie li
2013-10-28 2:58 ` annie li
2013-10-28 2:58 ` [Xen-devel] " annie li
2013-10-28 8:21 ` David Vrabel
2 siblings, 2 replies; 13+ messages in thread
From: annie li @ 2013-10-28 2:36 UTC (permalink / raw)
To: Wei Liu
Cc: xen-devel, netdev, david.vrabel, jbeulich, Ian Campbell, Jason Luan
On 2013-10-27 19:11, Wei Liu wrote:
> time_after_eq() only works if the delta is < MAX_ULONG/2.
>
> For a 32bit Dom0, if netfront sends packets at a very low rate, the time
> between subsequent calls to tx_credit_exceeded() may exceed MAX_ULONG/2
> and the test for timer_after_eq() will be incorrect. Credit will not be
> replenished and the guest may become unable to send packets (e.g., if
> prior to the long gap, all credit was exhausted).
>
> Use jiffies_64 variant to mitigate this problem for 32bit Dom0.
>
> Suggested-by: Jan Beulich <jbeulich@suse.com>
> Suggested-by: David Vrabel <david.vrabel@citrix.com>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Jason Luan <jianhai.luan@oracle.com>
> ---
> drivers/net/xen-netback/common.h | 1 +
> drivers/net/xen-netback/interface.c | 4 ++--
> drivers/net/xen-netback/netback.c | 13 ++++++-------
> 3 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
> index 5715318..400fea1 100644
> --- a/drivers/net/xen-netback/common.h
> +++ b/drivers/net/xen-netback/common.h
> @@ -163,6 +163,7 @@ struct xenvif {
> unsigned long credit_usec;
> unsigned long remaining_credit;
> struct timer_list credit_timeout;
> + u64 credit_window_start;
>
> /* Statistics */
> unsigned long rx_gso_checksum_fixup;
> diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
> index 01bb854..8c45b63 100644
> --- a/drivers/net/xen-netback/interface.c
> +++ b/drivers/net/xen-netback/interface.c
> @@ -312,8 +312,8 @@ struct xenvif *xenvif_alloc(struct device *parent, domid_t domid,
> vif->credit_bytes = vif->remaining_credit = ~0UL;
> vif->credit_usec = 0UL;
> init_timer(&vif->credit_timeout);
> - /* Initialize 'expires' now: it's used to track the credit window. */
> - vif->credit_timeout.expires = jiffies;
> + /* credit window is tracked in credit_window_start */
> + vif->credit_window_start = get_jiffies_64();
>
> dev->netdev_ops = &xenvif_netdev_ops;
> dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO;
> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index f3e591c..1bc0688 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -1185,18 +1185,17 @@ out:
>
> static bool tx_credit_exceeded(struct xenvif *vif, unsigned size)
> {
> - unsigned long now = jiffies;
> - unsigned long next_credit =
> - vif->credit_timeout.expires +
> - msecs_to_jiffies(vif->credit_usec / 1000);
> + u64 now = get_jiffies_64();
> + u64 next_credit = vif->credit_window_start +
> + (u64)msecs_to_jiffies(vif->credit_usec / 1000);
You simply replace "credit_timeout.expires" with
"vif->credit_window_start" here, and never update
"vif->credit_window_start" in following code.
>
> /* Timer could already be pending in rare cases. */
> if (timer_pending(&vif->credit_timeout))
> return true;
>
> /* Passed the point where we can replenish credit? */
> - if (time_after_eq(now, next_credit)) {
> - vif->credit_timeout.expires = now;
> + if (time_after_eq64(now, next_credit)) {
> + vif->credit_timeout.expires = (unsigned long)now;
updates credit_window_start as following,
vif->credit_window_start = (unsigned long)now;
> tx_add_credit(vif);
> }
>
> @@ -1207,7 +1206,7 @@ static bool tx_credit_exceeded(struct xenvif *vif, unsigned size)
> vif->credit_timeout.function =
> tx_credit_callback;
> mod_timer(&vif->credit_timeout,
> - next_credit);
> + (unsigned long)next_credit);
vif->credit_timeout.expires is unsigned long, and this still causes
original issue on 32bit system, which works well on 64bit. Or rewriting
code to avoid uses of vif->credit_timeout.expires, but it is complex.
BTW, I prefer Luan's patch which is simple and clear.
Thanks
Annie
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net] xen-netback: use jiffies_64 value to calculate credit timeout
2013-10-28 2:36 ` annie li
@ 2013-10-28 2:58 ` annie li
2013-10-28 2:58 ` [Xen-devel] " annie li
1 sibling, 0 replies; 13+ messages in thread
From: annie li @ 2013-10-28 2:58 UTC (permalink / raw)
To: Wei Liu
Cc: Ian Campbell, netdev, xen-devel, david.vrabel, jbeulich, Jason Luan
On 2013-10-28 10:36, annie li wrote:
>
> On 2013-10-27 19:11, Wei Liu wrote:
>> time_after_eq() only works if the delta is < MAX_ULONG/2.
>>
>> For a 32bit Dom0, if netfront sends packets at a very low rate, the time
>> between subsequent calls to tx_credit_exceeded() may exceed MAX_ULONG/2
>> and the test for timer_after_eq() will be incorrect. Credit will not be
>> replenished and the guest may become unable to send packets (e.g., if
>> prior to the long gap, all credit was exhausted).
>>
>> Use jiffies_64 variant to mitigate this problem for 32bit Dom0.
>>
>> Suggested-by: Jan Beulich <jbeulich@suse.com>
>> Suggested-by: David Vrabel <david.vrabel@citrix.com>
>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>> Cc: Ian Campbell <ian.campbell@citrix.com>
>> Cc: Jason Luan <jianhai.luan@oracle.com>
>> ---
>> drivers/net/xen-netback/common.h | 1 +
>> drivers/net/xen-netback/interface.c | 4 ++--
>> drivers/net/xen-netback/netback.c | 13 ++++++-------
>> 3 files changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/net/xen-netback/common.h
>> b/drivers/net/xen-netback/common.h
>> index 5715318..400fea1 100644
>> --- a/drivers/net/xen-netback/common.h
>> +++ b/drivers/net/xen-netback/common.h
>> @@ -163,6 +163,7 @@ struct xenvif {
>> unsigned long credit_usec;
>> unsigned long remaining_credit;
>> struct timer_list credit_timeout;
>> + u64 credit_window_start;
>> /* Statistics */
>> unsigned long rx_gso_checksum_fixup;
>> diff --git a/drivers/net/xen-netback/interface.c
>> b/drivers/net/xen-netback/interface.c
>> index 01bb854..8c45b63 100644
>> --- a/drivers/net/xen-netback/interface.c
>> +++ b/drivers/net/xen-netback/interface.c
>> @@ -312,8 +312,8 @@ struct xenvif *xenvif_alloc(struct device
>> *parent, domid_t domid,
>> vif->credit_bytes = vif->remaining_credit = ~0UL;
>> vif->credit_usec = 0UL;
>> init_timer(&vif->credit_timeout);
>> - /* Initialize 'expires' now: it's used to track the credit
>> window. */
>> - vif->credit_timeout.expires = jiffies;
>> + /* credit window is tracked in credit_window_start */
>> + vif->credit_window_start = get_jiffies_64();
>> dev->netdev_ops = &xenvif_netdev_ops;
>> dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO;
>> diff --git a/drivers/net/xen-netback/netback.c
>> b/drivers/net/xen-netback/netback.c
>> index f3e591c..1bc0688 100644
>> --- a/drivers/net/xen-netback/netback.c
>> +++ b/drivers/net/xen-netback/netback.c
>> @@ -1185,18 +1185,17 @@ out:
>> static bool tx_credit_exceeded(struct xenvif *vif, unsigned size)
>> {
>> - unsigned long now = jiffies;
>> - unsigned long next_credit =
>> - vif->credit_timeout.expires +
>> - msecs_to_jiffies(vif->credit_usec / 1000);
>> + u64 now = get_jiffies_64();
>> + u64 next_credit = vif->credit_window_start +
>> + (u64)msecs_to_jiffies(vif->credit_usec / 1000);
>
> You simply replace "credit_timeout.expires" with
> "vif->credit_window_start" here, and never update
> "vif->credit_window_start" in following code.
>
>> /* Timer could already be pending in rare cases. */
>> if (timer_pending(&vif->credit_timeout))
>> return true;
>> /* Passed the point where we can replenish credit? */
>> - if (time_after_eq(now, next_credit)) {
>> - vif->credit_timeout.expires = now;
>> + if (time_after_eq64(now, next_credit)) {
>> + vif->credit_timeout.expires = (unsigned long)now;
>
> updates credit_window_start as following,
> vif->credit_window_start = (unsigned long)now;
both credit_window_start and credit_timeout.expires need to be updated
here,
vif->credit_window_start = (unsigned long)now;
vif->credit_timeout.expires = (unsigned long)now;
>
>> tx_add_credit(vif);
>> }
>> @@ -1207,7 +1206,7 @@ static bool tx_credit_exceeded(struct xenvif
>> *vif, unsigned size)
>> vif->credit_timeout.function =
>> tx_credit_callback;
>> mod_timer(&vif->credit_timeout,
>> - next_credit);
>> + (unsigned long)next_credit);
>
> vif->credit_timeout.expires is unsigned long, and this still causes
> original issue on 32bit system, which works well on 64bit. Or
> rewriting code to avoid uses of vif->credit_timeout.expires, but it is
> complex.
My understanding here is wrong, please ignore this.
Thanks
Annie
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Xen-devel] [PATCH net] xen-netback: use jiffies_64 value to calculate credit timeout
2013-10-28 2:36 ` annie li
2013-10-28 2:58 ` annie li
@ 2013-10-28 2:58 ` annie li
2013-10-28 6:28 ` jianhai luan
` (3 more replies)
1 sibling, 4 replies; 13+ messages in thread
From: annie li @ 2013-10-28 2:58 UTC (permalink / raw)
To: Wei Liu
Cc: Ian Campbell, netdev, xen-devel, david.vrabel, jbeulich, Jason Luan
On 2013-10-28 10:36, annie li wrote:
>
> On 2013-10-27 19:11, Wei Liu wrote:
>> time_after_eq() only works if the delta is < MAX_ULONG/2.
>>
>> For a 32bit Dom0, if netfront sends packets at a very low rate, the time
>> between subsequent calls to tx_credit_exceeded() may exceed MAX_ULONG/2
>> and the test for timer_after_eq() will be incorrect. Credit will not be
>> replenished and the guest may become unable to send packets (e.g., if
>> prior to the long gap, all credit was exhausted).
>>
>> Use jiffies_64 variant to mitigate this problem for 32bit Dom0.
>>
>> Suggested-by: Jan Beulich <jbeulich@suse.com>
>> Suggested-by: David Vrabel <david.vrabel@citrix.com>
>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>> Cc: Ian Campbell <ian.campbell@citrix.com>
>> Cc: Jason Luan <jianhai.luan@oracle.com>
>> ---
>> drivers/net/xen-netback/common.h | 1 +
>> drivers/net/xen-netback/interface.c | 4 ++--
>> drivers/net/xen-netback/netback.c | 13 ++++++-------
>> 3 files changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/net/xen-netback/common.h
>> b/drivers/net/xen-netback/common.h
>> index 5715318..400fea1 100644
>> --- a/drivers/net/xen-netback/common.h
>> +++ b/drivers/net/xen-netback/common.h
>> @@ -163,6 +163,7 @@ struct xenvif {
>> unsigned long credit_usec;
>> unsigned long remaining_credit;
>> struct timer_list credit_timeout;
>> + u64 credit_window_start;
>> /* Statistics */
>> unsigned long rx_gso_checksum_fixup;
>> diff --git a/drivers/net/xen-netback/interface.c
>> b/drivers/net/xen-netback/interface.c
>> index 01bb854..8c45b63 100644
>> --- a/drivers/net/xen-netback/interface.c
>> +++ b/drivers/net/xen-netback/interface.c
>> @@ -312,8 +312,8 @@ struct xenvif *xenvif_alloc(struct device
>> *parent, domid_t domid,
>> vif->credit_bytes = vif->remaining_credit = ~0UL;
>> vif->credit_usec = 0UL;
>> init_timer(&vif->credit_timeout);
>> - /* Initialize 'expires' now: it's used to track the credit
>> window. */
>> - vif->credit_timeout.expires = jiffies;
>> + /* credit window is tracked in credit_window_start */
>> + vif->credit_window_start = get_jiffies_64();
>> dev->netdev_ops = &xenvif_netdev_ops;
>> dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO;
>> diff --git a/drivers/net/xen-netback/netback.c
>> b/drivers/net/xen-netback/netback.c
>> index f3e591c..1bc0688 100644
>> --- a/drivers/net/xen-netback/netback.c
>> +++ b/drivers/net/xen-netback/netback.c
>> @@ -1185,18 +1185,17 @@ out:
>> static bool tx_credit_exceeded(struct xenvif *vif, unsigned size)
>> {
>> - unsigned long now = jiffies;
>> - unsigned long next_credit =
>> - vif->credit_timeout.expires +
>> - msecs_to_jiffies(vif->credit_usec / 1000);
>> + u64 now = get_jiffies_64();
>> + u64 next_credit = vif->credit_window_start +
>> + (u64)msecs_to_jiffies(vif->credit_usec / 1000);
>
> You simply replace "credit_timeout.expires" with
> "vif->credit_window_start" here, and never update
> "vif->credit_window_start" in following code.
>
>> /* Timer could already be pending in rare cases. */
>> if (timer_pending(&vif->credit_timeout))
>> return true;
>> /* Passed the point where we can replenish credit? */
>> - if (time_after_eq(now, next_credit)) {
>> - vif->credit_timeout.expires = now;
>> + if (time_after_eq64(now, next_credit)) {
>> + vif->credit_timeout.expires = (unsigned long)now;
>
> updates credit_window_start as following,
> vif->credit_window_start = (unsigned long)now;
both credit_window_start and credit_timeout.expires need to be updated
here,
vif->credit_window_start = (unsigned long)now;
vif->credit_timeout.expires = (unsigned long)now;
>
>> tx_add_credit(vif);
>> }
>> @@ -1207,7 +1206,7 @@ static bool tx_credit_exceeded(struct xenvif
>> *vif, unsigned size)
>> vif->credit_timeout.function =
>> tx_credit_callback;
>> mod_timer(&vif->credit_timeout,
>> - next_credit);
>> + (unsigned long)next_credit);
>
> vif->credit_timeout.expires is unsigned long, and this still causes
> original issue on 32bit system, which works well on 64bit. Or
> rewriting code to avoid uses of vif->credit_timeout.expires, but it is
> complex.
My understanding here is wrong, please ignore this.
Thanks
Annie
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Xen-devel] [PATCH net] xen-netback: use jiffies_64 value to calculate credit timeout
2013-10-28 2:58 ` [Xen-devel] " annie li
@ 2013-10-28 6:28 ` jianhai luan
2013-10-28 6:28 ` jianhai luan
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: jianhai luan @ 2013-10-28 6:28 UTC (permalink / raw)
To: annie li, Wei Liu; +Cc: Ian Campbell, netdev, xen-devel, david.vrabel, jbeulich
On 2013-10-28 10:58, annie li wrote:
>
> On 2013-10-28 10:36, annie li wrote:
>>
>> On 2013-10-27 19:11, Wei Liu wrote:
>>> time_after_eq() only works if the delta is < MAX_ULONG/2.
>>>
>>> For a 32bit Dom0, if netfront sends packets at a very low rate, the
>>> time
>>> between subsequent calls to tx_credit_exceeded() may exceed MAX_ULONG/2
>>> and the test for timer_after_eq() will be incorrect. Credit will not be
>>> replenished and the guest may become unable to send packets (e.g., if
>>> prior to the long gap, all credit was exhausted).
>>>
>>> Use jiffies_64 variant to mitigate this problem for 32bit Dom0.
>>>
>>> Suggested-by: Jan Beulich <jbeulich@suse.com>
>>> Suggested-by: David Vrabel <david.vrabel@citrix.com>
>>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>>> Cc: Ian Campbell <ian.campbell@citrix.com>
>>> Cc: Jason Luan <jianhai.luan@oracle.com>
>>> ---
>>> drivers/net/xen-netback/common.h | 1 +
>>> drivers/net/xen-netback/interface.c | 4 ++--
>>> drivers/net/xen-netback/netback.c | 13 ++++++-------
>>> 3 files changed, 9 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/net/xen-netback/common.h
>>> b/drivers/net/xen-netback/common.h
>>> index 5715318..400fea1 100644
>>> --- a/drivers/net/xen-netback/common.h
>>> +++ b/drivers/net/xen-netback/common.h
>>> @@ -163,6 +163,7 @@ struct xenvif {
>>> unsigned long credit_usec;
>>> unsigned long remaining_credit;
>>> struct timer_list credit_timeout;
>>> + u64 credit_window_start;
>>> /* Statistics */
>>> unsigned long rx_gso_checksum_fixup;
>>> diff --git a/drivers/net/xen-netback/interface.c
>>> b/drivers/net/xen-netback/interface.c
>>> index 01bb854..8c45b63 100644
>>> --- a/drivers/net/xen-netback/interface.c
>>> +++ b/drivers/net/xen-netback/interface.c
>>> @@ -312,8 +312,8 @@ struct xenvif *xenvif_alloc(struct device
>>> *parent, domid_t domid,
>>> vif->credit_bytes = vif->remaining_credit = ~0UL;
>>> vif->credit_usec = 0UL;
>>> init_timer(&vif->credit_timeout);
>>> - /* Initialize 'expires' now: it's used to track the credit
>>> window. */
>>> - vif->credit_timeout.expires = jiffies;
>>> + /* credit window is tracked in credit_window_start */
>>> + vif->credit_window_start = get_jiffies_64();
>>> dev->netdev_ops = &xenvif_netdev_ops;
>>> dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO;
>>> diff --git a/drivers/net/xen-netback/netback.c
>>> b/drivers/net/xen-netback/netback.c
>>> index f3e591c..1bc0688 100644
>>> --- a/drivers/net/xen-netback/netback.c
>>> +++ b/drivers/net/xen-netback/netback.c
>>> @@ -1185,18 +1185,17 @@ out:
>>> static bool tx_credit_exceeded(struct xenvif *vif, unsigned size)
>>> {
>>> - unsigned long now = jiffies;
>>> - unsigned long next_credit =
>>> - vif->credit_timeout.expires +
>>> - msecs_to_jiffies(vif->credit_usec / 1000);
>>> + u64 now = get_jiffies_64();
>>> + u64 next_credit = vif->credit_window_start +
>>> + (u64)msecs_to_jiffies(vif->credit_usec / 1000);
>>
>> You simply replace "credit_timeout.expires" with
>> "vif->credit_window_start" here, and never update
>> "vif->credit_window_start" in following code.
>>
>>> /* Timer could already be pending in rare cases. */
>>> if (timer_pending(&vif->credit_timeout))
>>> return true;
>>> /* Passed the point where we can replenish credit? */
>>> - if (time_after_eq(now, next_credit)) {
>>> - vif->credit_timeout.expires = now;
>>> + if (time_after_eq64(now, next_credit)) {
>>> + vif->credit_timeout.expires = (unsigned long)now;
>>
>> updates credit_window_start as following,
>> vif->credit_window_start = (unsigned long)now;
>
> both credit_window_start and credit_timeout.expires need to be updated
> here,
>
> vif->credit_window_start = (unsigned long)now;
> vif->credit_timeout.expires = (unsigned long)now;
Annie should be correct.
It is good to me but i afraid that get_jiffies_64() and
time_after_eq64() will bring some performance's load when transmit is
busy in 32bit. I will do some test about the patch.
No matter what, i prefer own patch.
Thanks,
Jason
>
>>
>>> tx_add_credit(vif);
>>> }
>>> @@ -1207,7 +1206,7 @@ static bool tx_credit_exceeded(struct xenvif
>>> *vif, unsigned size)
>>> vif->credit_timeout.function =
>>> tx_credit_callback;
>>> mod_timer(&vif->credit_timeout,
>>> - next_credit);
>>> + (unsigned long)next_credit);
>>
>> vif->credit_timeout.expires is unsigned long, and this still causes
>> original issue on 32bit system, which works well on 64bit. Or
>> rewriting code to avoid uses of vif->credit_timeout.expires, but it
>> is complex.
> My understanding here is wrong, please ignore this.
>
> Thanks
> Annie
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net] xen-netback: use jiffies_64 value to calculate credit timeout
2013-10-28 2:58 ` [Xen-devel] " annie li
2013-10-28 6:28 ` jianhai luan
@ 2013-10-28 6:28 ` jianhai luan
2013-10-28 11:30 ` [Xen-devel] " Wei Liu
2013-10-28 11:30 ` Wei Liu
3 siblings, 0 replies; 13+ messages in thread
From: jianhai luan @ 2013-10-28 6:28 UTC (permalink / raw)
To: annie li, Wei Liu; +Cc: netdev, david.vrabel, Ian Campbell, jbeulich, xen-devel
On 2013-10-28 10:58, annie li wrote:
>
> On 2013-10-28 10:36, annie li wrote:
>>
>> On 2013-10-27 19:11, Wei Liu wrote:
>>> time_after_eq() only works if the delta is < MAX_ULONG/2.
>>>
>>> For a 32bit Dom0, if netfront sends packets at a very low rate, the
>>> time
>>> between subsequent calls to tx_credit_exceeded() may exceed MAX_ULONG/2
>>> and the test for timer_after_eq() will be incorrect. Credit will not be
>>> replenished and the guest may become unable to send packets (e.g., if
>>> prior to the long gap, all credit was exhausted).
>>>
>>> Use jiffies_64 variant to mitigate this problem for 32bit Dom0.
>>>
>>> Suggested-by: Jan Beulich <jbeulich@suse.com>
>>> Suggested-by: David Vrabel <david.vrabel@citrix.com>
>>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>>> Cc: Ian Campbell <ian.campbell@citrix.com>
>>> Cc: Jason Luan <jianhai.luan@oracle.com>
>>> ---
>>> drivers/net/xen-netback/common.h | 1 +
>>> drivers/net/xen-netback/interface.c | 4 ++--
>>> drivers/net/xen-netback/netback.c | 13 ++++++-------
>>> 3 files changed, 9 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/net/xen-netback/common.h
>>> b/drivers/net/xen-netback/common.h
>>> index 5715318..400fea1 100644
>>> --- a/drivers/net/xen-netback/common.h
>>> +++ b/drivers/net/xen-netback/common.h
>>> @@ -163,6 +163,7 @@ struct xenvif {
>>> unsigned long credit_usec;
>>> unsigned long remaining_credit;
>>> struct timer_list credit_timeout;
>>> + u64 credit_window_start;
>>> /* Statistics */
>>> unsigned long rx_gso_checksum_fixup;
>>> diff --git a/drivers/net/xen-netback/interface.c
>>> b/drivers/net/xen-netback/interface.c
>>> index 01bb854..8c45b63 100644
>>> --- a/drivers/net/xen-netback/interface.c
>>> +++ b/drivers/net/xen-netback/interface.c
>>> @@ -312,8 +312,8 @@ struct xenvif *xenvif_alloc(struct device
>>> *parent, domid_t domid,
>>> vif->credit_bytes = vif->remaining_credit = ~0UL;
>>> vif->credit_usec = 0UL;
>>> init_timer(&vif->credit_timeout);
>>> - /* Initialize 'expires' now: it's used to track the credit
>>> window. */
>>> - vif->credit_timeout.expires = jiffies;
>>> + /* credit window is tracked in credit_window_start */
>>> + vif->credit_window_start = get_jiffies_64();
>>> dev->netdev_ops = &xenvif_netdev_ops;
>>> dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO;
>>> diff --git a/drivers/net/xen-netback/netback.c
>>> b/drivers/net/xen-netback/netback.c
>>> index f3e591c..1bc0688 100644
>>> --- a/drivers/net/xen-netback/netback.c
>>> +++ b/drivers/net/xen-netback/netback.c
>>> @@ -1185,18 +1185,17 @@ out:
>>> static bool tx_credit_exceeded(struct xenvif *vif, unsigned size)
>>> {
>>> - unsigned long now = jiffies;
>>> - unsigned long next_credit =
>>> - vif->credit_timeout.expires +
>>> - msecs_to_jiffies(vif->credit_usec / 1000);
>>> + u64 now = get_jiffies_64();
>>> + u64 next_credit = vif->credit_window_start +
>>> + (u64)msecs_to_jiffies(vif->credit_usec / 1000);
>>
>> You simply replace "credit_timeout.expires" with
>> "vif->credit_window_start" here, and never update
>> "vif->credit_window_start" in following code.
>>
>>> /* Timer could already be pending in rare cases. */
>>> if (timer_pending(&vif->credit_timeout))
>>> return true;
>>> /* Passed the point where we can replenish credit? */
>>> - if (time_after_eq(now, next_credit)) {
>>> - vif->credit_timeout.expires = now;
>>> + if (time_after_eq64(now, next_credit)) {
>>> + vif->credit_timeout.expires = (unsigned long)now;
>>
>> updates credit_window_start as following,
>> vif->credit_window_start = (unsigned long)now;
>
> both credit_window_start and credit_timeout.expires need to be updated
> here,
>
> vif->credit_window_start = (unsigned long)now;
> vif->credit_timeout.expires = (unsigned long)now;
Annie should be correct.
It is good to me but i afraid that get_jiffies_64() and
time_after_eq64() will bring some performance's load when transmit is
busy in 32bit. I will do some test about the patch.
No matter what, i prefer own patch.
Thanks,
Jason
>
>>
>>> tx_add_credit(vif);
>>> }
>>> @@ -1207,7 +1206,7 @@ static bool tx_credit_exceeded(struct xenvif
>>> *vif, unsigned size)
>>> vif->credit_timeout.function =
>>> tx_credit_callback;
>>> mod_timer(&vif->credit_timeout,
>>> - next_credit);
>>> + (unsigned long)next_credit);
>>
>> vif->credit_timeout.expires is unsigned long, and this still causes
>> original issue on 32bit system, which works well on 64bit. Or
>> rewriting code to avoid uses of vif->credit_timeout.expires, but it
>> is complex.
> My understanding here is wrong, please ignore this.
>
> Thanks
> Annie
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Xen-devel] [PATCH net] xen-netback: use jiffies_64 value to calculate credit timeout
2013-10-28 2:58 ` [Xen-devel] " annie li
2013-10-28 6:28 ` jianhai luan
2013-10-28 6:28 ` jianhai luan
@ 2013-10-28 11:30 ` Wei Liu
2013-10-28 11:30 ` Wei Liu
3 siblings, 0 replies; 13+ messages in thread
From: Wei Liu @ 2013-10-28 11:30 UTC (permalink / raw)
To: annie li
Cc: Wei Liu, Ian Campbell, netdev, xen-devel, david.vrabel, jbeulich,
Jason Luan
On Mon, Oct 28, 2013 at 10:58:32AM +0800, annie li wrote:
[...]
> >> if (timer_pending(&vif->credit_timeout))
> >> return true;
> >> /* Passed the point where we can replenish credit? */
> >>- if (time_after_eq(now, next_credit)) {
> >>- vif->credit_timeout.expires = now;
> >>+ if (time_after_eq64(now, next_credit)) {
> >>+ vif->credit_timeout.expires = (unsigned long)now;
> >
> >updates credit_window_start as following,
> >vif->credit_window_start = (unsigned long)now;
>
> both credit_window_start and credit_timeout.expires need to be
> updated here,
>
> vif->credit_window_start = (unsigned long)now;
> vif->credit_timeout.expires = (unsigned long)now;
>
IMHO we don't need to update .expires anymore -- we now track the window
with another variable.
Wei.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net] xen-netback: use jiffies_64 value to calculate credit timeout
2013-10-28 2:58 ` [Xen-devel] " annie li
` (2 preceding siblings ...)
2013-10-28 11:30 ` [Xen-devel] " Wei Liu
@ 2013-10-28 11:30 ` Wei Liu
3 siblings, 0 replies; 13+ messages in thread
From: Wei Liu @ 2013-10-28 11:30 UTC (permalink / raw)
To: annie li
Cc: Wei Liu, Ian Campbell, netdev, xen-devel, david.vrabel, jbeulich,
Jason Luan
On Mon, Oct 28, 2013 at 10:58:32AM +0800, annie li wrote:
[...]
> >> if (timer_pending(&vif->credit_timeout))
> >> return true;
> >> /* Passed the point where we can replenish credit? */
> >>- if (time_after_eq(now, next_credit)) {
> >>- vif->credit_timeout.expires = now;
> >>+ if (time_after_eq64(now, next_credit)) {
> >>+ vif->credit_timeout.expires = (unsigned long)now;
> >
> >updates credit_window_start as following,
> >vif->credit_window_start = (unsigned long)now;
>
> both credit_window_start and credit_timeout.expires need to be
> updated here,
>
> vif->credit_window_start = (unsigned long)now;
> vif->credit_timeout.expires = (unsigned long)now;
>
IMHO we don't need to update .expires anymore -- we now track the window
with another variable.
Wei.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net] xen-netback: use jiffies_64 value to calculate credit timeout
2013-10-27 11:11 [PATCH net] xen-netback: use jiffies_64 value to calculate credit timeout Wei Liu
2013-10-28 2:36 ` annie li
2013-10-28 2:36 ` annie li
@ 2013-10-28 8:21 ` David Vrabel
2013-10-28 8:36 ` [Xen-devel] " jianhai luan
2013-10-28 8:36 ` jianhai luan
2 siblings, 2 replies; 13+ messages in thread
From: David Vrabel @ 2013-10-28 8:21 UTC (permalink / raw)
To: Wei Liu, xen-devel, netdev
Cc: Jason Luan, annie.li, Ian Campbell, jbeulich, david.vrabel
On 27/10/2013 11:11, Wei Liu wrote:
> time_after_eq() only works if the delta is < MAX_ULONG/2.
>
> For a 32bit Dom0, if netfront sends packets at a very low rate, the time
> between subsequent calls to tx_credit_exceeded() may exceed MAX_ULONG/2
> and the test for timer_after_eq() will be incorrect. Credit will not be
> replenished and the guest may become unable to send packets (e.g., if
> prior to the long gap, all credit was exhausted).
>
> Use jiffies_64 variant to mitigate this problem for 32bit Dom0.
Thanks.
> Suggested-by: Jan Beulich <jbeulich@suse.com>
> Suggested-by: David Vrabel <david.vrabel@citrix.com>
Jan's suggestion; I just agreed with him.
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Jason Luan <jianhai.luan@oracle.com>
> ---
> drivers/net/xen-netback/common.h | 1 +
> drivers/net/xen-netback/interface.c | 4 ++--
> drivers/net/xen-netback/netback.c | 13 ++++++-------
> 3 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
> index 5715318..400fea1 100644
> --- a/drivers/net/xen-netback/common.h
> +++ b/drivers/net/xen-netback/common.h
> @@ -163,6 +163,7 @@ struct xenvif {
> unsigned long credit_usec;
> unsigned long remaining_credit;
> struct timer_list credit_timeout;
> + u64 credit_window_start;
>
> /* Statistics */
> unsigned long rx_gso_checksum_fixup;
> diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
> index 01bb854..8c45b63 100644
> --- a/drivers/net/xen-netback/interface.c
> +++ b/drivers/net/xen-netback/interface.c
> @@ -312,8 +312,8 @@ struct xenvif *xenvif_alloc(struct device *parent, domid_t domid,
> vif->credit_bytes = vif->remaining_credit = ~0UL;
> vif->credit_usec = 0UL;
> init_timer(&vif->credit_timeout);
> - /* Initialize 'expires' now: it's used to track the credit window. */
> - vif->credit_timeout.expires = jiffies;
> + /* credit window is tracked in credit_window_start */
You could drop this comment as the field name makes this obvious.
> + vif->credit_window_start = get_jiffies_64();
>
> dev->netdev_ops = &xenvif_netdev_ops;
> dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO;
> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index f3e591c..1bc0688 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -1185,18 +1185,17 @@ out:
>
> static bool tx_credit_exceeded(struct xenvif *vif, unsigned size)
> {
> - unsigned long now = jiffies;
> - unsigned long next_credit =
> - vif->credit_timeout.expires +
> - msecs_to_jiffies(vif->credit_usec / 1000);
> + u64 now = get_jiffies_64();
> + u64 next_credit = vif->credit_window_start +
> + (u64)msecs_to_jiffies(vif->credit_usec / 1000);
>
> /* Timer could already be pending in rare cases. */
> if (timer_pending(&vif->credit_timeout))
> return true;
>
> /* Passed the point where we can replenish credit? */
> - if (time_after_eq(now, next_credit)) {
> - vif->credit_timeout.expires = now;
> + if (time_after_eq64(now, next_credit)) {
> + vif->credit_timeout.expires = (unsigned long)now;
Need to set vif->credit_window_start = now here instead of .expires.
> tx_add_credit(vif);
> }
>
> @@ -1207,7 +1206,7 @@ static bool tx_credit_exceeded(struct xenvif *vif, unsigned size)
> vif->credit_timeout.function =
> tx_credit_callback;
> mod_timer(&vif->credit_timeout,
> - next_credit);
> + (unsigned long)next_credit);
Need to set vif->credit_window_start = next_credit here as well.
David
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Xen-devel] [PATCH net] xen-netback: use jiffies_64 value to calculate credit timeout
2013-10-28 8:21 ` David Vrabel
@ 2013-10-28 8:36 ` jianhai luan
2013-10-28 8:36 ` jianhai luan
1 sibling, 0 replies; 13+ messages in thread
From: jianhai luan @ 2013-10-28 8:36 UTC (permalink / raw)
To: David Vrabel, Wei Liu, xen-devel, netdev
Cc: Ian Campbell, annie.li, david.vrabel, jbeulich
On 2013-10-28 16:21, David Vrabel wrote:
> On 27/10/2013 11:11, Wei Liu wrote:
>> time_after_eq() only works if the delta is < MAX_ULONG/2.
>>
>> For a 32bit Dom0, if netfront sends packets at a very low rate, the time
>> between subsequent calls to tx_credit_exceeded() may exceed MAX_ULONG/2
>> and the test for timer_after_eq() will be incorrect. Credit will not be
>> replenished and the guest may become unable to send packets (e.g., if
>> prior to the long gap, all credit was exhausted).
>>
>> Use jiffies_64 variant to mitigate this problem for 32bit Dom0.
> Thanks.
>
>> Suggested-by: Jan Beulich <jbeulich@suse.com>
>> Suggested-by: David Vrabel <david.vrabel@citrix.com>
> Jan's suggestion; I just agreed with him.
>
>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>> Cc: Ian Campbell <ian.campbell@citrix.com>
>> Cc: Jason Luan <jianhai.luan@oracle.com>
>> ---
>> drivers/net/xen-netback/common.h | 1 +
>> drivers/net/xen-netback/interface.c | 4 ++--
>> drivers/net/xen-netback/netback.c | 13 ++++++-------
>> 3 files changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
>> index 5715318..400fea1 100644
>> --- a/drivers/net/xen-netback/common.h
>> +++ b/drivers/net/xen-netback/common.h
>> @@ -163,6 +163,7 @@ struct xenvif {
>> unsigned long credit_usec;
>> unsigned long remaining_credit;
>> struct timer_list credit_timeout;
>> + u64 credit_window_start;
>>
>> /* Statistics */
>> unsigned long rx_gso_checksum_fixup;
>> diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
>> index 01bb854..8c45b63 100644
>> --- a/drivers/net/xen-netback/interface.c
>> +++ b/drivers/net/xen-netback/interface.c
>> @@ -312,8 +312,8 @@ struct xenvif *xenvif_alloc(struct device *parent, domid_t domid,
>> vif->credit_bytes = vif->remaining_credit = ~0UL;
>> vif->credit_usec = 0UL;
>> init_timer(&vif->credit_timeout);
>> - /* Initialize 'expires' now: it's used to track the credit window. */
>> - vif->credit_timeout.expires = jiffies;
>> + /* credit window is tracked in credit_window_start */
> You could drop this comment as the field name makes this obvious.
>
>> + vif->credit_window_start = get_jiffies_64();
>>
>> dev->netdev_ops = &xenvif_netdev_ops;
>> dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO;
>> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
>> index f3e591c..1bc0688 100644
>> --- a/drivers/net/xen-netback/netback.c
>> +++ b/drivers/net/xen-netback/netback.c
>> @@ -1185,18 +1185,17 @@ out:
>>
>> static bool tx_credit_exceeded(struct xenvif *vif, unsigned size)
>> {
>> - unsigned long now = jiffies;
>> - unsigned long next_credit =
>> - vif->credit_timeout.expires +
>> - msecs_to_jiffies(vif->credit_usec / 1000);
>> + u64 now = get_jiffies_64();
>> + u64 next_credit = vif->credit_window_start +
>> + (u64)msecs_to_jiffies(vif->credit_usec / 1000);
>>
>> /* Timer could already be pending in rare cases. */
>> if (timer_pending(&vif->credit_timeout))
>> return true;
>>
>> /* Passed the point where we can replenish credit? */
>> - if (time_after_eq(now, next_credit)) {
>> - vif->credit_timeout.expires = now;
>> + if (time_after_eq64(now, next_credit)) {
>> + vif->credit_timeout.expires = (unsigned long)now;
> Need to set vif->credit_window_start = now here instead of .expires.
I agree Annie's suggest. add the below line;
vif->credit_window_start = now
vif->credit_timeout.expires = (unsigned long)now;
>> tx_add_credit(vif);
>> }
>>
>> @@ -1207,7 +1206,7 @@ static bool tx_credit_exceeded(struct xenvif *vif, unsigned size)
>> vif->credit_timeout.function =
>> tx_credit_callback;
>> mod_timer(&vif->credit_timeout,
>> - next_credit);
>> + (unsigned long)next_credit);
> Need to set vif->credit_window_start = next_credit here as well.
I think that we shouldn't add the above line.
>
> David
Jason
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net] xen-netback: use jiffies_64 value to calculate credit timeout
2013-10-28 8:21 ` David Vrabel
2013-10-28 8:36 ` [Xen-devel] " jianhai luan
@ 2013-10-28 8:36 ` jianhai luan
1 sibling, 0 replies; 13+ messages in thread
From: jianhai luan @ 2013-10-28 8:36 UTC (permalink / raw)
To: David Vrabel, Wei Liu, xen-devel, netdev
Cc: annie.li, Ian Campbell, jbeulich, david.vrabel
On 2013-10-28 16:21, David Vrabel wrote:
> On 27/10/2013 11:11, Wei Liu wrote:
>> time_after_eq() only works if the delta is < MAX_ULONG/2.
>>
>> For a 32bit Dom0, if netfront sends packets at a very low rate, the time
>> between subsequent calls to tx_credit_exceeded() may exceed MAX_ULONG/2
>> and the test for timer_after_eq() will be incorrect. Credit will not be
>> replenished and the guest may become unable to send packets (e.g., if
>> prior to the long gap, all credit was exhausted).
>>
>> Use jiffies_64 variant to mitigate this problem for 32bit Dom0.
> Thanks.
>
>> Suggested-by: Jan Beulich <jbeulich@suse.com>
>> Suggested-by: David Vrabel <david.vrabel@citrix.com>
> Jan's suggestion; I just agreed with him.
>
>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>> Cc: Ian Campbell <ian.campbell@citrix.com>
>> Cc: Jason Luan <jianhai.luan@oracle.com>
>> ---
>> drivers/net/xen-netback/common.h | 1 +
>> drivers/net/xen-netback/interface.c | 4 ++--
>> drivers/net/xen-netback/netback.c | 13 ++++++-------
>> 3 files changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
>> index 5715318..400fea1 100644
>> --- a/drivers/net/xen-netback/common.h
>> +++ b/drivers/net/xen-netback/common.h
>> @@ -163,6 +163,7 @@ struct xenvif {
>> unsigned long credit_usec;
>> unsigned long remaining_credit;
>> struct timer_list credit_timeout;
>> + u64 credit_window_start;
>>
>> /* Statistics */
>> unsigned long rx_gso_checksum_fixup;
>> diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
>> index 01bb854..8c45b63 100644
>> --- a/drivers/net/xen-netback/interface.c
>> +++ b/drivers/net/xen-netback/interface.c
>> @@ -312,8 +312,8 @@ struct xenvif *xenvif_alloc(struct device *parent, domid_t domid,
>> vif->credit_bytes = vif->remaining_credit = ~0UL;
>> vif->credit_usec = 0UL;
>> init_timer(&vif->credit_timeout);
>> - /* Initialize 'expires' now: it's used to track the credit window. */
>> - vif->credit_timeout.expires = jiffies;
>> + /* credit window is tracked in credit_window_start */
> You could drop this comment as the field name makes this obvious.
>
>> + vif->credit_window_start = get_jiffies_64();
>>
>> dev->netdev_ops = &xenvif_netdev_ops;
>> dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO;
>> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
>> index f3e591c..1bc0688 100644
>> --- a/drivers/net/xen-netback/netback.c
>> +++ b/drivers/net/xen-netback/netback.c
>> @@ -1185,18 +1185,17 @@ out:
>>
>> static bool tx_credit_exceeded(struct xenvif *vif, unsigned size)
>> {
>> - unsigned long now = jiffies;
>> - unsigned long next_credit =
>> - vif->credit_timeout.expires +
>> - msecs_to_jiffies(vif->credit_usec / 1000);
>> + u64 now = get_jiffies_64();
>> + u64 next_credit = vif->credit_window_start +
>> + (u64)msecs_to_jiffies(vif->credit_usec / 1000);
>>
>> /* Timer could already be pending in rare cases. */
>> if (timer_pending(&vif->credit_timeout))
>> return true;
>>
>> /* Passed the point where we can replenish credit? */
>> - if (time_after_eq(now, next_credit)) {
>> - vif->credit_timeout.expires = now;
>> + if (time_after_eq64(now, next_credit)) {
>> + vif->credit_timeout.expires = (unsigned long)now;
> Need to set vif->credit_window_start = now here instead of .expires.
I agree Annie's suggest. add the below line;
vif->credit_window_start = now
vif->credit_timeout.expires = (unsigned long)now;
>> tx_add_credit(vif);
>> }
>>
>> @@ -1207,7 +1206,7 @@ static bool tx_credit_exceeded(struct xenvif *vif, unsigned size)
>> vif->credit_timeout.function =
>> tx_credit_callback;
>> mod_timer(&vif->credit_timeout,
>> - next_credit);
>> + (unsigned long)next_credit);
> Need to set vif->credit_window_start = next_credit here as well.
I think that we shouldn't add the above line.
>
> David
Jason
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH net] xen-netback: use jiffies_64 value to calculate credit timeout
@ 2013-10-27 11:11 Wei Liu
0 siblings, 0 replies; 13+ messages in thread
From: Wei Liu @ 2013-10-27 11:11 UTC (permalink / raw)
To: xen-devel, netdev
Cc: Wei Liu, Ian Campbell, annie.li, david.vrabel, jbeulich, Jason Luan
time_after_eq() only works if the delta is < MAX_ULONG/2.
For a 32bit Dom0, if netfront sends packets at a very low rate, the time
between subsequent calls to tx_credit_exceeded() may exceed MAX_ULONG/2
and the test for timer_after_eq() will be incorrect. Credit will not be
replenished and the guest may become unable to send packets (e.g., if
prior to the long gap, all credit was exhausted).
Use jiffies_64 variant to mitigate this problem for 32bit Dom0.
Suggested-by: Jan Beulich <jbeulich@suse.com>
Suggested-by: David Vrabel <david.vrabel@citrix.com>
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Jason Luan <jianhai.luan@oracle.com>
---
drivers/net/xen-netback/common.h | 1 +
drivers/net/xen-netback/interface.c | 4 ++--
drivers/net/xen-netback/netback.c | 13 ++++++-------
3 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
index 5715318..400fea1 100644
--- a/drivers/net/xen-netback/common.h
+++ b/drivers/net/xen-netback/common.h
@@ -163,6 +163,7 @@ struct xenvif {
unsigned long credit_usec;
unsigned long remaining_credit;
struct timer_list credit_timeout;
+ u64 credit_window_start;
/* Statistics */
unsigned long rx_gso_checksum_fixup;
diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
index 01bb854..8c45b63 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c
@@ -312,8 +312,8 @@ struct xenvif *xenvif_alloc(struct device *parent, domid_t domid,
vif->credit_bytes = vif->remaining_credit = ~0UL;
vif->credit_usec = 0UL;
init_timer(&vif->credit_timeout);
- /* Initialize 'expires' now: it's used to track the credit window. */
- vif->credit_timeout.expires = jiffies;
+ /* credit window is tracked in credit_window_start */
+ vif->credit_window_start = get_jiffies_64();
dev->netdev_ops = &xenvif_netdev_ops;
dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO;
diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index f3e591c..1bc0688 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -1185,18 +1185,17 @@ out:
static bool tx_credit_exceeded(struct xenvif *vif, unsigned size)
{
- unsigned long now = jiffies;
- unsigned long next_credit =
- vif->credit_timeout.expires +
- msecs_to_jiffies(vif->credit_usec / 1000);
+ u64 now = get_jiffies_64();
+ u64 next_credit = vif->credit_window_start +
+ (u64)msecs_to_jiffies(vif->credit_usec / 1000);
/* Timer could already be pending in rare cases. */
if (timer_pending(&vif->credit_timeout))
return true;
/* Passed the point where we can replenish credit? */
- if (time_after_eq(now, next_credit)) {
- vif->credit_timeout.expires = now;
+ if (time_after_eq64(now, next_credit)) {
+ vif->credit_timeout.expires = (unsigned long)now;
tx_add_credit(vif);
}
@@ -1207,7 +1206,7 @@ static bool tx_credit_exceeded(struct xenvif *vif, unsigned size)
vif->credit_timeout.function =
tx_credit_callback;
mod_timer(&vif->credit_timeout,
- next_credit);
+ (unsigned long)next_credit);
return true;
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2013-10-28 11:30 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-10-27 11:11 [PATCH net] xen-netback: use jiffies_64 value to calculate credit timeout Wei Liu
2013-10-28 2:36 ` annie li
2013-10-28 2:36 ` annie li
2013-10-28 2:58 ` annie li
2013-10-28 2:58 ` [Xen-devel] " annie li
2013-10-28 6:28 ` jianhai luan
2013-10-28 6:28 ` jianhai luan
2013-10-28 11:30 ` [Xen-devel] " Wei Liu
2013-10-28 11:30 ` Wei Liu
2013-10-28 8:21 ` David Vrabel
2013-10-28 8:36 ` [Xen-devel] " jianhai luan
2013-10-28 8:36 ` jianhai luan
-- strict thread matches above, loose matches on Subject: below --
2013-10-27 11:11 Wei Liu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.