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-next>] [day] [month] [year] [list]
Date:	Sat, 28 Sep 2013 10:06:37 -0700
From:	Stephen Hemminger <stephen@...workplumber.org>
To:	Vijay Subramanian <subramanian.vijay@...il.com>
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH v1 net-next] net: pkt_sched: PIE AQM scheme

Thanks for submitting this, it is in ok shape, but I still
see several issues.

> diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
> index f2624b5..2fb6e6d 100644
> --- a/include/uapi/linux/pkt_sched.h
> +++ b/include/uapi/linux/pkt_sched.h
> @@ -787,4 +787,30 @@ struct tc_fq_qd_stats {
>         __u32   throttled_flows;
>         __u32   pad;
>  };
> +
> +/*PIE*/
> +enum {
> +       TCA_PIE_UNSPEC,
> +       TCA_PIE_TARGET,
> +       TCA_PIE_LIMIT,
> +       TCA_PIE_TUPDATE,
> +       TCA_PIE_ALPHA,
> +       TCA_PIE_BETA,
> +       TCA_PIE_ECN,
> +       TCA_PIE_BYTEMODE,
> +       __TCA_PIE_MAX
> +};
> +#define TCA_PIE_MAX   (__TCA_PIE_MAX - 1)
> +
> +struct tc_pie_xstats {
> +       __u32 prob;             /* current probability */
> +       __u32 delay;            /* current delay in ms */
> +       __u32 avg_dq_rate;      /* current average dq_rate in bits/pie_time */
> +       __u32 packets_in;       /* total number of packets enqueued */
> +       __u32 dropped;          /* packets dropped due to pie_action */
> +       __u32 overlimit;        /* dropped due to lack of space in queue */
> +       __u32 maxq;             /* maximum queue size */
> +       __u32 ecn_mark;         /* packets marked with ecn*/
> +};
> +
>  #endif
> diff --git a/net/sched/Kconfig b/net/sched/Kconfig
> index c03a32a..7b32e58 100644
> --- a/net/sched/Kconfig
> +++ b/net/sched/Kconfig
> @@ -286,6 +286,17 @@ config NET_SCH_FQ
> 
>           If unsure, say N.
> 
> +config NET_SCH_PIE
> +        tristate "Proportianal Enhanced Controller AQM (PIE)"

Spelling should be: Proportional 


> diff --git a/net/sched/sch_pie.c b/net/sched/sch_pie.c
> new file mode 100644
> index 0000000..cfcfde9
> --- /dev/null
> +++ b/net/sched/sch_pie.c
> @@ -0,0 +1,623 @@
> +/* Copyright (C) 2013 Cisco Systems, Inc, 2013.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301,
> + * USA.
> + *
> + * Author: Vijay Subramanian <vijaynsu@...co.com>
> + * Author: Mythili Prabhu <mysuryan@...co.com>
> + *
> + * ECN support is added by Naeem Khademi <naeemk@....uio.no>
> + * University of Oslo, Norway.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/skbuff.h>
> +#include <net/pkt_sched.h>
> +#include <net/inet_ecn.h>
> +
> +#define PIE_DEFAULT_QUEUE_LIMIT 200    /* in packets */
> +#define QUEUE_THRESHOLD (5000)
Useless paren's here

> +#define DQCOUNT_INVALID -1
> +#define THRESHOLD_PKT_SIZE     1500
> +#define MAX_INT_VALUE  0xffffffff
> +#define MAX_INT_VALUE_CAP  (0xffffffff >> 8)
> +
> +typedef u32 pie_time_t;
> +typedef s32 pie_tdiff_t;
> +#define PIE_SHIFT 10
> +#define MS2PIETIME(a) ((a * NSEC_PER_MSEC) >> PIE_SHIFT)
> +#define PIE_TIME_PER_SEC  ((NSEC_PER_SEC >> PIE_SHIFT))
> +

I would prefer that all packet schedulers use the same set of clock
routines (psched), rather than inventing own wrapper for high resolution
clock.

