* [PATCH] [PATCH] net/sched : cbs : fix calculation error of idleslope credits
@ 2020-09-17 6:05 Xiaoyong Yan
2020-09-17 19:12 ` Cong Wang
0 siblings, 1 reply; 2+ messages in thread
From: Xiaoyong Yan @ 2020-09-17 6:05 UTC (permalink / raw)
To: jhs, xiyou.wangcong, jiri; +Cc: davem, netdev, linux-kernel, Xiaoyong Yan
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.
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 <yanxiaoyong5@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.
*/
+
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",
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)
+{
+ 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);
+}
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);
}
--
2.25.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] [PATCH] net/sched : cbs : fix calculation error of idleslope credits
2020-09-17 6:05 [PATCH] [PATCH] net/sched : cbs : fix calculation error of idleslope credits Xiaoyong Yan
@ 2020-09-17 19:12 ` Cong Wang
0 siblings, 0 replies; 2+ messages in thread
From: Cong Wang @ 2020-09-17 19:12 UTC (permalink / raw)
To: Xiaoyong Yan
Cc: Jamal Hadi Salim, Jiri Pirko, David Miller,
Linux Kernel Network Developers, LKML
On Wed, Sep 16, 2020 at 11:05 PM Xiaoyong Yan <yanxiaoyong5@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 <yanxiaoyong5@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.
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-09-17 19:13 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-17 6:05 [PATCH] [PATCH] net/sched : cbs : fix calculation error of idleslope credits Xiaoyong Yan
2020-09-17 19:12 ` Cong Wang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).