Re: [Xen-devel] [PATCH v4 3/3] netif documentation

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Mon, Aug 05, 2013 at 05:13:10PM +0200, William Dauchy wrote:
> add netif old rate limit documentation
> add new pps limit documentation
> 
> Signed-off-by: Ahmed Amamou <ahmed@xxxxxxxxx>
> Signed-off-by: William Dauchy <william@xxxxxxxxx>
> Signed-off-by: Kamel Haddadou <kamel@xxxxxxxxx>
> ---
>  docs/misc/xl-network-configuration.markdown |   18 +++++++++++++-----
>  xen/include/public/io/netif.h               |   27 
> +++++++++++++++++++++++++++
>  2 files changed, 40 insertions(+), 5 deletions(-)
> 
> diff --git a/docs/misc/xl-network-configuration.markdown 
> b/docs/misc/xl-network-configuration.markdown
> index e0d3d2a..1f4ffff 100644
> --- a/docs/misc/xl-network-configuration.markdown
> +++ b/docs/misc/xl-network-configuration.markdown
> @@ -141,25 +141,33 @@ domain which is outside the scope of this document.
>  Specifies the rate at which the outgoing traffic will be limited to.
>  The default if this keyword is not specified is unlimited.
>  
> -The rate may be specified as "<RATE>/s" or optionally "<RATE>/s@<INTERVAL>".
> +The rate may be specified as "<RATE>/s" or optionally
> +"<RATE>/s@<INTERVAL>" or "<RATE>/s&<PPS>pps" or 
> "<RATE>/s&<PPS>pps@<INTERVAL>".
>  
>    * `RATE` is in bytes and can accept suffixes:
>        * GB, MB, KB, B for bytes.
>        * Gb, Mb, Kb, b for bits.
> +
> +  * `PPS` is in packet and can accept suffixes:
> +      * Gpps, Mpps, Kpps, pps.
> +    It determines the packets per second that outgoing traffic will be
> +    limited to.
> +

This documentation doesn't match the fact. Actually the "packet" here is
just proximation. You should probably describe the limitation /
assumption as well.

>    * `INTERVAL` is in microseconds and can accept suffixes: ms, us, s.
>      It determines the frequency at which the vif transmission credit
>      is replenished. The default is 50ms.
>  
> -Vif rate limiting is credit-based. It means that for "1MB/s@20ms", the
> +Vif rate limiting is credit-based. It means that for "1MB/s&50Kpps@20ms", the

Line too long. Please align with previous line.

>  available credit will be equivalent of the traffic you would have done
> -at "1MB/s" during 20ms. This will results in a credit of 20,000 bytes
> -replenished every 20,000 us.
> +at "1MB/s" or 50,000 pps during 20ms. This will results in a credit of 20,000

Ditto.

> +bytes or 1000 packets replenished every 20,000 us.
>  
>  For example:
>  
>          'rate=10Mb/s' -- meaning up to 10 megabits every second
>          'rate=250KB/s' -- meaning up to 250 kilobytes every second
> -        'rate=1MB/s@20ms' -- meaning 20,000 bytes in every 20 millisecond 
> period
> +        'rate=1MB/s&10Kpps@20ms' -- meaning 20,000 bytes or 200 packets in
> +every 20 millisecond period

Should indent this line with "meaning".

>  
>  NOTE: The actual underlying limits of rate limiting are dependent
>  on the underlying netback implementation.
> diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h
> index d477751..8bd112f 100644
> --- a/xen/include/public/io/netif.h
> +++ b/xen/include/public/io/netif.h
> @@ -79,6 +79,33 @@
>   *  Request N: netif_tx_request -- 0
>   */
>  
> +/*
> + * ------------------------- Backend Device Properties 
> -------------------------
> + *
> + * interval
> + *      Values:         <uint64_t>
> + *      Default Value:  0
> + *
> + *      Used time interval in usecond for throughput and pps limits. The
> + * same interval is used for both of them in order to simplify the

What is "them" referring to?

> + * implementation. Using the same check interval also avoid possible bugs
> + * that invole bypassing these limits
           ^^^^^^
           typo?

> + *
> + * rate
> + *      Values:         <uint64_t>
> + *      Default Value:  ~0
> + *
> + *      The netback reading rate in bytes from the shared ring. This rate is
> + * represented in bytes per interval.
> + *
> + * pps
> + *      Values:         <uint64_t>
> + *      Default Value:  ~0
> + *
> + *      The netback reading rate in tx slots from the shared ring.This rate 
> is

Need a space before "This". Also this line exceeds 80 characters.

> + * represented in request per interval.

This is rather confusing, because
1) pps is packets per second, but you use "request per interval" here
2) "request" is just too generic (should use netif_tx_* instead)


Wei.

> + */
> +
>  /* Protocol checksum field is blank in the packet (hardware offload)? */
>  #define _NETTXF_csum_blank     (0)
>  #define  NETTXF_csum_blank     (1U<<_NETTXF_csum_blank)
> -- 
> 1.7.9.5

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