lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 9 Mar 2020 15:12:55 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Ido Schimmel <idosch@...sch.org>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, jiri@...lanox.com,
        petrm@...lanox.com, jhs@...atatu.com, xiyou.wangcong@...il.com,
        mlxsw@...lanox.com, Ido Schimmel <idosch@...lanox.com>
Subject: Re: [PATCH net-next 3/6] net: sched: RED: Introduce an ECN
 tail-dropping mode

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.

> +{
> +	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.

> +			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;
>  	}
>  

Powered by blists - more mailing lists