Re: ECN + pfifo_fast borked?

[d@taht.net (Dave Täht)]

Note: this is a forward of an existing conversation on netdev about how
to prioritize ECN enabled packets better, in light of dan siemon's
discovery that ECN + pfifo_fast was somewhat borked, over the weekend.

There's a very simple patch for pfifo_fast also being evaluated.

For reference to the problem being addressed, see:

http://www.coverfire.com/archives/2011/03/13/pfifo_fast-and-ecn/

And the thread on netdev...

Eric Dumazet <eric.dumazet@gmail.com> writes:

> Le mardi 15 mars 2011 à 19:09 +0200, Jonathan Morton a écrit :
>> On 15 Mar, 2011, at 8:15 am, Eric Dumazet wrote:
>> 
>> > band 0 : high priority packets (like now)
>> > band 1 : (old band 1, ECN capable flows)
>> > band 2 : (old band 1, no ECN flows)
>> > band 3 : low priority packets (old band 2)
>> 
>> This seems good to me.  It would provide a concrete (if minor) enticement to turn ECN on.
>> 
>>  
>
> Here is a patch to implement that, on top of net-next-2.6 git tree
>
> qdisc pfifo_fast 0: dev eth1 root refcnt 2 bands 4 priomap  2 1 3 3 2 3 0 0 2 2 2 2 2 2 2 2
>  Sent 168 bytes 2 pkt (dropped 0, overlimits 0 requeues 0) 
>  backlog 0b 0p requeues 0 
>
>
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index c84b659..95ddf54 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -412,19 +412,39 @@ static struct Qdisc noqueue_qdisc = {
>  };
>  
>  
> -static const u8 prio2band[TC_PRIO_MAX + 1] = {
> -	1, 2, 2, 2, 1, 2, 0, 0 , 1, 1, 1, 1, 1, 1, 1, 1
> +/* 4-band FIFO queue: old style, but should be a bit faster than
> +   generic prio+fifo combination.
> + */
> +
> +enum {
> +	BAND_HIGH_PRIO,
> +	BAND_MEDIUM_ECN_PRIO,
> +	BAND_MEDIUM_PRIO,
> +	BAND_LOW_PRIO,
> +
> +	PFIFO_FAST_BANDS
>  };
>  
> -/* 3-band FIFO queue: old style, but should be a bit faster than
> -   generic prio+fifo combination.
> +/*
> + * We give a litle incent to ECN flows
>   */
> +static const u8 prio2band[TC_PRIO_MAX + 1] = {
> +	[TC_PRIO_BESTEFFORT]	= BAND_MEDIUM_PRIO,
> +	[TC_PRIO_FILLER]	= BAND_MEDIUM_ECN_PRIO,
> +	[TC_PRIO_BULK]		= BAND_LOW_PRIO,
> +	[TC_PRIO_BULK + 1]	= BAND_LOW_PRIO,
> +	[TC_PRIO_INTERACTIVE_BULK] = BAND_MEDIUM_PRIO,
> +	[TC_PRIO_INTERACTIVE_BULK + 1] = BAND_LOW_PRIO,
> +	[TC_PRIO_INTERACTIVE]	= BAND_HIGH_PRIO,
> +	[TC_PRIO_CONTROL]	= BAND_HIGH_PRIO,
> +
> +	[TC_PRIO_CONTROL+1 ... TC_PRIO_MAX] = BAND_MEDIUM_PRIO,
> +};
>  
> -#define PFIFO_FAST_BANDS 3
>  
>  /*
>   * Private data for a pfifo_fast scheduler containing:
> - * 	- queues for the three band
> + * 	- queues for the four bands
>   * 	- bitmap indicating which of the bands contain skbs
>   */
>  struct pfifo_fast_priv {
> @@ -436,9 +456,13 @@ struct pfifo_fast_priv {
>   * Convert a bitmap to the first band number where an skb is queued, where:
>   * 	bitmap=0 means there are no skbs on any band.
>   * 	bitmap=1 means there is an skb on band 0.
> - *	bitmap=7 means there are skbs on all 3 bands, etc.
> + * 	bitmap=2 means there is an skb on band 1.
> + *	bitmap=15 means there are skbs on all 4 bands.
>   */
> -static const int bitmap2band[] = {-1, 0, 1, 0, 2, 0, 1, 0};
> +static const int bitmap2band[1 << PFIFO_FAST_BANDS] = {
> +	-1, 0, 1, 0, 2, 0, 1, 0,
> +	 3, 0, 1, 0, 2, 0, 1, 0
> +};
>  
>  static inline struct sk_buff_head *band2list(struct pfifo_fast_priv *priv,
>  					     int band)
>
>

-- 
Dave Taht
http://nex-6.taht.net