Re: [PATCH 1/7] Preparatory refactoring part 1.

[Corey Hickey <bugfood-ml@fatooh.org>]

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