From: Corey Hickey <bugfood-ml@fatooh•org>
To: Patrick McHardy <kaber@trash•net>
Cc: netdev@vger•kernel.org
Subject: Re: [PATCH 1/7] Preparatory refactoring part 1.
Date: Mon, 30 Jul 2007 18:26:47 -0700 [thread overview]
Message-ID: <46AE8FD7.3010201@fatooh.org> (raw)
In-Reply-To: <46ADECC9.7010303@trash.net>
Patrick McHardy wrote:
>> -static int
>> -sfq_enqueue(struct sk_buff *skb, struct Qdisc* sch)
>> +static void sfq_q_enqueue(struct sk_buff *skb, struct sfq_sched_data *q, unsigned int end)
>
>
> Please make sure to break at 80 chars and to keep the style
> in this file consistent (newline before function name).
Ok. For what it's worth, though, most of the original functions in the
file don't have a newline before the function name. Omitting the newline
would thus make the new/changed functions more consistent with the
rest of the file. I don't have a preference either way, so unless you
change your mind I'll put the newline back in..
>> {
>> - struct sfq_sched_data *q = qdisc_priv(sch);
>> unsigned hash = sfq_hash(q, skb);
>> sfq_index x;
>>
>> @@ -256,8 +257,12 @@ sfq_enqueue(struct sk_buff *skb, struct Qdisc* sch)
>> q->ht[hash] = x = q->dep[SFQ_DEPTH].next;
>> q->hash[x] = hash;
>> }
>> - sch->qstats.backlog += skb->len;
>
> Why not keep this instead of having both callers do it?
My idea was to have all the sfq_q_* functions operate on "struct
sfq_sched_data" and have no knowledge of the "struct Qdisc". I did this
in order to be able to use the new functions in sfq_change() when the
temporary sfq_sched_data doesn't have a parent Qdisc.
There's probably a better way, and I am of course open to suggestions,
but what I did made sense to me.
>> - __skb_queue_tail(&q->qs[x], skb);
>> +
>> + if (end == SFQ_TAIL)
>> + __skb_queue_tail(&q->qs[x], skb);
>> + else
>> + __skb_queue_head(&q->qs[x], skb);
>> +
>> sfq_inc(q, x);
>> if (q->qs[x].qlen == 1) { /* The flow is new */
>> if (q->tail == SFQ_DEPTH) { /* It is the first flow */
>> @@ -270,12 +275,21 @@ sfq_enqueue(struct sk_buff *skb, struct Qdisc* sch)
>> q->tail = x;
>> }
>> }
>> +}
>> +
>> +static int
>> +sfq_enqueue(struct sk_buff *skb, struct Qdisc* sch)
>> +{
>> + struct sfq_sched_data *q = qdisc_priv(sch);
>
> newline please.
Ok.
>> + sfq_q_enqueue(skb, q, SFQ_TAIL);
>> + sch->qstats.backlog += skb->len;
>> if (++sch->q.qlen < q->limit-1) {
>> sch->bstats.bytes += skb->len;
>> sch->bstats.packets++;
>> return 0;
>> }
>>
>> + sch->qstats.drops++;
>
> sfq_drop already increments this.
You're right, of course. When I look at the original sfq_requeue(),
though, I see that same line right before sfq_drop(). That's probably
why it ended up in my patch. Is that a bug? The original sfq_enqueue()
doesn't have that line.
http://git.kernel.org/?p=linux/kernel/git/jgarzik/netdev-2.6.git;a=blob;f=net/sched/sch_sfq.c
line 314
>> sfq_drop(sch);
>> return NET_XMIT_CN;
>> }
>> @@ -284,28 +298,8 @@ static int
>> sfq_requeue(struct sk_buff *skb, struct Qdisc* sch)
>> {
>> struct sfq_sched_data *q = qdisc_priv(sch);
>
> newline please
Ok.
>> -static struct sk_buff *
>> -sfq_dequeue(struct Qdisc* sch)
>> +static struct sk_buff *sfq_q_dequeue(struct sfq_sched_data *q)
>
>
> Keep style consistent please.
Ok.
Thank you for the review. I'll address the rest of your comments when I
have time later.
-Corey
next prev parent reply other threads:[~2007-07-31 1:26 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2007-07-30 0:21 SFQ: backport some features from ESFQ (try 2) Corey Hickey
2007-07-30 0:21 ` [PATCH 1/7] Preparatory refactoring part 1 Corey Hickey
2007-07-30 13:51 ` Patrick McHardy
2007-07-31 1:26 ` Corey Hickey [this message]
2007-07-31 10:46 ` Patrick McHardy
2007-07-30 0:21 ` [PATCH 2/7] Preparatory refactoring part 2 Corey Hickey
2007-07-30 13:59 ` Patrick McHardy
2007-07-31 7:43 ` Corey Hickey
2007-07-30 0:21 ` [PATCH 3/7] Move two functions Corey Hickey
2007-07-30 0:21 ` [PATCH 4/7] Add "depth" Corey Hickey
2007-07-30 0:21 ` [PATCH 5/7] Add divisor Corey Hickey
2007-07-30 0:21 ` [PATCH 6/7] Make qdisc changeable Corey Hickey
2007-07-30 14:11 ` Patrick McHardy
2007-07-31 7:43 ` Corey Hickey
2007-08-06 2:47 ` Corey Hickey
2007-08-06 12:06 ` Patrick McHardy
2007-07-30 0:21 ` [PATCH 7/7] Remove comments about hardcoded values Corey Hickey
2007-07-30 0:21 ` [PATCH] [iproute2] SFQ: Support changing depth and divisor Corey Hickey
-- strict thread matches above, loose matches on Subject: below --
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
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=46AE8FD7.3010201@fatooh.org \
--to=bugfood-ml@fatooh$(echo .)org \
--cc=kaber@trash$(echo .)net \
--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