[PATCH v2] xen-netback: add a pseudo pps rate limit

[PATCH v2] xen-netback: add a pseudo pps rate limit

[William Dauchy]

VM traffic is already limited by a throughput limit, but there is no
control over the maximum packet per second (PPS).
In DDOS attack the major issue is rather PPS than throughput.
With provider offering more bandwidth to VMs, it becames easy to
coordinate a massive attack using VMs. Example: 100Mbits ~ 200kpps using
64B packets.
This patch provides a new option to limit VMs maximum packets per second
emission rate.
It follows the same credits logic used for throughput shaping. For the
moment we have considered each "txreq" as a packet.
PPS limits is passed to VIF at connection time via xenstore.
PPS credit uses the same usecond period used by rate shaping check.

known limitations:
- by using the same usecond period, PPS shaping depends on throughput
  shaping.
- it is not always true that a "txreq" correspond to a paquet
  (fragmentation cases) but as this shaping is meant to avoid DDOS
  (small paquets) such an pproximation should not impact the results.
- Some help on burst handling will be appreciated.

v2:
- fixing some typo

Signed-off-by: Ahmed Amamou <ahmed@gandi.net>
Signed-off-by: William Dauchy <william@gandi.net>
Signed-off-by: Kamel Haddadou <kamel@gandi.net>
---
 drivers/net/xen-netback/common.h    |    2 ++
 drivers/net/xen-netback/interface.c |    1 +
 drivers/net/xen-netback/netback.c   |   46 +++++++++++++++++++++++++++++++++++
 drivers/net/xen-netback/xenbus.c    |   25 ++++++++++++++++---
 4 files changed, 70 insertions(+), 4 deletions(-)

diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
index 9d7f172..fefa79a 100644
--- a/drivers/net/xen-netback/common.h
+++ b/drivers/net/xen-netback/common.h
@@ -85,8 +85,10 @@ struct xenvif {
 
 	/* Transmit shaping: allow 'credit_bytes' every 'credit_usec'. */
 	unsigned long   credit_bytes;
+	unsigned long   credit_packets;
 	unsigned long   credit_usec;
 	unsigned long   remaining_credit;
+	unsigned long   remaining_packets;
 	struct timer_list credit_timeout;
 
 	/* Statistics */
diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
index d984141..06257dd 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c
@@ -273,6 +273,7 @@ struct xenvif *xenvif_alloc(struct device *parent, domid_t domid,
 	INIT_LIST_HEAD(&vif->notify_list);
 
 	vif->credit_bytes = vif->remaining_credit = ~0UL;
+	vif->credit_packets = vif->remaining_packets = ~0UL;
 	vif->credit_usec  = 0UL;
 	init_timer(&vif->credit_timeout);
 	/* Initialize 'expires' now: it's used to track the credit window. */
diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index 8c20935..097a390 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -905,10 +905,16 @@ static void tx_add_credit(struct xenvif *vif)
 	vif->remaining_credit = min(max_credit, max_burst);
 }
 
+static void tx_add_packets(struct xenvif *vif)
+{
+	vif->remaining_packets = vif->credit_packets;
+}
+
 static void tx_credit_callback(unsigned long data)
 {
 	struct xenvif *vif = (struct xenvif *)data;
 	tx_add_credit(vif);
+	tx_add_packets(vif);
 	xen_netbk_check_rx_xenvif(vif);
 }
 
@@ -1419,6 +1425,38 @@ static bool tx_credit_exceeded(struct xenvif *vif, unsigned size)
 	return false;
 }
 
+static bool tx_packets_exceeded(struct xenvif *vif)
+{
+	unsigned long now = jiffies;
+	unsigned long next_credit =
+		vif->credit_timeout.expires +
+		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;
+		tx_add_packets(vif);
+	}
+
+	/* Not enough slot to send right now? Set a callback. */
+	if (vif->remaining_packets < 1) {
+		vif->credit_timeout.data     =
+			(unsigned long)vif;
+		vif->credit_timeout.function =
+			tx_credit_callback;
+		mod_timer(&vif->credit_timeout,
+			  next_credit);
+
+		return true;
+	}
+
+	return false;
+}
+
 static unsigned xen_netbk_tx_build_gops(struct xen_netbk *netbk)
 {
 	struct gnttab_copy *gop = netbk->tx_copy_ops, *request_gop;
@@ -1470,6 +1508,13 @@ static unsigned xen_netbk_tx_build_gops(struct xen_netbk *netbk)
 		rmb(); /* Ensure that we see the request before we copy it. */
 		memcpy(&txreq, RING_GET_REQUEST(&vif->tx, idx), sizeof(txreq));
 
+		/* pps-based scheduling. */
+		if(vif->remaining_packets < 1 &&
+		   tx_packets_exceeded(vif)) {
+		    xenvif_put(vif);
+		    continue;
+		}
+
 		/* Credit-based scheduling. */
 		if (txreq.size > vif->remaining_credit &&
 		    tx_credit_exceeded(vif, txreq.size)) {
@@ -1478,6 +1523,7 @@ static unsigned xen_netbk_tx_build_gops(struct xen_netbk *netbk)
 		}
 
 		vif->remaining_credit -= txreq.size;
+		vif->remaining_packets--;
 
 		work_to_do--;
 		vif->tx.req_cons = ++idx;
diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
index 410018c..7c55bed 100644
--- a/drivers/net/xen-netback/xenbus.c
+++ b/drivers/net/xen-netback/xenbus.c
@@ -267,15 +267,18 @@ static void frontend_changed(struct xenbus_device *dev,
 
 
 static void xen_net_read_rate(struct xenbus_device *dev,
-			      unsigned long *bytes, unsigned long *usec)
+			      unsigned long *bytes,
+			      unsigned long *packet,
+			      unsigned long *usec)
 {
 	char *s, *e;
-	unsigned long b, u;
-	char *ratestr;
+	unsigned long b, u, pps;
+	char *ratestr, *ppsstr;
 
 	/* Default to unlimited bandwidth. */
 	*bytes = ~0UL;
 	*usec = 0;
+	*packet = ~0UL;
 
 	ratestr = xenbus_read(XBT_NIL, dev->nodename, "rate", NULL);
 	if (IS_ERR(ratestr))
@@ -295,11 +298,24 @@ static void xen_net_read_rate(struct xenbus_device *dev,
 	*usec = u;
 
 	kfree(ratestr);
+	ppsstr = xenbus_read(XBT_NIL, dev->nodename, "pps", NULL);
+	if (IS_ERR(ppsstr))
+	  return;
+	s = ppsstr;
+	pps = simple_strtoul(s, &e, 10);
+	if ((s == e) || (*e != '\0'))
+	  goto fail2;
+	*packet = pps;
+	kfree(ppsstr);
 	return;
 
  fail:
 	pr_warn("Failed to parse network rate limit. Traffic unlimited.\n");
 	kfree(ratestr);
+	return;
+fail2:
+	pr_warn("Failed to parse network PPS limit. PPS unlimited.\n");
+	kfree(ppsstr);
 }
 
 static int xen_net_read_mac(struct xenbus_device *dev, u8 mac[])
@@ -370,8 +386,9 @@ static void connect(struct backend_info *be)
 	}
 
 	xen_net_read_rate(dev, &be->vif->credit_bytes,
-			  &be->vif->credit_usec);
+			  &be->vif->credit_packets, &be->vif->credit_usec);
 	be->vif->remaining_credit = be->vif->credit_bytes;
