shaper team forming up

shaper team forming up

[Dave Täht]

There are multiple people on these lists looking into existing and
potential shaper designs. I'd like it very much if we could co-ordinate
our efforts. I've had several folk volunteer their efforts, could use more.

Also...

Over the weekend, Dan Siemons uncovered a possible bad interaction
between ECN and the default pfifo_fast qdisc in Linux.

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

Perhaps some people could look into this, determine the extent that this
is a real problem (it seems very bad to me that acks would get tossed
into different queues based on interpreting mmc instead of ecn)

and figure out where the right place(s) are for a patch?

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

Re: [Bloat] shaper team forming up

[Jonathan Morton]

On 14 Mar, 2011, at 9:26 pm, Dave Täht wrote:

> Over the weekend, Dan Siemons uncovered a possible bad interaction
> between ECN and the default pfifo_fast qdisc in Linux.
> 
> http://www.coverfire.com/archives/2011/03/13/pfifo_fast-and-ecn/

This seems to be more complicated that it appears.  It looks as though Linux has re-used the LSB of the old TOS field for some "link local" flag which is used by routing.

It's not immediately obvious whether pfifo_fast is using this new interpretation though.  If it isn't, the fix should be to remove the RTO_ONLINK bit from the mask it's using on the tos field.  The other half of the mask correctly excludes the ECN bits from the field.

 - Jonathan

Re: [Bloat] shaper team forming up

[Eric Dumazet]

Le lundi 14 mars 2011 à 21:55 +0200, Jonathan Morton a écrit :
> On 14 Mar, 2011, at 9:26 pm, Dave Täht wrote:
> 
> > Over the weekend, Dan Siemons uncovered a possible bad interaction
> > between ECN and the default pfifo_fast qdisc in Linux.
> > 
> > http://www.coverfire.com/archives/2011/03/13/pfifo_fast-and-ecn/
> 
> This seems to be more complicated that it appears.  It looks as though
> Linux has re-used the LSB of the old TOS field for some "link local"
> flag which is used by routing.
> 
> It's not immediately obvious whether pfifo_fast is using this new
> interpretation though.  If it isn't, the fix should be to remove the
> RTO_ONLINK bit from the mask it's using on the tos field.  The other
> half of the mask correctly excludes the ECN bits from the field.
> 

CC netdev, where linux network dev can take a look.

I would say that this is a wrong analysis : 

1) ECN uses two low order bits of TOS byte

2) pfifo_fast uses skb->priority


skb->priority = rt_tos2priority(iph->tos);

#define IPTOS_TOS_MASK            0x1E
#define IPTOS_TOS(tos)            ((tos)&IPTOS_TOS_MASK)

static inline char rt_tos2priority(u8 tos)
{
	return ip_tos2prio[IPTOS_TOS(tos)>>1];
}

No interference between two mechanisms, unless sysadmin messed up things
(skb_edit)

Re: ECN + pfifo_fast borked?

[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