Re: [PATCH net-next 3/6] net: sched: RED: Introduce an ECN tail-dropping mode

[Petr Machata <petrm@mellanox.com>]

Jakub Kicinski <kuba@kernel.org> writes:

> On Mon,  9 Mar 2020 20:35:00 +0200 Ido Schimmel wrote:
>> diff --git a/net/sched/sch_red.c b/net/sched/sch_red.c
>> index f9839d68b811..d72db7643a37 100644
>> --- a/net/sched/sch_red.c
>> +++ b/net/sched/sch_red.c
>> @@ -44,7 +44,8 @@ struct red_sched_data {
>>  	struct Qdisc		*qdisc;
>>  };
>>
>> -#define RED_SUPPORTED_FLAGS (TC_RED_ECN | TC_RED_HARDDROP | TC_RED_ADAPTATIVE)
>> +#define RED_SUPPORTED_FLAGS (TC_RED_ECN | TC_RED_HARDDROP | \
>> +			     TC_RED_ADAPTATIVE | TC_RED_TAILDROP)
>>
>>  static inline int red_use_ecn(struct red_sched_data *q)
>>  {
>> @@ -56,6 +57,11 @@ static inline int red_use_harddrop(struct red_sched_data *q)
>>  	return q->flags & TC_RED_HARDDROP;
>>  }
>>
>> +static inline int red_use_taildrop(struct red_sched_data *q)
>
> Please don't do static inlines in C code, even if the neighboring code
> does.

Ok.

>> +{
>> +	return q->flags & TC_RED_TAILDROP;
>> +}
>> +
>>  static int red_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>>  		       struct sk_buff **to_free)
>>  {
>> @@ -76,23 +82,36 @@ static int red_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>>
>>  	case RED_PROB_MARK:
>>  		qdisc_qstats_overlimit(sch);
>> -		if (!red_use_ecn(q) || !INET_ECN_set_ce(skb)) {
>> +		if (!red_use_ecn(q)) {
>>  			q->stats.prob_drop++;
>>  			goto congestion_drop;
>>  		}
>>
>> -		q->stats.prob_mark++;
>> +		if (INET_ECN_set_ce(skb)) {
>> +			q->stats.prob_mark++;
>> +		} else if (red_use_taildrop(q)) {
>
> This condition is inverted, no?
>
> If user requested taildrop the packet should be queued.

Argh, yes! I was meddling with different ways of coding the if-else
after Ido reviewed it and screwed it up. Thanks for catching it.

My tests have not caught this, because they all hit the hard mark case
:(

>> +			q->stats.prob_drop++;
>> +			goto congestion_drop;
>> +		}
>> +
>> +		/* Non-ECT packet in ECN taildrop mode: queue it. */
>>  		break;
>>
>>  	case RED_HARD_MARK:
>>  		qdisc_qstats_overlimit(sch);
>> -		if (red_use_harddrop(q) || !red_use_ecn(q) ||
>> -		    !INET_ECN_set_ce(skb)) {
>> +		if (red_use_harddrop(q) || !red_use_ecn(q)) {
>> +			q->stats.forced_drop++;
>> +			goto congestion_drop;
>> +		}
>> +
>> +		if (INET_ECN_set_ce(skb)) {
>> +			q->stats.forced_mark++;
>> +		} else if (!red_use_taildrop(q)) {
>
> This one looks correct.
>
>>  			q->stats.forced_drop++;
>>  			goto congestion_drop;
>>  		}
>>
>> -		q->stats.forced_mark++;
>> +		/* Non-ECT packet in ECN taildrop mode: queue it. */
>>  		break;
>>  	}
>>