> +static inline pie_time_t pie_get_time(void)
> +{
> +       u64 ns = ktime_to_ns(ktime_get());
> +       return ns >> PIE_SHIFT;
> +}
> +
> +static inline u32 pie_time_to_ms(pie_time_t val)
> +{
> +       u64 valms = ((u64) val << PIE_SHIFT);
> +
> +       do_div(valms, NSEC_PER_MSEC);
> +       return (u32) valms;
> +}

Psched has all this.

> +/* parameters used*/
> +struct pie_params {
> +       pie_time_t target;      /* user specified target delay in pietime*/
> +       pie_time_t tupdate;     /* frequency with which the timer fires*/
                                                                         ^ space before end of comment
> +       u32 limit;              /* number of packets that can be enqueued */
> +       u32 alpha;              /* alpha and beta are between -4 and 4 */
> +       u32 beta;               /* and are used for shift relative to 1 */
> +       bool ecn;               /* true if ecn is enabled */
> +       bool bytemode;          /* to scale drop early prob based on pkt size */
> +};
> +
> +/* variables used*/
> +struct pie_vars {
> +       u32 prob;               /* probability but scaled by u32 limit. */
> +       pie_time_t burst_time;
> +       pie_time_t qdelay;
> +       pie_time_t qdelay_old;
> +       u32 dq_count;           /* measured in bytes */
> +       pie_time_t dq_tstamp;   /* drain rate */
> +       u32 avg_dq_rate;        /* bytes per pietime tick, scaled by 8 */
> +       u32 qlen_old;           /* in bytes */
> +};
> +
> +struct pie_stats {
> +       u32 packets_in;         /* total number of packets enqueued */
> +       u32 dropped;            /* packets dropped due to pie_action */
> +       u32 overlimit;          /* dropped due to lack of space in queue */
> +       u32 maxq;               /* maximum queue size */
> +       u32 ecn_mark;           /* packets marked with ECN */
> +};
> +
> +static void pie_params_init(struct pie_params *params)
> +{
> +       memset(params, 0, sizeof(*params));

Unnecessary, already zero'd when created by qdisc_alloc()
Call chain
      qdisc_create
	      qdisc_alloc
              ops->init => pie_init
                 pie_params_init
      
> +       params->alpha = 2;
> +       params->beta = 20;
> +       params->tupdate = MS2PIETIME(30);       /* 30 ms */
> +       params->limit = PIE_DEFAULT_QUEUE_LIMIT;
> +       params->target = MS2PIETIME(20);        /* 20 ms */
> +       params->ecn = false;
> +       params->bytemode = false;
> +}


> +
> +static void pie_vars_init(struct pie_vars *vars)
> +{
> +       memset(vars, 0, sizeof(*vars));
ditto, already zero'd

> +       vars->dq_count = DQCOUNT_INVALID;
> +       vars->avg_dq_rate = 0;
> +       /* default of 100 ms in pietime  */
> +       vars->burst_time = MS2PIETIME(100);
> +}
> +
> +static void pie_stats_init(struct pie_stats *stats)
> +{
> +       memset(stats, 0, sizeof(*stats));
> +}
> +
ditto, already zero'd

> +struct pie_sched_data {
> +       struct pie_params params;
> +       struct pie_vars vars;
> +       struct pie_stats stats;
> +       struct timer_list adapt_timer;
> +};

Standard practice is to put data structures before code.

> +
> +static inline bool drop_early(struct Qdisc *sch, u32 packet_size)

Inline is not needed if static. Let compiler decide.

