From: Michael Buesch <mb@bu3sch•de>
To: Corey Hickey <bugfood-ml@fatooh•org>
Cc: netdev@vger•kernel.org
Subject: Re: [PATCH 4/7] Add "depth".
Date: Sun, 29 Jul 2007 20:41:45 +0200 [thread overview]
Message-ID: <200707292041.45495.mb@bu3sch.de> (raw)
In-Reply-To: <11856929362692-git-send-email-bugfood-ml@fatooh.org>
On Sunday 29 July 2007 09:08:51 Corey Hickey wrote:
> p = d;
> n = q->dep[d].next;
> @@ -215,7 +216,7 @@ static unsigned int sfq_drop(struct Qdisc *sch)
> drop a packet from it */
>
> if (d > 1) {
> - sfq_index x = q->dep[d+SFQ_DEPTH].next;
> + sfq_index x = q->dep[d+q->depth].next;
Please q->dep[d + q->depth]
Makes it _much_ more readable. And doesn't confuse my brain with a
minus and a BiggerThan sign ;)
> @@ -383,6 +384,16 @@ static void sfq_perturbation(unsigned long arg)
> static void sfq_q_destroy(struct sfq_sched_data *q)
> {
> del_timer(&q->perturb_timer);
> + if(q->dep)
> + kfree(q->dep);
> + if(q->next)
> + kfree(q->next);
> + if(q->allot)
> + kfree(q->allot);
> + if(q->hash)
> + kfree(q->hash);
> + if(q->qs)
> + kfree(q->qs);
No need to check for !=NULL. kfree handles NULL.
> }
>
> static void sfq_destroy(struct Qdisc *sch)
> @@ -394,6 +405,7 @@ static void sfq_destroy(struct Qdisc *sch)
> static int sfq_q_init(struct sfq_sched_data *q, struct rtattr *opt)
> {
> struct tc_sfq_qopt *ctl = RTA_DATA(opt);
> + sfq_index p = ~0U/2;
> int i;
>
> if (opt && opt->rta_len < RTA_LENGTH(sizeof(*ctl)))
> @@ -401,30 +413,53 @@ static int sfq_q_init(struct sfq_sched_data *q, struct rtattr *opt)
>
> q->perturbation = 0;
> q->max_depth = 0;
> - q->tail = q->limit = SFQ_DEPTH;
> if (opt == NULL) {
> q->perturb_period = 0;
> + q->tail = q->limit = q->depth = SFQ_DEPTH_DEFAULT;
> } else {
> struct tc_sfq_qopt *ctl = RTA_DATA(opt);
> if (ctl->quantum)
> q->quantum = ctl->quantum;
> q->perturb_period = ctl->perturb_period*HZ;
> + q->tail = q->limit = q->depth = ctl->flows ? : SFQ_DEPTH_DEFAULT;
> +
> + if (q->depth > p - 1)
> + return -EINVAL;
Compare depth against (~0U/2)-1? What's that doing? Should probably add a comment.
>
> if (ctl->limit)
> - q->limit = min_t(u32, ctl->limit, SFQ_DEPTH);
> + q->limit = min_t(u32, ctl->limit, q->depth);
> }
>
> + q->dep = kmalloc((1+q->depth*2)*sizeof(struct sfq_head), GFP_KERNEL);
> + if (!q->dep)
> + goto err_case;
> + q->next = kmalloc(q->depth*sizeof(sfq_index), GFP_KERNEL);
> + if (!q->next)
> + goto err_case;
> + q->allot = kmalloc(q->depth*sizeof(short), GFP_KERNEL);
> + if (!q->allot)
> + goto err_case;
> + q->hash = kmalloc(q->depth*sizeof(unsigned short), GFP_KERNEL);
> + if (!q->hash)
> + goto err_case;
> + q->qs = kmalloc(q->depth*sizeof(struct sk_buff_head), GFP_KERNEL);
> + if (!q->qs)
> + goto err_case;
You may chose to use kcalloc for array allocations.
> for (i=0; i<SFQ_HASH_DIVISOR; i++)
> - q->ht[i] = SFQ_DEPTH;
> - for (i=0; i<SFQ_DEPTH; i++) {
> + q->ht[i] = q->depth;
> + for (i=0; i<q->depth; i++) {
> skb_queue_head_init(&q->qs[i]);
> - q->dep[i+SFQ_DEPTH].next = i+SFQ_DEPTH;
> - q->dep[i+SFQ_DEPTH].prev = i+SFQ_DEPTH;
> + q->dep[i+q->depth].next = i+q->depth;
> + q->dep[i+q->depth].prev = i+q->depth;
> }
>
> - for (i=0; i<SFQ_DEPTH; i++)
> + for (i=0; i<q->depth; i++)
> sfq_link(q, i);
> return 0;
> +err_case:
This leaks a few kmallocs.
> + sfq_q_destroy(q);
> + return -ENOBUFS;
> }
>
> static int sfq_init(struct Qdisc *sch, struct rtattr *opt)
> @@ -458,7 +493,7 @@ static int sfq_dump(struct Qdisc *sch, struct sk_buff *skb)
>
> opt.limit = q->limit;
> opt.divisor = SFQ_HASH_DIVISOR;
> - opt.flows = q->limit;
> + opt.flows = q->depth;
>
> RTA_PUT(skb, TCA_OPTIONS, sizeof(opt), &opt);
>
--
Greetings Michael.
next prev parent reply other threads:[~2007-07-29 18:41 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2007-07-29 7:08 [PATCH 0/7] SFQ: backport some features from ESFQ Corey Hickey
2007-07-29 7:08 ` [PATCH 1/7] Preparatory refactoring part 1 Corey Hickey
2007-07-29 7:08 ` [PATCH 2/7] Preparatory refactoring part 2 Corey Hickey
2007-07-29 7:08 ` [PATCH 3/7] Move two functions Corey Hickey
2007-07-29 7:08 ` [PATCH 4/7] Add "depth" Corey Hickey
2007-07-29 7:08 ` [PATCH 5/7] Add divisor Corey Hickey
2007-07-29 7:08 ` [PATCH 6/7] Make qdisc changeable Corey Hickey
2007-07-29 7:08 ` [PATCH 7/7] Remove comments about hardcoded values Corey Hickey
2007-07-29 7:08 ` [PATCH] [iproute2] SFQ: Support changing depth and divisor Corey Hickey
2007-07-29 18:41 ` Michael Buesch [this message]
2007-07-29 20:21 ` [PATCH 4/7] Add "depth" Corey Hickey
2007-07-29 20:54 ` Michael Buesch
2007-07-29 7:17 ` [PATCH 0/7] SFQ: backport some features from ESFQ Corey Hickey
2007-07-29 7:19 ` David Miller
-- strict thread matches above, loose matches on Subject: below --
2007-07-30 0:21 SFQ: backport some features from ESFQ (try 2) Corey Hickey
2007-07-30 0:21 ` [PATCH 4/7] Add "depth" Corey Hickey
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=200707292041.45495.mb@bu3sch.de \
--to=mb@bu3sch$(echo .)de \
--cc=bugfood-ml@fatooh$(echo .)org \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox