On Wed, Sep 16, 2020 at 11:05 PM Xiaoyong Yan <yanxiaoyo...@gmail.com> wrote: > > in the function cbs_dequeue_soft, when q->credits <0, [now - q->last] > should be accounted for sendslope,not idleslope. > > so the solution as follows: when q->credits is less than 0, directly > calculate delay time, activate hrtimer and when hrtimer fires, > calculate idleslope credits and update it to q->credits. > > because of the lack of self-defined qdisc_watchdog_func, so in the > generic sch_api, add qdisc_watchdog_init_func and > qdisc_watchdog_init_clockid_func, so sch_cbs can use it to define its > own process.
You do not have to define them as generic API, you can just use hrtimer API directly in sch_cbs, as it is the only user within net/sched/. Hopefully this would reduce the size of your patch too. > > the patch is changed based on v5.4.42,and the test result as follows: > the NIC is 100Mb/s ,full duplex. > > step1: > tc qdisc add dev ens33 root cbs idleslope 75 sendslope -25 hicredit 1000000 > locredit -1000000 offload 0 > step2: > root@ubuntu:/home/yxy/kernel/linux-stable# iperf -c 192.168.1.114 -i 1 > ------------------------------------------------------------ > Client connecting to 192.168.1.114, TCP port 5001 > TCP window size: 246 KByte (default) > ------------------------------------------------------------ > [ 3] local 192.168.1.120 port 42004 connected with 192.168.1.114 port 5001 > [ ID] Interval Transfer Bandwidth > [ 3] 0.0- 1.0 sec 9.00 MBytes 75.5 Mbits/sec > [ 3] 1.0- 2.0 sec 8.50 MBytes 71.3 Mbits/sec > [ 3] 2.0- 3.0 sec 8.50 MBytes 71.3 Mbits/sec > [ 3] 3.0- 4.0 sec 8.38 MBytes 70.3 Mbits/sec > [ 3] 4.0- 5.0 sec 8.38 MBytes 70.3 Mbits/sec > [ 3] 5.0- 6.0 sec 8.50 MBytes 71.3 Mbits/sec > [ 3] 6.0- 7.0 sec 8.50 MBytes 71.3 Mbits/sec > [ 3] 7.0- 8.0 sec 8.62 MBytes 72.4 Mbits/sec > [ 3] 8.0- 9.0 sec 8.50 MBytes 71.3 Mbits/sec > [ 3] 9.0-10.0 sec 8.62 MBytes 72.4 Mbits/sec > [ 3] 0.0-10.0 sec 85.5 MBytes 71.5 Mbits/sec > > Signed-off-by: Xiaoyong Yan <yanxiaoyo...@gmail.com> > --- > include/net/pkt_sched.h | 7 +++++++ > net/sched/sch_api.c | 20 ++++++++++++++++++-- > net/sched/sch_cbs.c | 41 +++++++++++++++++++++++++---------------- > 3 files changed, 50 insertions(+), 18 deletions(-) > > diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h > index 6a70845bd9ab..5fec23b15e1c 100644 > --- a/include/net/pkt_sched.h > +++ b/include/net/pkt_sched.h > @@ -9,6 +9,7 @@ > #include <net/sch_generic.h> > #include <net/net_namespace.h> > #include <uapi/linux/pkt_sched.h> > +#include <linux/hrtimer.h> > > #define DEFAULT_TX_QUEUE_LEN 1000 > > @@ -72,6 +73,12 @@ struct qdisc_watchdog { > struct Qdisc *qdisc; > }; > > +typedef enum hrtimer_restart (*qdisc_watchdog_func_t)(struct hrtimer *timer); > +void qdisc_watchdog_init_clockid_func(struct qdisc_watchdog *wd, struct > Qdisc *qdisc, > + clockid_t clockid,qdisc_watchdog_func_t func); > +void qdisc_watchdog_init_func(struct qdisc_watchdog *wd,struct Qdisc > *qdisc,qdisc_watchdog_func_t func); > +enum hrtimer_restart qdisc_watchdog(struct hrtimer *timer); > + > void qdisc_watchdog_init_clockid(struct qdisc_watchdog *wd, struct Qdisc > *qdisc, > clockid_t clockid); > void qdisc_watchdog_init(struct qdisc_watchdog *wd, struct Qdisc *qdisc); > diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c > index 50794125bf02..fea08d10c811 100644 > --- a/net/sched/sch_api.c > +++ b/net/sched/sch_api.c > @@ -22,7 +22,6 @@ > #include <linux/seq_file.h> > #include <linux/kmod.h> > #include <linux/list.h> > -#include <linux/hrtimer.h> > #include <linux/slab.h> > #include <linux/hashtable.h> > > @@ -591,7 +590,7 @@ void qdisc_warn_nonwc(const char *txt, struct Qdisc > *qdisc) > } > EXPORT_SYMBOL(qdisc_warn_nonwc); > > -static enum hrtimer_restart qdisc_watchdog(struct hrtimer *timer) > +enum hrtimer_restart qdisc_watchdog(struct hrtimer *timer) > { > struct qdisc_watchdog *wd = container_of(timer, struct qdisc_watchdog, > timer); > @@ -602,7 +601,24 @@ static enum hrtimer_restart qdisc_watchdog(struct > hrtimer *timer) > > return HRTIMER_NORESTART; > } > +EXPORT_SYMBOL(qdisc_watchdog); > > +void qdisc_watchdog_init_clockid_func(struct qdisc_watchdog *wd,struct Qdisc > *qdisc,clockid_t clockid,qdisc_watchdog_func_t func) > +{ > + hrtimer_init(&wd->timer,clockid,HRTIMER_MODE_ABS_PINNED); > + if(!func) > + wd->timer.function = qdisc_watchdog; > + else > + wd->timer.function = func; > + wd->qdisc = qdisc; > +} > +EXPORT_SYMBOL(qdisc_watchdog_init_clockid_func); > + > +void qdisc_watchdog_init_func(struct qdisc_watchdog *wd,struct Qdisc > *qdisc,qdisc_watchdog_func_t func) > +{ > + qdisc_watchdog_init_clockid_func(wd,qdisc,CLOCK_MONOTONIC,func); > +} > +EXPORT_SYMBOL(qdisc_watchdog_init_func); > void qdisc_watchdog_init_clockid(struct qdisc_watchdog *wd, struct Qdisc > *qdisc, > clockid_t clockid) > { > diff --git a/net/sched/sch_cbs.c b/net/sched/sch_cbs.c > index 2eaac2ff380f..351d3927e107 100644 > --- a/net/sched/sch_cbs.c > +++ b/net/sched/sch_cbs.c > @@ -187,21 +187,15 @@ static struct sk_buff *cbs_dequeue_soft(struct Qdisc > *sch) > return NULL; > } > if (q->credits < 0) { > - credits = timediff_to_credits(now - q->last, q->idleslope); > + s64 delay; > + > + delay = delay_from_credits(q->credits, q->idleslope); > + qdisc_watchdog_schedule_ns(&q->watchdog, now + delay); > > - credits = q->credits + credits; > - q->credits = min_t(s64, credits, q->hicredit); > - > - if (q->credits < 0) { > - s64 delay; > - > - delay = delay_from_credits(q->credits, q->idleslope); > - qdisc_watchdog_schedule_ns(&q->watchdog, now + delay); > - > - q->last = now; > + q->last = now; > > - return NULL; > - } > + return NULL; > + > } > skb = cbs_child_dequeue(sch, qdisc); > if (!skb) > @@ -212,11 +206,12 @@ static struct sk_buff *cbs_dequeue_soft(struct Qdisc > *sch) > /* As sendslope is a negative number, this will decrease the > * amount of q->credits. > */ > + This is not necessary. > credits = credits_from_len(len, q->sendslope, > atomic64_read(&q->port_rate)); > credits += q->credits; > > - q->credits = max_t(s64, credits, q->locredit); > + q->credits = clamp_t(s64, credits, q->locredit,q->hicredit); > /* Estimate of the transmission of the last byte of the packet in ns > */ > if (unlikely(atomic64_read(&q->port_rate) == 0)) > q->last = now; > @@ -323,7 +318,7 @@ static void cbs_set_port_rate(struct net_device *dev, > struct cbs_sched_data *q) > port_rate = speed * 1000 * BYTES_PER_KBIT; > > atomic64_set(&q->port_rate, port_rate); > - netdev_dbg(dev, "cbs: set %s's port_rate to: %lld, linkspeed: %d\n", > + netdev_dbg(dev,"cbs: set %s's port_rate to: %lld, linkspeed: %d\n", This does not look relevant to your goal. > dev->name, (long long)atomic64_read(&q->port_rate), > ecmd.base.speed); > } > @@ -396,7 +391,21 @@ static int cbs_change(struct Qdisc *sch, struct nlattr > *opt, > > return 0; > } > +static enum hrtimer_restart cbs_qdisc_watchdog(struct hrtimer *timer) A newline is needed before this. > +{ > + struct qdisc_watchdog *wd = container_of(timer,struct > qdisc_watchdog,timer); > + struct Qdisc *sch = wd->qdisc; > + struct cbs_sched_data *q = qdisc_priv(sch); > + s64 now = ktime_get_ns(); > + s64 credits; > + > + credits = timediff_to_credits(now-q->last,q->idleslope); > + credits = q->credits+credits; > + q->credits = clamp_t(s64,credits,q->locredit,q->hicredit); > + q->last = now; > > + return qdisc_watchdog(timer); > +} A newline is needed after this. > static int cbs_init(struct Qdisc *sch, struct nlattr *opt, > struct netlink_ext_ack *extack) > { > @@ -424,7 +433,7 @@ static int cbs_init(struct Qdisc *sch, struct nlattr *opt, > q->enqueue = cbs_enqueue_soft; > q->dequeue = cbs_dequeue_soft; > > - qdisc_watchdog_init(&q->watchdog, sch); > + qdisc_watchdog_init_func(&q->watchdog, sch,cbs_qdisc_watchdog); > > return cbs_change(sch, opt, extack); > } Please run scripts/checkpatch.pl before sending out your patch. Thanks.