| Message ID | 1444675083-9825-2-git-send-email-xiyou.wangcong@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Cong Wang <xiyou.wangcong@gmail.com> Date: Mon, 12 Oct 2015 11:38:00 -0700 > Remove nearly duplicated code and prepare for the following patch. > > Cc: Jamal Hadi Salim <jhs@mojatatu.com> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> 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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/12/15 14:38, Cong Wang wrote: > Remove nearly duplicated code and prepare for the following patch. > Cong - like Dave, I dont see equivalence in some of these changes. Example not sure how the qfq grafting invocation of qfq_purge_queue fits in. There are a few others. cheers, jamal -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 13, 2015 at 6:54 PM, David Miller <davem@davemloft.net> wrote: > From: Cong Wang <xiyou.wangcong@gmail.com> > Date: Mon, 12 Oct 2015 11:38:00 -0700 > >> Remove nearly duplicated code and prepare for the following patch. >> >> Cc: Jamal Hadi Salim <jhs@mojatatu.com> >> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 14, 2015 at 4:56 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote: > On 10/12/15 14:38, Cong Wang wrote: >> >> Remove nearly duplicated code and prepare for the following patch. >> > > > Cong - like Dave, I dont see equivalence in some of these > changes. > Example not sure how the qfq grafting invocation of > qfq_purge_queue fits in. There are a few others. drr_purge_queue() and qfq_purge_queue() are both qdisc_reset() + qdisc_tree_decrease_qlen(): 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); } static void qfq_purge_queue(struct qfq_class *cl) { unsigned int len = cl->qdisc->q.qlen; qdisc_reset(cl->qdisc); qdisc_tree_decrease_qlen(cl->qdisc, len); } Or you mean the order of calling them?? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/15/15 00:15, Cong Wang wrote: > On Wed, Oct 14, 2015 at 4:56 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote: >> On 10/12/15 14:38, Cong Wang wrote: >>> >>> Remove nearly duplicated code and prepare for the following patch. >>> >> >> >> Cong - like Dave, I dont see equivalence in some of these >> changes. >> Example not sure how the qfq grafting invocation of >> qfq_purge_queue fits in. There are a few others. > > drr_purge_queue() and qfq_purge_queue() are both > qdisc_reset() + qdisc_tree_decrease_qlen(): > > > 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); > } > > static void qfq_purge_queue(struct qfq_class *cl) > { > unsigned int len = cl->qdisc->q.qlen; > > qdisc_reset(cl->qdisc); > qdisc_tree_decrease_qlen(cl->qdisc, len); > } > > Or you mean the order of calling them?? > I dont think the order is as important (although the way you have it seems to be the sanest). Thanks for clarifying Cong. I took a closer look and all looks good. Acked-by: Jamal Hadi Salim <jhs@mojatatu.com> cheers, jamal -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h index 4c79ce8..0ad4dcf 100644 --- a/include/net/sch_generic.h +++ b/include/net/sch_generic.h @@ -695,6 +695,23 @@ static inline void qdisc_reset_queue(struct Qdisc *sch) sch->qstats.backlog = 0; } +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; +} + static inline unsigned int __qdisc_queue_drop(struct Qdisc *sch, struct sk_buff_head *list) { diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c index c538d9e..7f8474c 100644 --- a/net/sched/sch_cbq.c +++ b/net/sched/sch_cbq.c @@ -1624,13 +1624,8 @@ static int cbq_graft(struct Qdisc *sch, unsigned long arg, struct Qdisc *new, new->reshape_fail = cbq_reshape_fail; #endif } - sch_tree_lock(sch); - *old = cl->q; - cl->q = new; - qdisc_tree_decrease_qlen(*old, (*old)->q.qlen); - qdisc_reset(*old); - sch_tree_unlock(sch); + *old = qdisc_replace(sch, new, &cl->q); return 0; } 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; } diff --git a/net/sched/sch_dsmark.c b/net/sched/sch_dsmark.c index f357f34..cfddb1c 100644 --- a/net/sched/sch_dsmark.c +++ b/net/sched/sch_dsmark.c @@ -73,13 +73,7 @@ static int dsmark_graft(struct Qdisc *sch, unsigned long arg, new = &noop_qdisc; } - sch_tree_lock(sch); - *old = p->q; - p->q = new; - qdisc_tree_decrease_qlen(*old, (*old)->q.qlen); - qdisc_reset(*old); - sch_tree_unlock(sch); - + *old = qdisc_replace(sch, new, &p->q); return 0; } diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c index b7ebe2c..089f3b6 100644 --- a/net/sched/sch_hfsc.c +++ b/net/sched/sch_hfsc.c @@ -1215,11 +1215,7 @@ hfsc_graft_class(struct Qdisc *sch, unsigned long arg, struct Qdisc *new, new = &noop_qdisc; } - sch_tree_lock(sch); - hfsc_purge_queue(sch, cl); - *old = cl->qdisc; - cl->qdisc = new; - sch_tree_unlock(sch); + *old = qdisc_replace(sch, new, &cl->qdisc); return 0; } diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c index 15ccd7f..0efbcf3 100644 --- a/net/sched/sch_htb.c +++ b/net/sched/sch_htb.c @@ -1163,14 +1163,7 @@ static int htb_graft(struct Qdisc *sch, unsigned long arg, struct Qdisc *new, cl->common.classid)) == NULL) return -ENOBUFS; - sch_tree_lock(sch); - *old = cl->un.leaf.q; - cl->un.leaf.q = new; - if (*old != NULL) { - qdisc_tree_decrease_qlen(*old, (*old)->q.qlen); - qdisc_reset(*old); - } - sch_tree_unlock(sch); + *old = qdisc_replace(sch, new, &cl->un.leaf.q); return 0; } diff --git a/net/sched/sch_multiq.c b/net/sched/sch_multiq.c index 4e904ca..a0103a1 100644 --- a/net/sched/sch_multiq.c +++ b/net/sched/sch_multiq.c @@ -303,13 +303,7 @@ static int multiq_graft(struct Qdisc *sch, unsigned long arg, struct Qdisc *new, if (new == NULL) new = &noop_qdisc; - sch_tree_lock(sch); - *old = q->queues[band]; - q->queues[band] = new; - qdisc_tree_decrease_qlen(*old, (*old)->q.qlen); - qdisc_reset(*old); - sch_tree_unlock(sch); - + *old = qdisc_replace(sch, new, &q->queues[band]); return 0; } diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c index 5abd1d9..0a6ddaf 100644 --- a/net/sched/sch_netem.c +++ b/net/sched/sch_netem.c @@ -1037,15 +1037,7 @@ static int netem_graft(struct Qdisc *sch, unsigned long arg, struct Qdisc *new, { struct netem_sched_data *q = qdisc_priv(sch); - sch_tree_lock(sch); - *old = q->qdisc; - q->qdisc = new; - if (*old) { - qdisc_tree_decrease_qlen(*old, (*old)->q.qlen); - qdisc_reset(*old); - } - sch_tree_unlock(sch); - + *old = qdisc_replace(sch, new, &q->qdisc); return 0; } diff --git a/net/sched/sch_prio.c b/net/sched/sch_prio.c index ba6487f..1b4aaec 100644 --- a/net/sched/sch_prio.c +++ b/net/sched/sch_prio.c @@ -268,13 +268,7 @@ static int prio_graft(struct Qdisc *sch, unsigned long arg, struct Qdisc *new, if (new == NULL) new = &noop_qdisc; - sch_tree_lock(sch); - *old = q->queues[band]; - q->queues[band] = new; - qdisc_tree_decrease_qlen(*old, (*old)->q.qlen); - qdisc_reset(*old); - sch_tree_unlock(sch); - + *old = qdisc_replace(sch, new, &q->queues[band]); return 0; } diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c index 3dc3a6e..b5c52ca 100644 --- a/net/sched/sch_qfq.c +++ b/net/sched/sch_qfq.c @@ -617,11 +617,7 @@ static int qfq_graft_class(struct Qdisc *sch, unsigned long arg, new = &noop_qdisc; } - sch_tree_lock(sch); - qfq_purge_queue(cl); - *old = cl->qdisc; - cl->qdisc = new; - sch_tree_unlock(sch); + *old = qdisc_replace(sch, new, &cl->qdisc); return 0; } diff --git a/net/sched/sch_red.c b/net/sched/sch_red.c index 6c0534c..d5abcee 100644 --- a/net/sched/sch_red.c +++ b/net/sched/sch_red.c @@ -313,12 +313,7 @@ static int red_graft(struct Qdisc *sch, unsigned long arg, struct Qdisc *new, if (new == NULL) new = &noop_qdisc; - sch_tree_lock(sch); - *old = q->qdisc; - q->qdisc = new; - qdisc_tree_decrease_qlen(*old, (*old)->q.qlen); - qdisc_reset(*old); - sch_tree_unlock(sch); + *old = qdisc_replace(sch, new, &q->qdisc); return 0; } diff --git a/net/sched/sch_sfb.c b/net/sched/sch_sfb.c index 5bbb633..0e74e55 100644 --- a/net/sched/sch_sfb.c +++ b/net/sched/sch_sfb.c @@ -606,12 +606,7 @@ static int sfb_graft(struct Qdisc *sch, unsigned long arg, struct Qdisc *new, if (new == NULL) new = &noop_qdisc; - sch_tree_lock(sch); - *old = q->qdisc; - q->qdisc = new; - qdisc_tree_decrease_qlen(*old, (*old)->q.qlen); - qdisc_reset(*old); - sch_tree_unlock(sch); + *old = qdisc_replace(sch, new, &q->qdisc); return 0; } diff --git a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c index a4afde1..56a1aef 100644 --- a/net/sched/sch_tbf.c +++ b/net/sched/sch_tbf.c @@ -502,13 +502,7 @@ static int tbf_graft(struct Qdisc *sch, unsigned long arg, struct Qdisc *new, if (new == NULL) new = &noop_qdisc; - sch_tree_lock(sch); - *old = q->qdisc; - q->qdisc = new; - qdisc_tree_decrease_qlen(*old, (*old)->q.qlen); - qdisc_reset(*old); - sch_tree_unlock(sch); - + *old = qdisc_replace(sch, new, &q->qdisc); return 0; }
Remove nearly duplicated code and prepare for the following patch. Cc: Jamal Hadi Salim <jhs@mojatatu.com> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> --- include/net/sch_generic.h | 17 +++++++++++++++++ net/sched/sch_cbq.c | 7 +------ net/sched/sch_drr.c | 6 +----- net/sched/sch_dsmark.c | 8 +------- net/sched/sch_hfsc.c | 6 +----- net/sched/sch_htb.c | 9 +-------- net/sched/sch_multiq.c | 8 +------- net/sched/sch_netem.c | 10 +--------- net/sched/sch_prio.c | 8 +------- net/sched/sch_qfq.c | 6 +----- net/sched/sch_red.c | 7 +------ net/sched/sch_sfb.c | 7 +------ net/sched/sch_tbf.c | 8 +------- 13 files changed, 29 insertions(+), 78 deletions(-)