On Tue, Oct 13, 2015 at 6:54 PM, David Miller <[email protected]> wrote:
> From: Cong Wang <[email protected]>
> Date: Mon, 12 Oct 2015 11:38:00 -0700
>
>> Remove nearly duplicated code and prepare for the following patch.
>>
>> Cc: Jamal Hadi Salim <[email protected]>
>> Signed-off-by: Cong Wang <[email protected]>
>
> This isn't an equivalent transformation:
>
>> +static inline struct Qdisc *qdisc_replace(struct Qdisc *sch, struct Qdisc
>> *new,
>> + struct Qdisc **pold)
>> +{
>> + struct Qdisc *old;
>> +
>> + sch_tree_lock(sch);
>> + old = *pold;
>> + *pold = new;
>> + if (old != NULL) {
>> + qdisc_tree_decrease_qlen(old, old->q.qlen);
>> + qdisc_reset(old);
>> + }
>> + sch_tree_unlock(sch);
>> +
>> + return old;
>> +}
>> +
>
> Is not the same as:
>
>> diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c
>> index f26bdea..c76cdd4 100644
>> --- a/net/sched/sch_drr.c
>> +++ b/net/sched/sch_drr.c
>> @@ -226,11 +226,7 @@ static int drr_graft_class(struct Qdisc *sch, unsigned
>> long arg,
>> new = &noop_qdisc;
>> }
>>
>> - sch_tree_lock(sch);
>> - drr_purge_queue(cl);
>> - *old = cl->qdisc;
>> - cl->qdisc = new;
>> - sch_tree_unlock(sch);
>> + *old = qdisc_replace(sch, new, &cl->qdisc);
>> return 0;
>> }
>>
>
> This.
>
> If you want to change semantics, you must do it explicitly in a separate
> commit with a detailed commit message explaining how and why.
If you meant drr_purge_queue(), it is same:
static void drr_purge_queue(struct drr_class *cl)
{
unsigned int len = cl->qdisc->q.qlen;
qdisc_reset(cl->qdisc);
qdisc_tree_decrease_qlen(cl->qdisc, len);
}
Or if you mean the 'if', always having one if doesn't harm, do it?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