> +{
> +       struct pie_sched_data *q = qdisc_priv(sch);
> +       u32 rnd;
> +       u32 local_prob = q->vars.prob;
> +
> +
> +       /* If there is still burst allowance left or delay is much below target
> +        * not due to heavy dropping, skip random early drop
> +        */
> +       if (q->vars.burst_time > 0)
> +               return false;
> +
> +       /* If current delay is less than half of target, and
> +        * if drop prob is low already, disable early_drop
> +        */
> +       if ((q->vars.qdelay < q->params.target / 2)
> +           && (q->vars.prob < MAX_INT_VALUE / 5))
> +               return false;
> +
> +       /* If we have fewer than 2 packets, disable drop_early,
> +        * similar to min_th in RED
> +        */
> +       if (sch->qstats.backlog < 2 * 1500)
> +               return false;
> +
> +       /* If bytemode is turned on, use packet size to compute new
> +        * probablity. Smaller packets will have lower drop prob in this case
> +        */
> +       if (q->params.bytemode) {
> +               /* If packet_size is greater than THRESHOLD_PKT_SIZE,
> +                * we cap the probability to the maximum value
> +                */
> +               if (packet_size <= THRESHOLD_PKT_SIZE) {
> +                       local_prob =
> +                           (local_prob / THRESHOLD_PKT_SIZE) * packet_size;
> +               } else {
> +                       local_prob = MAX_INT_VALUE_CAP;
> +               }
> +
> +       } else {
> +               local_prob = q->vars.prob;
> +       }
> +
> +       rnd = net_random() % MAX_INT_VALUE_CAP;
> +       if (rnd < local_prob)
> +               return true;
> +
> +       return false;
> +}
> +
> +static int pie_qdisc_enqueue(struct sk_buff *skb, struct Qdisc *sch)
> +{
> +       struct pie_sched_data *q = qdisc_priv(sch);
> +
> +       if (unlikely(qdisc_qlen(sch) >= sch->limit))
> +               goto out;
> +
> +       if (!drop_early(sch, skb->len)) {
> +               /* we can enqueue the packet */
> +               q->stats.packets_in++;
> +
> +               if (qdisc_qlen(sch) > q->stats.maxq)
> +                       q->stats.maxq = qdisc_qlen(sch);
> +
> +               return qdisc_enqueue_tail(skb, sch);
> +       } else if (q->params.ecn && INET_ECN_set_ce(skb) &&
> +                  (q->vars.prob <= MAX_INT_VALUE / 10)) {
> +                       /* If packet is ecn capable, mark it if drop probability
> +                        * is lower than 10%, else drop it.
> +                        */
> +                       q->stats.ecn_mark++;
> +                       return qdisc_enqueue_tail(skb, sch);
> +       }
> +out:
> +       q->stats.overlimit++;
> +       qdisc_drop(skb, sch);
> +       return NET_XMIT_CN;  /*indicate congestion*/
> +}
> +
> +static const struct nla_policy pie_policy[TCA_PIE_MAX + 1] = {
> +       [TCA_PIE_TARGET] = {.type = NLA_U32},
                                             ^ space before }
> +       [TCA_PIE_LIMIT] = {.type = NLA_U32},
> +       [TCA_PIE_TUPDATE] = {.type = NLA_U32},
> +       [TCA_PIE_ALPHA] = {.type = NLA_U32},
> +       [TCA_PIE_BETA] = {.type = NLA_U32},
> +       [TCA_PIE_ECN] = {.type = NLA_U32},
> +       [TCA_PIE_BYTEMODE] = {.type = NLA_U32},
              Looks prettier if all = are aligned

