Re: [PATCH 6/7] Make qdisc changeable.
Corey Hickey wrote: Patrick McHardy wrote: +if ((err = sfq_q_init(tmp, opt))) +return err; This will also use defaults for all unspecified values. It would be more consistent with other qdiscs to only change those values that are actually specified, so something like tc qdisc change ... perturb 10 will *only* change the perturbation parameter. I'm fixed this for all the parameters except one--your example above. Since 0 is a valid value for perturb, I can't see any clever way to differentiate between the user specifying perturb 0 or leaving perturb unspecified. Either way, opt-perturb_period is 0. The only way I can see would be to add another member, say, opt-perturb_specified, and use that accordingly. Unfortunately, this would break usage of sfq with older versions of tc, so I'm hoping there's a better approach. Do you have any suggestions? I've looked at the other qdisc files and I don't see any other instances like this. Maybe use the nested compat attribute and split the base SFQ struct in the individual members .. - 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
Re: [PATCH 6/7] Make qdisc changeable.
Patrick McHardy wrote: + if ((err = sfq_q_init(tmp, opt))) + return err; This will also use defaults for all unspecified values. It would be more consistent with other qdiscs to only change those values that are actually specified, so something like tc qdisc change ... perturb 10 will *only* change the perturbation parameter. I'm fixed this for all the parameters except one--your example above. Since 0 is a valid value for perturb, I can't see any clever way to differentiate between the user specifying perturb 0 or leaving perturb unspecified. Either way, opt-perturb_period is 0. The only way I can see would be to add another member, say, opt-perturb_specified, and use that accordingly. Unfortunately, this would break usage of sfq with older versions of tc, so I'm hoping there's a better approach. Do you have any suggestions? I've looked at the other qdisc files and I don't see any other instances like this. Thanks, Corey - 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
Re: [PATCH 6/7] Make qdisc changeable.
Patrick McHardy wrote:
Corey Hickey wrote:
Re-implement sfq_change() and enable Qdisc_opts.change so tc qdisc
change will work.
Signed-off-by: Corey Hickey [EMAIL PROTECTED]
---
net/sched/sch_sfq.c | 51 ++-
1 files changed, 50 insertions(+), 1 deletions(-)
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index e6a6a21..e042cd0 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -485,6 +485,55 @@ static int sfq_init(struct Qdisc *sch, struct rtattr *opt)
return 0;
}
+static int sfq_change(struct Qdisc *sch, struct rtattr *opt)
+{
+ struct sfq_sched_data *q = qdisc_priv(sch);
+ struct sfq_sched_data tmp;
+ struct sk_buff *skb;
+ int err;
+
+ /* set up tmp queue */
+ memset(tmp, 0, sizeof(struct sfq_sched_data));
+ tmp.quantum = psched_mtu(sch-dev); /* default */
If no value is given it should use the old value instead of
reinitializing to the default.
+ if ((err = sfq_q_init(tmp, opt)))
+ return err;
This will also use defaults for all unspecified values. It would
be more consistent with other qdiscs to only change those values
that are actually specified, so something like tc qdisc change ...
perturb 10 will *only* change the perturbation parameter.
I somehow had it in my head that a qdisc change was supposed to work
that way. I'll change my patch.
I'm not sure reusing the initialization function and copying the
parameters is the best way to do this.
I'm not sure either, but I do like that it's conceptually simple and it
keeps the parameter handling in one place. I've thought and stared for a
while, and I don't see a better way, but that could of course be due to
my limited understanding and general newbiehood in that regard.
Thanks again for the review. I'll try to get a new batch of patches sent
off tomorrow.
-Corey
-
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
Re: [PATCH 6/7] Make qdisc changeable.
Corey Hickey wrote:
Re-implement sfq_change() and enable Qdisc_opts.change so tc qdisc
change will work.
Signed-off-by: Corey Hickey [EMAIL PROTECTED]
---
net/sched/sch_sfq.c | 51
++-
1 files changed, 50 insertions(+), 1 deletions(-)
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index e6a6a21..e042cd0 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -485,6 +485,55 @@ static int sfq_init(struct Qdisc *sch, struct rtattr
*opt)
return 0;
}
+static int sfq_change(struct Qdisc *sch, struct rtattr *opt)
+{
+ struct sfq_sched_data *q = qdisc_priv(sch);
+ struct sfq_sched_data tmp;
+ struct sk_buff *skb;
+ int err;
+
+ /* set up tmp queue */
+ memset(tmp, 0, sizeof(struct sfq_sched_data));
+ tmp.quantum = psched_mtu(sch-dev); /* default */
If no value is given it should use the old value instead of
reinitializing to the default.
+ if ((err = sfq_q_init(tmp, opt)))
+ return err;
This will also use defaults for all unspecified values. It would
be more consistent with other qdiscs to only change those values
that are actually specified, so something like tc qdisc change ...
perturb 10 will *only* change the perturbation parameter.
I'm not sure reusing the initialization function and copying the
parameters is the best way to do this.
+
+ /* copy packets from the old queue to the tmp queue */
+ sch_tree_lock(sch);
+ while (sch-q.qlen = tmp.limit - 1)
+ sfq_drop(sch);
+ while ((skb = sfq_q_dequeue(q)) != NULL)
+ sfq_q_enqueue(skb, tmp, SFQ_TAIL);
+
+ /* clean up the old queue */
+ sfq_q_destroy(q);
+
+ /* copy elements of the tmp queue into the old queue */
+ q-perturb_period = tmp.perturb_period;
+ q-quantum= tmp.quantum;
+ q-limit = tmp.limit;
+ q-depth = tmp.depth;
+ q-hash_divisor = tmp.hash_divisor;
+ q-tail = tmp.tail;
+ q-max_depth = tmp.max_depth;
+ q-ht= tmp.ht;
+ q-dep = tmp.dep;
+ q-next = tmp.next;
+ q-allot = tmp.allot;
+ q-hash = tmp.hash;
+ q-qs= tmp.qs;
+
+ /* finish up */
+ if (q-perturb_period) {
+ q-perturb_timer.expires = jiffies + q-perturb_period;
+ add_timer(q-perturb_timer);
+ } else {
+ q-perturbation = 0;
+ }
+ sch_tree_unlock(sch);
+ return 0;
+}
+
-
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
[PATCH 6/7] Make qdisc changeable.
Re-implement sfq_change() and enable Qdisc_opts.change so tc qdisc
change will work.
Signed-off-by: Corey Hickey [EMAIL PROTECTED]
---
net/sched/sch_sfq.c | 51 ++-
1 files changed, 50 insertions(+), 1 deletions(-)
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 3e67a68..4cd523f 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -490,6 +490,55 @@ static int sfq_init(struct Qdisc *sch, struct rtattr *opt)
return 0;
}
+static int sfq_change(struct Qdisc *sch, struct rtattr *opt)
+{
+ struct sfq_sched_data *q = qdisc_priv(sch);
+ struct sfq_sched_data tmp;
+ struct sk_buff *skb;
+ int err;
+
+ /* set up tmp queue */
+ memset(tmp, 0, sizeof(struct sfq_sched_data));
+ tmp.quantum = psched_mtu(sch-dev); /* default */
+ if ((err = sfq_q_init(tmp, opt)))
+ return err;
+
+ /* copy packets from the old queue to the tmp queue */
+ sch_tree_lock(sch);
+ while (sch-q.qlen = tmp.limit - 1)
+ sfq_drop(sch);
+ while ((skb = sfq_q_dequeue(q)) != NULL)
+ sfq_q_enqueue(skb, tmp, SFQ_TAIL);
+
+ /* clean up the old queue */
+ sfq_q_destroy(q);
+
+ /* copy elements of the tmp queue into the old queue */
+ q-perturb_period = tmp.perturb_period;
+ q-quantum= tmp.quantum;
+ q-limit = tmp.limit;
+ q-depth = tmp.depth;
+ q-hash_divisor = tmp.hash_divisor;
+ q-tail = tmp.tail;
+ q-max_depth = tmp.max_depth;
+ q-ht= tmp.ht;
+ q-dep = tmp.dep;
+ q-next = tmp.next;
+ q-allot = tmp.allot;
+ q-hash = tmp.hash;
+ q-qs= tmp.qs;
+
+ /* finish up */
+ if (q-perturb_period) {
+ q-perturb_timer.expires = jiffies + q-perturb_period;
+ add_timer(q-perturb_timer);
+ } else {
+ q-perturbation = 0;
+ }
+ sch_tree_unlock(sch);
+ return 0;
+}
+
static int sfq_dump(struct Qdisc *sch, struct sk_buff *skb)
{
struct sfq_sched_data *q = qdisc_priv(sch);
@@ -524,7 +573,7 @@ static struct Qdisc_ops sfq_qdisc_ops = {
.init = sfq_init,
.reset = sfq_reset,
.destroy= sfq_destroy,
- .change = NULL,
+ .change = sfq_change,
.dump = sfq_dump,
.owner = THIS_MODULE,
};
--
1.5.2.4
-
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
[PATCH 6/7] Make qdisc changeable.
Re-implement sfq_change() and enable Qdisc_opts.change so tc qdisc
change will work.
Signed-off-by: Corey Hickey [EMAIL PROTECTED]
---
net/sched/sch_sfq.c | 51 ++-
1 files changed, 50 insertions(+), 1 deletions(-)
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index e6a6a21..e042cd0 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -485,6 +485,55 @@ static int sfq_init(struct Qdisc *sch, struct rtattr *opt)
return 0;
}
+static int sfq_change(struct Qdisc *sch, struct rtattr *opt)
+{
+ struct sfq_sched_data *q = qdisc_priv(sch);
+ struct sfq_sched_data tmp;
+ struct sk_buff *skb;
+ int err;
+
+ /* set up tmp queue */
+ memset(tmp, 0, sizeof(struct sfq_sched_data));
+ tmp.quantum = psched_mtu(sch-dev); /* default */
+ if ((err = sfq_q_init(tmp, opt)))
+ return err;
+
+ /* copy packets from the old queue to the tmp queue */
+ sch_tree_lock(sch);
+ while (sch-q.qlen = tmp.limit - 1)
+ sfq_drop(sch);
+ while ((skb = sfq_q_dequeue(q)) != NULL)
+ sfq_q_enqueue(skb, tmp, SFQ_TAIL);
+
+ /* clean up the old queue */
+ sfq_q_destroy(q);
+
+ /* copy elements of the tmp queue into the old queue */
+ q-perturb_period = tmp.perturb_period;
+ q-quantum= tmp.quantum;
+ q-limit = tmp.limit;
+ q-depth = tmp.depth;
+ q-hash_divisor = tmp.hash_divisor;
+ q-tail = tmp.tail;
+ q-max_depth = tmp.max_depth;
+ q-ht= tmp.ht;
+ q-dep = tmp.dep;
+ q-next = tmp.next;
+ q-allot = tmp.allot;
+ q-hash = tmp.hash;
+ q-qs= tmp.qs;
+
+ /* finish up */
+ if (q-perturb_period) {
+ q-perturb_timer.expires = jiffies + q-perturb_period;
+ add_timer(q-perturb_timer);
+ } else {
+ q-perturbation = 0;
+ }
+ sch_tree_unlock(sch);
+ return 0;
+}
+
static int sfq_dump(struct Qdisc *sch, struct sk_buff *skb)
{
struct sfq_sched_data *q = qdisc_priv(sch);
@@ -519,7 +568,7 @@ static struct Qdisc_ops sfq_qdisc_ops = {
.init = sfq_init,
.reset = sfq_reset,
.destroy= sfq_destroy,
- .change = NULL,
+ .change = sfq_change,
.dump = sfq_dump,
.owner = THIS_MODULE,
};
--
1.5.2.4
-
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