+	be->vif->remaining_packets = be->vif->credit_packets;
 
 	unregister_hotplug_status_watch(be);
 	err = xenbus_watch_pathfmt(dev, &be->hotplug_status_watch,
-- 
1.7.9.5

Re: [PATCH v2] xen-netback: add a pseudo pps rate limit

[William Dauchy]

[-- Attachment #1.1: Type: text/plain, Size: 1236 bytes --]

On Jun24 17:22, William Dauchy wrote:
> VM traffic is already limited by a throughput limit, but there is no
> control over the maximum packet per second (PPS).
> In DDOS attack the major issue is rather PPS than throughput.
> With provider offering more bandwidth to VMs, it becames easy to
> coordinate a massive attack using VMs. Example: 100Mbits ~ 200kpps using
> 64B packets.
> This patch provides a new option to limit VMs maximum packets per second
> emission rate.
> It follows the same credits logic used for throughput shaping. For the
> moment we have considered each "txreq" as a packet.
> PPS limits is passed to VIF at connection time via xenstore.
> PPS credit uses the same usecond period used by rate shaping check.
> 
> known limitations:
> - by using the same usecond period, PPS shaping depends on throughput
>   shaping.
> - it is not always true that a "txreq" correspond to a paquet
>   (fragmentation cases) but as this shaping is meant to avoid DDOS
>   (small paquets) such an pproximation should not impact the results.
> - Some help on burst handling will be appreciated.
> 
> v2:
> - fixing some typo

any chance to get it accepted? some other comments?

Regards,
-- 
William

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2] xen-netback: add a pseudo pps rate limit

[Ian Campbell]

On Mon, 2013-06-24 at 17:22 +0200, William Dauchy wrote:

Sorry for the delay replying.

> VM traffic is already limited by a throughput limit, but there is no
> control over the maximum packet per second (PPS).
> In DDOS attack the major issue is rather PPS than throughput.
> With provider offering more bandwidth to VMs, it becames easy to
> coordinate a massive attack using VMs. Example: 100Mbits ~ 200kpps using
> 64B packets.
> This patch provides a new option to limit VMs maximum packets per second
> emission rate.
> It follows the same credits logic used for throughput shaping. For the
> moment we have considered each "txreq" as a packet.

So it is in effect a slots-per-second (or requests-per...) limit, which
I suppose is pseudo-pps as you say.

> PPS limits is passed to VIF at connection time via xenstore.
> PPS credit uses the same usecond period used by rate shaping check.
> 
> known limitations:
> - by using the same usecond period, PPS shaping depends on throughput
>   shaping.
> - it is not always true that a "txreq" correspond to a paquet

"packet"

>   (fragmentation cases) but as this shaping is meant to avoid DDOS
>   (small paquets) such an pproximation should not impact the results.

and again, plus missing an a on the front of approx...

Are you going to provide toolstack side patches to utilise this feature,
as well as documentation for the xenstore key?

> - Some help on burst handling will be appreciated.
> 
> v2:
> - fixing some typo
> 
> Signed-off-by: Ahmed Amamou <ahmed@gandi.net>
> Signed-off-by: William Dauchy <william@gandi.net>
> Signed-off-by: Kamel Haddadou <kamel@gandi.net>
> ---
>  drivers/net/xen-netback/common.h    |    2 ++
>  drivers/net/xen-netback/interface.c |    1 +
>  drivers/net/xen-netback/netback.c   |   46 +++++++++++++++++++++++++++++++++++
>  drivers/net/xen-netback/xenbus.c    |   25 ++++++++++++++++---
>  4 files changed, 70 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
> index 9d7f172..fefa79a 100644
> --- a/drivers/net/xen-netback/common.h
> +++ b/drivers/net/xen-netback/common.h
> @@ -85,8 +85,10 @@ struct xenvif {
>  
>  	/* Transmit shaping: allow 'credit_bytes' every 'credit_usec'. */
>  	unsigned long   credit_bytes;
> +	unsigned long   credit_packets;
>  	unsigned long   credit_usec;
>  	unsigned long   remaining_credit;
> +	unsigned long   remaining_packets;
>  	struct timer_list credit_timeout;
>  
>  	/* Statistics */
> diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
> index d984141..06257dd 100644
> --- a/drivers/net/xen-netback/interface.c
> +++ b/drivers/net/xen-netback/interface.c
> @@ -273,6 +273,7 @@ struct xenvif *xenvif_alloc(struct device *parent, domid_t domid,
>  	INIT_LIST_HEAD(&vif->notify_list);
>  
>  	vif->credit_bytes = vif->remaining_credit = ~0UL;
> +	vif->credit_packets = vif->remaining_packets = ~0UL;
>  	vif->credit_usec  = 0UL;
>  	init_timer(&vif->credit_timeout);
>  	/* Initialize 'expires' now: it's used to track the credit window. */
> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index 8c20935..097a390 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -905,10 +905,16 @@ static void tx_add_credit(struct xenvif *vif)
>  	vif->remaining_credit = min(max_credit, max_burst);
>  }
>  
> +static void tx_add_packets(struct xenvif *vif)
> +{
> +	vif->remaining_packets = vif->credit_packets;
> +}
> +
>  static void tx_credit_callback(unsigned long data)
>  {
>  	struct xenvif *vif = (struct xenvif *)data;
>  	tx_add_credit(vif);
> +	tx_add_packets(vif);
>  	xen_netbk_check_rx_xenvif(vif);
>  }
>  
> @@ -1419,6 +1425,38 @@ static bool tx_credit_exceeded(struct xenvif *vif, unsigned size)
>  	return false;
>  }
>  
> +static bool tx_packets_exceeded(struct xenvif *vif)
> +{
> +	unsigned long now = jiffies;
> +	unsigned long next_credit =
> +		vif->credit_timeout.expires +
> +		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;
> +		tx_add_packets(vif);
> +	}
> +
> +	/* Not enough slot to send right now? Set a callback. */
> +	if (vif->remaining_packets < 1) {
> +		vif->credit_timeout.data     =
> +			(unsigned long)vif;
> +		vif->credit_timeout.function =
> +			tx_credit_callback;
> +		mod_timer(&vif->credit_timeout,
> +			  next_credit);

This is the same timer as the credit based timeout?

How do the two interact? Can I cause my packet credits to be replenished
sooner by cleverly manipulating my byte rates or vice versa?

> +
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
>  static unsigned xen_netbk_tx_build_gops(struct xen_netbk *netbk)
>  {
>  	struct gnttab_copy *gop = netbk->tx_copy_ops, *request_gop;
> @@ -1470,6 +1508,13 @@ static unsigned xen_netbk_tx_build_gops(struct xen_netbk *netbk)
>  		rmb(); /* Ensure that we see the request before we copy it. */
>  		memcpy(&txreq, RING_GET_REQUEST(&vif->tx, idx), sizeof(txreq));
>  
> +		/* pps-based scheduling. */
> +		if(vif->remaining_packets < 1 &&
> +		   tx_packets_exceeded(vif)) {
> +		    xenvif_put(vif);
> +		    continue;

This should be tab indented, you've used spaces after the initial tabs.

> +		}
> +
>  		/* Credit-based scheduling. */
>  		if (txreq.size > vif->remaining_credit &&
>  		    tx_credit_exceeded(vif, txreq.size)) {
> @@ -1478,6 +1523,7 @@ static unsigned xen_netbk_tx_build_gops(struct xen_netbk *netbk)
>  		}
>  
>  		vif->remaining_credit -= txreq.size;
> +		vif->remaining_packets--;

Interesting that even if credit scoring is disable (== ~0) we will
eventually hit zero and have to replenish. This applies to the existing
credit stuff too as far as I can tell.

I suppose if we replenish to ~0 then it is likely to be mostly harmless.


>  
>  		work_to_do--;
>  		vif->tx.req_cons = ++idx;
> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
> index 410018c..7c55bed 100644
> --- a/drivers/net/xen-netback/xenbus.c
> +++ b/drivers/net/xen-netback/xenbus.c
> @@ -267,15 +267,18 @@ static void frontend_changed(struct xenbus_device *dev,
>  
> 
>  static void xen_net_read_rate(struct xenbus_device *dev,
> -			      unsigned long *bytes, unsigned long *usec)
> +			      unsigned long *bytes,
> +			      unsigned long *packet,
> +			      unsigned long *usec)
>  {
>  	char *s, *e;
> -	unsigned long b, u;
> -	char *ratestr;
> +	unsigned long b, u, pps;
> +	char *ratestr, *ppsstr;
>  
>  	/* Default to unlimited bandwidth. */
>  	*bytes = ~0UL;
>  	*usec = 0;
> +	*packet = ~0UL;
>  
>  	ratestr = xenbus_read(XBT_NIL, dev->nodename, "rate", NULL);
>  	if (IS_ERR(ratestr))
> @@ -295,11 +298,24 @@ static void xen_net_read_rate(struct xenbus_device *dev,
>  	*usec = u;
>  
>  	kfree(ratestr);
> +	ppsstr = xenbus_read(XBT_NIL, dev->nodename, "pps", NULL);

What is the format of this string? Please can you document in the Xen
tree in xen/include/public/io/netif.h.

netif isn't terribly well documented, but we can at least strive to
document new stuff. The blkif header is a good example of how things
should be.

> +	if (IS_ERR(ppsstr))
> +	  return;
> +	s = ppsstr;
> +	pps = simple_strtoul(s, &e, 10);
> +	if ((s == e) || (*e != '\0'))
> +	  goto fail2;
> +	*packet = pps;
> +	kfree(ppsstr);
>  	return;
>  
>   fail:
>  	pr_warn("Failed to parse network rate limit. Traffic unlimited.\n");
>  	kfree(ratestr);
> +	return;
> +fail2:
> +	pr_warn("Failed to parse network PPS limit. PPS unlimited.\n");
> +	kfree(ppsstr);
>  }
>  
>  static int xen_net_read_mac(struct xenbus_device *dev, u8 mac[])
> @@ -370,8 +386,9 @@ static void connect(struct backend_info *be)
>  	}
>  
>  	xen_net_read_rate(dev, &be->vif->credit_bytes,
> -			  &be->vif->credit_usec);
> +			  &be->vif->credit_packets, &be->vif->credit_usec);
>  	be->vif->remaining_credit = be->vif->credit_bytes;
> +	be->vif->remaining_packets = be->vif->credit_packets;
>  
>  	unregister_hotplug_status_watch(be);
>  	err = xenbus_watch_pathfmt(dev, &be->hotplug_status_watch,

Re: [PATCH v2] xen-netback: add a pseudo pps rate limit

[Ahmed AMAMOU]

2013/7/2 Ian Campbell <Ian.Campbell@citrix.com>:
> On Mon, 2013-06-24 at 17:22 +0200, William Dauchy wrote:
>
> Sorry for the delay replying.
>
>> VM traffic is already limited by a throughput limit, but there is no
>> control over the maximum packet per second (PPS).
>> In DDOS attack the major issue is rather PPS than throughput.
>> With provider offering more bandwidth to VMs, it becames easy to
>> coordinate a massive attack using VMs. Example: 100Mbits ~ 200kpps using
>> 64B packets.
>> This patch provides a new option to limit VMs maximum packets per second
>> emission rate.
>> It follows the same credits logic used for throughput shaping. For the
>> moment we have considered each "txreq" as a packet.
>
> So it is in effect a slots-per-second (or requests-per...) limit, which
> I suppose is pseudo-pps as you say.
>
>> PPS limits is passed to VIF at connection time via xenstore.
>> PPS credit uses the same usecond period used by rate shaping check.
>>
>> known limitations:
>> - by using the same usecond period, PPS shaping depends on throughput
>>   shaping.
>> - it is not always true that a "txreq" correspond to a paquet
>
> "packet"
>
>>   (fragmentation cases) but as this shaping is meant to avoid DDOS
>>   (small paquets) such an pproximation should not impact the results.
>
> and again, plus missing an a on the front of approx...
>

typo correction and toolstack patch to libxl will be provided in patch v3

> Are you going to provide toolstack side patches to utilise this feature,
> as well as documentation for the xenstore key?
>
>> - Some help on burst handling will be appreciated.
>>
>> v2:
>> - fixing some typo
>>
>> Signed-off-by: Ahmed Amamou <ahmed@gandi.net>
>> Signed-off-by: William Dauchy <william@gandi.net>
>> Signed-off-by: Kamel Haddadou <kamel@gandi.net>
>> ---
>>  drivers/net/xen-netback/common.h    |    2 ++
>>  drivers/net/xen-netback/interface.c |    1 +
>>  drivers/net/xen-netback/netback.c   |   46 +++++++++++++++++++++++++++++++++++
>>  drivers/net/xen-netback/xenbus.c    |   25 ++++++++++++++++---
>>  4 files changed, 70 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
>> index 9d7f172..fefa79a 100644
>> --- a/drivers/net/xen-netback/common.h
>> +++ b/drivers/net/xen-netback/common.h
>> @@ -85,8 +85,10 @@ struct xenvif {
>>
>>       /* Transmit shaping: allow 'credit_bytes' every 'credit_usec'. */
>>       unsigned long   credit_bytes;
>> +     unsigned long   credit_packets;
>>       unsigned long   credit_usec;
>>       unsigned long   remaining_credit;
>> +     unsigned long   remaining_packets;
>>       struct timer_list credit_timeout;
>>
>>       /* Statistics */
>> diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
>> index d984141..06257dd 100644
>> --- a/drivers/net/xen-netback/interface.c
>> +++ b/drivers/net/xen-netback/interface.c
>> @@ -273,6 +273,7 @@ struct xenvif *xenvif_alloc(struct device *parent, domid_t domid,
>>       INIT_LIST_HEAD(&vif->notify_list);
>>
>>       vif->credit_bytes = vif->remaining_credit = ~0UL;
>> +     vif->credit_packets = vif->remaining_packets = ~0UL;
>>       vif->credit_usec  = 0UL;
>>       init_timer(&vif->credit_timeout);
>>       /* Initialize 'expires' now: it's used to track the credit window. */
>> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
>> index 8c20935..097a390 100644
>> --- a/drivers/net/xen-netback/netback.c
>> +++ b/drivers/net/xen-netback/netback.c
>> @@ -905,10 +905,16 @@ static void tx_add_credit(struct xenvif *vif)
>>       vif->remaining_credit = min(max_credit, max_burst);
>>  }
>>
>> +static void tx_add_packets(struct xenvif *vif)
>> +{
>> +     vif->remaining_packets = vif->credit_packets;
>> +}
>> +
>>  static void tx_credit_callback(unsigned long data)
>>  {
>>       struct xenvif *vif = (struct xenvif *)data;
>>       tx_add_credit(vif);
>> +     tx_add_packets(vif);
>>       xen_netbk_check_rx_xenvif(vif);
>>  }
>>
>> @@ -1419,6 +1425,38 @@ static bool tx_credit_exceeded(struct xenvif *vif, unsigned size)
>>       return false;
>>  }
>>
>> +static bool tx_packets_exceeded(struct xenvif *vif)
>> +{
>> +     unsigned long now = jiffies;
>> +     unsigned long next_credit =
>> +             vif->credit_timeout.expires +
>> +             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;
>> +             tx_add_packets(vif);
>> +     }
>> +
>> +     /* Not enough slot to send right now? Set a callback. */
>> +     if (vif->remaining_packets < 1) {
>> +             vif->credit_timeout.data     =
>> +                     (unsigned long)vif;
>> +             vif->credit_timeout.function =
>> +                     tx_credit_callback;
>> +             mod_timer(&vif->credit_timeout,
>> +                       next_credit);
>
> This is the same timer as the credit based timeout?
>
> How do the two interact? Can I cause my packet credits to be replenished
> sooner by cleverly manipulating my byte rates or vice versa?

Yes we use the same timer, we made this on purpose just to avoid any
interaction that may involve replenishing the credits sooner that expected
>> +
>> +             return true;
>> +     }
>> +
>> +     return false;
>> +}
>> +
>>  static unsigned xen_netbk_tx_build_gops(struct xen_netbk *netbk)
>>  {
>>       struct gnttab_copy *gop = netbk->tx_copy_ops, *request_gop;
>> @@ -1470,6 +1508,13 @@ static unsigned xen_netbk_tx_build_gops(struct xen_netbk *netbk)
>>               rmb(); /* Ensure that we see the request before we copy it. */
>>               memcpy(&txreq, RING_GET_REQUEST(&vif->tx, idx), sizeof(txreq));
>>
>> +             /* pps-based scheduling. */
>> +             if(vif->remaining_packets < 1 &&
>> +                tx_packets_exceeded(vif)) {
>> +                 xenvif_put(vif);
>> +                 continue;
>
> This should be tab indented, you've used spaces after the initial tabs.
corrected in v3
>> +             }
>> +
>>               /* Credit-based scheduling. */
>>               if (txreq.size > vif->remaining_credit &&
>>                   tx_credit_exceeded(vif, txreq.size)) {
>> @@ -1478,6 +1523,7 @@ static unsigned xen_netbk_tx_build_gops(struct xen_netbk *netbk)
>>               }
>>
>>               vif->remaining_credit -= txreq.size;
>> +             vif->remaining_packets--;
>
> Interesting that even if credit scoring is disable (== ~0) we will
> eventually hit zero and have to replenish. This applies to the existing
> credit stuff too as far as I can tell.
>
> I suppose if we replenish to ~0 then it is likely to be mostly harmless.
>
>
>>
>>               work_to_do--;
>>               vif->tx.req_cons = ++idx;
>> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
>> index 410018c..7c55bed 100644
>> --- a/drivers/net/xen-netback/xenbus.c
>> +++ b/drivers/net/xen-netback/xenbus.c
>> @@ -267,15 +267,18 @@ static void frontend_changed(struct xenbus_device *dev,
>>
>>
>>  static void xen_net_read_rate(struct xenbus_device *dev,
>> -                           unsigned long *bytes, unsigned long *usec)
>> +                           unsigned long *bytes,
>> +                           unsigned long *packet,
>> +                           unsigned long *usec)
>>  {
>>       char *s, *e;
>> -     unsigned long b, u;
>> -     char *ratestr;
>> +     unsigned long b, u, pps;
>> +     char *ratestr, *ppsstr;
>>
>>       /* Default to unlimited bandwidth. */
>>       *bytes = ~0UL;
>>       *usec = 0;
>> +     *packet = ~0UL;
>>
>>       ratestr = xenbus_read(XBT_NIL, dev->nodename, "rate", NULL);
>>       if (IS_ERR(ratestr))
>> @@ -295,11 +298,24 @@ static void xen_net_read_rate(struct xenbus_device *dev,
>>       *usec = u;
>>
>>       kfree(ratestr);
>> +     ppsstr = xenbus_read(XBT_NIL, dev->nodename, "pps", NULL);
>
> What is the format of this string? Please can you document in the Xen
> tree in xen/include/public/io/netif.h.
>
> netif isn't terribly well documented, but we can at least strive to
> document new stuff. The blkif header is a good example of how things
> should be.
>
documentation on netif.h will also be available on patch v3
>> +     if (IS_ERR(ppsstr))
>> +       return;
>> +     s = ppsstr;
>> +     pps = simple_strtoul(s, &e, 10);
>> +     if ((s == e) || (*e != '\0'))
>> +       goto fail2;
>> +     *packet = pps;
>> +     kfree(ppsstr);
>>       return;
>>
>>   fail:
>>       pr_warn("Failed to parse network rate limit. Traffic unlimited.\n");
>>       kfree(ratestr);
>> +     return;
>> +fail2:
>> +     pr_warn("Failed to parse network PPS limit. PPS unlimited.\n");
>> +     kfree(ppsstr);
>>  }
>>
>>  static int xen_net_read_mac(struct xenbus_device *dev, u8 mac[])
>> @@ -370,8 +386,9 @@ static void connect(struct backend_info *be)
>>       }
>>
>>       xen_net_read_rate(dev, &be->vif->credit_bytes,
>> -                       &be->vif->credit_usec);
>> +                       &be->vif->credit_packets, &be->vif->credit_usec);
>>       be->vif->remaining_credit = be->vif->credit_bytes;
>> +     be->vif->remaining_packets = be->vif->credit_packets;
>>
>>       unregister_hotplug_status_watch(be);
>>       err = xenbus_watch_pathfmt(dev, &be->hotplug_status_watch,
>