> +};
> +
> +static int pie_change(struct Qdisc *sch, struct nlattr *opt)
> +{
> +       struct pie_sched_data *q = qdisc_priv(sch);
> +       struct nlattr *tb[TCA_PIE_MAX + 1];
> +       unsigned int qlen;
> +       int err;
> +
> +       if (!opt)
> +               return -EINVAL;
> +
> +       err = nla_parse_nested(tb, TCA_PIE_MAX, opt, pie_policy);
> +       if (err < 0)
> +               return err;
> +
> +       sch_tree_lock(sch);
> +
> +       /* convert from microseconds to pietime */
> +       if (tb[TCA_PIE_TARGET]) {
> +               /* target is in us */
> +               u32 target = nla_get_u32(tb[TCA_PIE_TARGET]);
> +               /* convert to pietime */
> +               q->params.target = ((u64) target * NSEC_PER_USEC) >> PIE_SHIFT;
> +       }
> +
> +       if (tb[TCA_PIE_TUPDATE]) {
> +               /* tupdate is in us */
> +               u32 tupdate = nla_get_u32(tb[TCA_PIE_TUPDATE]);
> +               /* convert to pietime */
> +               q->params.tupdate =
> +                   ((u64) tupdate * NSEC_PER_USEC) >> PIE_SHIFT;
> +       }
> +
> +       if (tb[TCA_PIE_LIMIT]) {
> +               u32 limit = nla_get_u32(tb[TCA_PIE_LIMIT]);
> +               q->params.limit = limit;
> +               sch->limit = limit;
> +       }
> +
> +       if (tb[TCA_PIE_ALPHA])
> +               q->params.alpha = nla_get_u32(tb[TCA_PIE_ALPHA]);
> +
> +       if (tb[TCA_PIE_BETA])
> +               q->params.beta = nla_get_u32(tb[TCA_PIE_BETA]);
> +
> +       if (tb[TCA_PIE_ECN])
> +               q->params.ecn = nla_get_u32(tb[TCA_PIE_ECN]);
> +
> +       if (tb[TCA_PIE_BYTEMODE])
> +               q->params.bytemode = nla_get_u32(tb[TCA_PIE_BYTEMODE]);
> +
> +       /* Drop excess packets if new limit is lower */
> +       qlen = sch->q.qlen;
> +       while (sch->q.qlen > sch->limit) {
> +               struct sk_buff *skb = __skb_dequeue(&sch->q);
> +
> +               sch->qstats.backlog -= qdisc_pkt_len(skb);
> +               qdisc_drop(skb, sch);
> +       }
> +       qdisc_tree_decrease_qlen(sch, qlen - sch->q.qlen);
> +
> +       sch_tree_unlock(sch);
> +       return 0;
> +}
> +
> +static int pie_process_dequeue(struct Qdisc *sch, struct sk_buff *skb)
> +{
> +
> +       struct pie_sched_data *q = qdisc_priv(sch);
> +       int qlen = sch->qstats.backlog; /* current queue size in bytes */
> +
> +       /* If current queue is about 10 packets or more and dq_count is unset
> +        *  we have enough packets to calculate the drain rate. Save
> +        *  current time as dq_tstamp and start measurement cycle.
> +        */
> +
> +       if (qlen >= QUEUE_THRESHOLD && q->vars.dq_count == DQCOUNT_INVALID) {
> +               q->vars.dq_tstamp = pie_get_time();
> +               q->vars.dq_count = 0;
> +       }
> +
> +       /*  Calculate the average drain rate from this value.  If queue length
> +        *  has receded to a small value viz., <= QUEUE_THRESHOLD bytes,reset
> +        *  the dq_count to -1 as we don't have enough packets to calculate the
> +        *  drain rate anymore The following if block is entered only when we
> +        *  have a substantial queue built up (QUEUE_THRESHOLD bytes or more)
> +        *  and we calculate the drain rate for the threshold here.  dq_count is
> +        *  in bytes, time difference in pie_time, hence rate is in
> +        *  bytes/pie_time.
> +        */
> +
> +       if (q->vars.dq_count != DQCOUNT_INVALID) {

I prefer not having blank line after each block comment when it refers
directly to the code below (save screen space when reading).

> +
> +               q->vars.dq_count += skb->len;
> +
> +               if (q->vars.dq_count >= QUEUE_THRESHOLD) {
> +                       pie_time_t now = pie_get_time();
> +                       pie_tdiff_t dtime = now - q->vars.dq_tstamp;
> +                       u64 count = q->vars.dq_count << 3;  /* scale by 8*/

This shift doesn't do what you expect. Since dq_count is 32 bit,
the compiler does a 32 bit shift and then assigns it to count.
So either just make count 32 bit and skip all the expensive 64 bit divide,
or cast q->vars.dq_count to 64 bit first.

> +
> +                       if (dtime == 0)
> +                               return 0;
> +
> +                       /* dtime has overflowed */
> +                       if (dtime < 0)
> +                               dtime = -dtime;
> +
> +                       do_div(count, dtime);
> +
> +                       if (q->vars.avg_dq_rate == 0)
> +                               q->vars.avg_dq_rate = count;
> +                       else
> +                               q->vars.avg_dq_rate =
> +                                   (q->vars.avg_dq_rate -
> +                                    (q->vars.avg_dq_rate >> 3)) + (count >> 3);
> +
> +                       /* If the queue has receded below the threshold, we hold
> +                        * on to the last drain rate calculated, else we reset
> +                        * dq_count to 0 to re-enter the if block when the next
> +                        * packet is dequeued
> +                        */
> +
> +                       if (qlen < QUEUE_THRESHOLD)
> +                               q->vars.dq_count = DQCOUNT_INVALID;
> +                       else {
> +                               q->vars.dq_count = 0;
> +                               q->vars.dq_tstamp = pie_get_time();
> +                       }
> +
> +                       if (q->vars.burst_time > 0) {
> +                               if (q->vars.burst_time > dtime)
> +                                       q->vars.burst_time -= dtime;
> +                               else
> +                                       q->vars.burst_time = 0;
> +                       }
> +               }
> +
> +       }
> +       return 0;
> +}
> +
> +static void calculate_probability(struct Qdisc *sch)
> +{
> +       struct pie_sched_data *q = qdisc_priv(sch);
> +       int qlen = sch->qstats.backlog; /* queue size in bytes */
> +       pie_time_t qdelay = 0;  /* in pietime */
> +       pie_time_t qdelay_old = q->vars.qdelay; /* in pietime */
> +       s32 delta = 0;          /* signed difference */
> +       u32 oldprob;
> +       u32 alpha, beta;
> +       bool update_prob = true;        /* Should probability be updated? */
> +

Big problem, this is called without locks??
timer -> pie_timer -> calculate_probability

When you add locks you also have to worry about deadlock
on shutdown.

> +       q->vars.qdelay_old = q->vars.qdelay;
> +
> +       if (q->vars.avg_dq_rate > 0)
> +               qdelay = (qlen << 3) / q->vars.avg_dq_rate;
> +       else
> +               qdelay = 0;
> +
> +       /* If qdelay is zero and qlen is not, it means qlen is very small, less
> +        * than dequeue_rate, so we do not update probabilty in this round
> +        */
> +       if (qdelay == 0 && qlen != 0)
> +               update_prob = false;
> +
> +       /* Add ranges for alpha and beta, more aggressive for high dropping
> +        * mode and gentle steps for light dropping mode
> +        * In light dropping mode, take gentle steps; in medium dropping mode,
> +        * take medium steps; in high dropping mode, take big steps.
> +        */
> +       if (q->vars.prob < MAX_INT_VALUE / 100) {
> +               alpha =
> +                   (q->params.alpha * (MAX_INT_VALUE / PIE_TIME_PER_SEC)) >> 7;
> +               beta =
> +                   (q->params.beta * (MAX_INT_VALUE / PIE_TIME_PER_SEC)) >> 7;
> +       } else if (q->vars.prob < MAX_INT_VALUE / 10) {
> +               alpha =
> +                   (q->params.alpha * (MAX_INT_VALUE / PIE_TIME_PER_SEC)) >> 5;
> +               beta =
> +                   (q->params.beta * (MAX_INT_VALUE / PIE_TIME_PER_SEC)) >> 5;
> +       } else {
> +               alpha =
> +                   (q->params.alpha * (MAX_INT_VALUE / PIE_TIME_PER_SEC)) >> 4;
> +               beta =
> +                   (q->params.beta * (MAX_INT_VALUE / PIE_TIME_PER_SEC)) >> 4;
> +       }
> +
> +       /* alpha and beta should be between 0 and 32, in multiples of 1/16
> +        */
> +       delta += alpha * ((qdelay - q->params.target));
> +       delta += beta * ((qdelay - qdelay_old));
> +
> +       oldprob = q->vars.prob;
> +
> +       /* addition to ensure we increase probability in steps of no
> +        *  more than 2%
> +        */
> +
> +       if (delta > (s32) (MAX_INT_VALUE * 2 / 100)
> +           && q->vars.prob >= MAX_INT_VALUE / 10) {
> +               delta = MAX_INT_VALUE * 2 / 100;
> +       }
> +
> +       /*  Non-linear drop
> +        *  Tune drop probability to increase quickly for high delays
> +        *  (250ms and above)
> +        *  250ms is derived through experiments and provides error protection
> +        */
> +
> +       if (qdelay > (MS2PIETIME(250)))
> +               delta += (2 * MAX_INT_VALUE) / 100;
> +
> +       q->vars.prob += delta;
> +
> +       if (delta > 0) {
> +               /* prevent overflow */
> +               if (q->vars.prob < oldprob) {
> +                       q->vars.prob = MAX_INT_VALUE;
> +                       /* Prevent normalization error
> +                        * If probability is the maximum value already,
> +                        * we normalize it here, and skip the
> +                        * check to do a non-linear drop in the next section
> +                        */
> +                       update_prob = false;
> +               }
> +       } else {
> +               /* prevent underflow */
> +               if (q->vars.prob > oldprob)
> +                       q->vars.prob = 0;
> +       }
> +
> +       /* Non-linear drop in probability */
> +       /* Reduce drop probability quickly if delay is 0 for 2 consecutive
> +        * Tupdate periods
> +        */
> +       if ((qdelay == 0) && (qdelay_old == 0) && update_prob)
> +               q->vars.prob = (q->vars.prob * 98) / 100;
> +
> +       q->vars.qdelay = qdelay;
> +       q->vars.qlen_old = qlen;
> +
> +       /* we restart the measurement cycle if the following conditions are met
> +        *  1. If the delay has been low for 2 consecutive Tupdate periods
> +        *  2. Calculated drop probability is zero
> +        *  3. We have atleast one estimate for the avg_dq_rate ie.,
> +        *     is a non-zero value
> +        */
> +       if ((q->vars.qdelay < q->params.target / 2)
> +           && (q->vars.qdelay_old < q->params.target / 2)
> +           && (q->vars.prob == 0)
> +           && q->vars.avg_dq_rate > 0)
> +               pie_vars_init(&q->vars);
> +
> +       return;
> +}

Don't do empty return at end of function. It is unnecessary.


> +
> +static inline void pie_timer(unsigned long arg)

Since this is a callback it can't be inlined anyway

> +{
> +       struct Qdisc *sch = (struct Qdisc *)arg;
> +       struct pie_sched_data *q = qdisc_priv(sch);
> +       u64 tup;
Does this really have to be 64 bit?

> +
> +       calculate_probability(sch);
> +       /* reset the timer to fire after 'tupdate' us,
> +        * tupdate is currently in pie_time
> +        * mod_timer expects time to be in jiffies
> +        */
> +       /* convert from pietime to nsecs to ms*/
> +       tup = pie_time_to_ms(q->params.tupdate);
> +       tup = (tup * HZ) / (1000);      /* and then to  jiffies */

Useless paren around (1000)

> +
> +       mod_timer(&q->adapt_timer, jiffies + tup);
> +
> +       return;
> +
> +}
> +
> +static int pie_init(struct Qdisc *sch, struct nlattr *opt)
> +{
> +       struct pie_sched_data *q = qdisc_priv(sch);
> +
> +       pie_params_init(&q->params);
> +       pie_vars_init(&q->vars);
> +       pie_stats_init(&q->stats);
> +       sch->limit = q->params.limit;
> +       setup_timer(&q->adapt_timer, pie_timer, (unsigned long)sch);
> +       add_timer(&q->adapt_timer);

This is wrong, you haven't set expiration timer (will be 0), so timer
will fire.

> +
> +       if (opt) {
> +               int err = pie_change(sch, opt);
> +
> +               if (err)
> +                       return err;
> +       }
> +
> +       return 0;
> +}
> +
> +static int pie_dump(struct Qdisc *sch, struct sk_buff *skb)
> +{
> +       struct pie_sched_data *q = qdisc_priv(sch);
> +       struct nlattr *opts;
> +
> +       opts = nla_nest_start(skb, TCA_OPTIONS);
> +       if (opts == NULL)
> +               goto nla_put_failure;
> +
> +       /* convert target and tupdate from pietime to us */
> +       if (nla_put_u32(skb, TCA_PIE_TARGET,
> +                       pie_time_to_ms(q->params.target) * 1000L) ||
> +           nla_put_u32(skb, TCA_PIE_LIMIT,
> +                       sch->limit) ||
> +           nla_put_u32(skb, TCA_PIE_TUPDATE,
> +                       pie_time_to_ms(q->params.tupdate) * 1000L) ||
> +           nla_put_u32(skb, TCA_PIE_ALPHA,
> +                       q->params.alpha) ||
> +           nla_put_u32(skb, TCA_PIE_BETA, q->params.beta) ||
> +           nla_put_u32(skb, TCA_PIE_ECN, q->params.ecn) ||
> +           nla_put_u32(skb, TCA_PIE_BYTEMODE, q->params.bytemode))
> +               goto nla_put_failure;
> +
> +       return nla_nest_end(skb, opts);
> +
> +nla_put_failure:
> +       nla_nest_cancel(skb, opts);
> +       return -1;
> +
> +}
> +
> +static int pie_dump_stats(struct Qdisc *sch, struct gnet_dump *d)
> +{
> +       struct pie_sched_data *q = qdisc_priv(sch);
> +       struct tc_pie_xstats st = {
> +               .prob = q->vars.prob,
> +               .delay = pie_time_to_ms(q->vars.qdelay) * 1000, /* in us*/
> +               /* unscale and return dq_rate in bytes per sec*/
> +               .avg_dq_rate = q->vars.avg_dq_rate * (PIE_TIME_PER_SEC/8),
> +               .packets_in = q->stats.packets_in,
> +               .overlimit = q->stats.overlimit,
> +               .maxq = q->stats.maxq,
> +               .dropped = q->stats.dropped,
> +               .ecn_mark = q->stats.ecn_mark,
> +       };
Personal preference, I prefer a aligned initialization style.

> +
> +       return gnet_stats_copy_app(d, &st, sizeof(st));
> +}
> +
> +static inline struct sk_buff *pie_qdisc_dequeue(struct Qdisc *sch)
> +{
> +       struct sk_buff *skb;
> +       skb = __qdisc_dequeue_head(sch, &sch->q);
> +
> +       if (!skb)
> +               return NULL;
> +
> +       pie_process_dequeue(sch, skb);
> +
> +       return skb;
> +}
> +
> +static void pie_reset(struct Qdisc *sch)
> +{
> +       struct pie_sched_data *q = qdisc_priv(sch);
> +       qdisc_reset_queue(sch);
> +       pie_vars_init(&q->vars);
> +
> +       return;
> +}
> +
> +static void pie_destroy(struct Qdisc *sch)
> +{
> +       struct pie_sched_data *q = qdisc_priv(sch);
> +
> +       del_timer_sync(&q->adapt_timer);
> +}
> +
> +static struct Qdisc_ops pie_qdisc_ops __read_mostly = {
> +       .id = "pie",
> +       .priv_size = sizeof(struct pie_sched_data),
> +
> +       .enqueue = pie_qdisc_enqueue,
> +       .dequeue = pie_qdisc_dequeue,
> +       .peek = qdisc_peek_dequeued,
> +       .init = pie_init,
> +       .destroy = pie_destroy,
> +       .reset = pie_reset,
> +       .change = pie_change,
> +       .dump = pie_dump,
> +       .dump_stats = pie_dump_stats,
> +       .owner = THIS_MODULE,
> +};
> +
> +static int __init pie_module_init(void)
> +{
> +       return register_qdisc(&pie_qdisc_ops);
> +}
> +
> +static void __exit pie_module_exit(void)
> +{
> +       unregister_qdisc(&pie_qdisc_ops);
> +}
> +
> +module_init(pie_module_init);
> +module_exit(pie_module_exit);
> +
> +MODULE_DESCRIPTION
> +       ("PIE (Proportional Intergal controller Enhanced) scheduler");
Keep this on one line, ignore any complaints from checkpatch about it.

> +MODULE_AUTHOR("Vijay Subramanian");
> +MODULE_AUTHOR("Mythili Prabhu");
> +MODULE_LICENSE("GPL");

Please fix and resubmit. This is just a first pass review, there are
probably more detailed issues that others will see.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists