* [PATCH 7/7] Remove comments about hardcoded values.
From: Corey Hickey @ 2007-07-29 7:08 UTC (permalink / raw)
To: netdev; +Cc: Corey Hickey
In-Reply-To: <11856929364083-git-send-email-bugfood-ml@fatooh.org>
None of these are true anymore (hooray!).
Signed-off-by: Corey Hickey <bugfood-ml@fatooh.org>
---
include/linux/pkt_sched.h | 8 --------
net/sched/sch_sfq.c | 17 +++--------------
2 files changed, 3 insertions(+), 22 deletions(-)
diff --git a/include/linux/pkt_sched.h b/include/linux/pkt_sched.h
index 268c515..58a0ea6 100644
--- a/include/linux/pkt_sched.h
+++ b/include/linux/pkt_sched.h
@@ -148,14 +148,6 @@ struct tc_sfq_qopt
unsigned flows; /* Maximal number of flows */
};
-/*
- * NOTE: limit, divisor and flows are hardwired to code at the moment.
- *
- * limit=flows=128, divisor=1024;
- *
- * The only reason for this is efficiency, it is possible
- * to change these parameters in compile time.
- */
/* RED section */
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 4cd523f..8e84881 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -61,18 +61,7 @@
We still need true WFQ for top level CSZ, but using WFQ
for the best effort traffic is absolutely pointless:
- SFQ is superior for this purpose.
-
- IMPLEMENTATION:
- This implementation limits maximal queue length to 128;
- maximal mtu to 2^15-1; number of hash buckets to 1024.
- The only goal of this restrictions was that all data
- fit into one 4K page :-). Struct sfq_sched_data is
- organized in anti-cache manner: all the data for a bucket
- are scattered over different locations. This is not good,
- but it allowed me to put it into 4K.
-
- It is easy to increase these values, but not in flight. */
+ SFQ is superior for this purpose. */
#define SFQ_DEPTH_DEFAULT 128
#define SFQ_DIVISOR_DEFAULT 1024
@@ -496,7 +485,7 @@ static int sfq_change(struct Qdisc *sch, struct rtattr *opt)
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 */
@@ -509,7 +498,7 @@ static int sfq_change(struct Qdisc *sch, struct rtattr *opt)
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);
--
1.5.2.4
^ permalink raw reply related
* [PATCH 4/7] Add "depth".
From: Corey Hickey @ 2007-07-29 7:08 UTC (permalink / raw)
To: netdev; +Cc: Corey Hickey
In-Reply-To: <11856929352115-git-send-email-bugfood-ml@fatooh.org>
Make "depth" (number of queues) user-configurable:
* replace #define with a parameter
* use old hardcoded value as a default
* kmalloc() arrays in sfq_q_init()
* free() arrays in sfq_q_destroy()
Signed-off-by: Corey Hickey <bugfood-ml@fatooh.org>
---
net/sched/sch_sfq.c | 85 ++++++++++++++++++++++++++++++++++++---------------
1 files changed, 60 insertions(+), 25 deletions(-)
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 583f925..2ff6a27 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -74,14 +74,14 @@
It is easy to increase these values, but not in flight. */
-#define SFQ_DEPTH 128
+#define SFQ_DEPTH_DEFAULT 128
#define SFQ_HASH_DIVISOR 1024
#define SFQ_HEAD 0
#define SFQ_TAIL 1
-/* This type should contain at least SFQ_DEPTH*2 values */
-typedef unsigned char sfq_index;
+/* This type should contain at least depth*2 values */
+typedef unsigned int sfq_index;
struct sfq_head
{
@@ -95,6 +95,7 @@ struct sfq_sched_data
int perturb_period;
unsigned quantum; /* Allotment per round: MUST BE >= MTU */
int limit;
+ unsigned depth;
/* Variables */
struct timer_list perturb_timer;
@@ -103,11 +104,11 @@ struct sfq_sched_data
sfq_index max_depth; /* Maximal depth */
sfq_index ht[SFQ_HASH_DIVISOR]; /* Hash table */
- sfq_index next[SFQ_DEPTH]; /* Active slots link */
- short allot[SFQ_DEPTH]; /* Current allotment per slot */
- unsigned short hash[SFQ_DEPTH]; /* Hash value indexed by slots */
- struct sk_buff_head qs[SFQ_DEPTH]; /* Slot queue */
- struct sfq_head dep[SFQ_DEPTH*2]; /* Linked list of slots, indexed by depth */
+ sfq_index *next; /* Active slots link */
+ short *allot; /* Current allotment per slot */
+ unsigned short *hash; /* Hash value indexed by slots */
+ struct sk_buff_head *qs; /* Slot queue */
+ struct sfq_head *dep; /* Linked list of slots, indexed by depth */
};
static __inline__ unsigned sfq_fold_hash(struct sfq_sched_data *q, u32 h, u32 h1)
@@ -164,7 +165,7 @@ static unsigned sfq_hash(struct sfq_sched_data *q, struct sk_buff *skb)
static inline void sfq_link(struct sfq_sched_data *q, sfq_index x)
{
sfq_index p, n;
- int d = q->qs[x].qlen + SFQ_DEPTH;
+ int d = q->qs[x].qlen + q->depth;
p = d;
n = q->dep[d].next;
@@ -215,7 +216,7 @@ static unsigned int sfq_drop(struct Qdisc *sch)
drop a packet from it */
if (d > 1) {
- sfq_index x = q->dep[d+SFQ_DEPTH].next;
+ sfq_index x = q->dep[d+q->depth].next;
skb = q->qs[x].prev;
len = skb->len;
__skb_unlink(skb, &q->qs[x]);
@@ -238,7 +239,7 @@ static unsigned int sfq_drop(struct Qdisc *sch)
kfree_skb(skb);
sfq_dec(q, d);
sch->q.qlen--;
- q->ht[q->hash[d]] = SFQ_DEPTH;
+ q->ht[q->hash[d]] = q->depth;
sch->qstats.drops++;
sch->qstats.backlog -= len;
return len;
@@ -253,8 +254,8 @@ static void sfq_q_enqueue(struct sk_buff *skb, struct sfq_sched_data *q, unsigne
sfq_index x;
x = q->ht[hash];
- if (x == SFQ_DEPTH) {
- q->ht[hash] = x = q->dep[SFQ_DEPTH].next;
+ if (x == q->depth) {
+ q->ht[hash] = x = q->dep[q->depth].next;
q->hash[x] = hash;
}
@@ -265,7 +266,7 @@ static void sfq_q_enqueue(struct sk_buff *skb, struct sfq_sched_data *q, unsigne
sfq_inc(q, x);
if (q->qs[x].qlen == 1) { /* The flow is new */
- if (q->tail == SFQ_DEPTH) { /* It is the first flow */
+ if (q->tail == q->depth) { /* It is the first flow */
q->tail = x;
q->next[x] = x;
q->allot[x] = q->quantum;
@@ -316,7 +317,7 @@ static struct sk_buff *sfq_q_dequeue(struct sfq_sched_data *q)
sfq_index a, old_a;
/* No active slots */
- if (q->tail == SFQ_DEPTH)
+ if (q->tail == q->depth)
return NULL;
a = old_a = q->next[q->tail];
@@ -327,10 +328,10 @@ static struct sk_buff *sfq_q_dequeue(struct sfq_sched_data *q)
/* Is the slot empty? */
if (q->qs[a].qlen == 0) {
- q->ht[q->hash[a]] = SFQ_DEPTH;
+ q->ht[q->hash[a]] = q->depth;
a = q->next[a];
if (a == old_a) {
- q->tail = SFQ_DEPTH;
+ q->tail = q->depth;
return skb;
}
q->next[q->tail] = a;
@@ -383,6 +384,16 @@ static void sfq_perturbation(unsigned long arg)
static void sfq_q_destroy(struct sfq_sched_data *q)
{
del_timer(&q->perturb_timer);
+ if(q->dep)
+ kfree(q->dep);
+ if(q->next)
+ kfree(q->next);
+ if(q->allot)
+ kfree(q->allot);
+ if(q->hash)
+ kfree(q->hash);
+ if(q->qs)
+ kfree(q->qs);
}
static void sfq_destroy(struct Qdisc *sch)
@@ -394,6 +405,7 @@ static void sfq_destroy(struct Qdisc *sch)
static int sfq_q_init(struct sfq_sched_data *q, struct rtattr *opt)
{
struct tc_sfq_qopt *ctl = RTA_DATA(opt);
+ sfq_index p = ~0U/2;
int i;
if (opt && opt->rta_len < RTA_LENGTH(sizeof(*ctl)))
@@ -401,30 +413,53 @@ static int sfq_q_init(struct sfq_sched_data *q, struct rtattr *opt)
q->perturbation = 0;
q->max_depth = 0;
- q->tail = q->limit = SFQ_DEPTH;
if (opt == NULL) {
q->perturb_period = 0;
+ q->tail = q->limit = q->depth = SFQ_DEPTH_DEFAULT;
} else {
struct tc_sfq_qopt *ctl = RTA_DATA(opt);
if (ctl->quantum)
q->quantum = ctl->quantum;
q->perturb_period = ctl->perturb_period*HZ;
+ q->tail = q->limit = q->depth = ctl->flows ? : SFQ_DEPTH_DEFAULT;
+
+ if (q->depth > p - 1)
+ return -EINVAL;
if (ctl->limit)
- q->limit = min_t(u32, ctl->limit, SFQ_DEPTH);
+ q->limit = min_t(u32, ctl->limit, q->depth);
}
+ q->dep = kmalloc((1+q->depth*2)*sizeof(struct sfq_head), GFP_KERNEL);
+ if (!q->dep)
+ goto err_case;
+ q->next = kmalloc(q->depth*sizeof(sfq_index), GFP_KERNEL);
+ if (!q->next)
+ goto err_case;
+ q->allot = kmalloc(q->depth*sizeof(short), GFP_KERNEL);
+ if (!q->allot)
+ goto err_case;
+ q->hash = kmalloc(q->depth*sizeof(unsigned short), GFP_KERNEL);
+ if (!q->hash)
+ goto err_case;
+ q->qs = kmalloc(q->depth*sizeof(struct sk_buff_head), GFP_KERNEL);
+ if (!q->qs)
+ goto err_case;
+
for (i=0; i<SFQ_HASH_DIVISOR; i++)
- q->ht[i] = SFQ_DEPTH;
- for (i=0; i<SFQ_DEPTH; i++) {
+ q->ht[i] = q->depth;
+ for (i=0; i<q->depth; i++) {
skb_queue_head_init(&q->qs[i]);
- q->dep[i+SFQ_DEPTH].next = i+SFQ_DEPTH;
- q->dep[i+SFQ_DEPTH].prev = i+SFQ_DEPTH;
+ q->dep[i+q->depth].next = i+q->depth;
+ q->dep[i+q->depth].prev = i+q->depth;
}
- for (i=0; i<SFQ_DEPTH; i++)
+ for (i=0; i<q->depth; i++)
sfq_link(q, i);
return 0;
+err_case:
+ sfq_q_destroy(q);
+ return -ENOBUFS;
}
static int sfq_init(struct Qdisc *sch, struct rtattr *opt)
@@ -458,7 +493,7 @@ static int sfq_dump(struct Qdisc *sch, struct sk_buff *skb)
opt.limit = q->limit;
opt.divisor = SFQ_HASH_DIVISOR;
- opt.flows = q->limit;
+ opt.flows = q->depth;
RTA_PUT(skb, TCA_OPTIONS, sizeof(opt), &opt);
--
1.5.2.4
^ permalink raw reply related
* [PATCH 5/7] Add divisor.
From: Corey Hickey @ 2007-07-29 7:08 UTC (permalink / raw)
To: netdev; +Cc: Corey Hickey
In-Reply-To: <11856929362692-git-send-email-bugfood-ml@fatooh.org>
Make hash divisor user-configurable.
Signed-off-by: Corey Hickey <bugfood-ml@fatooh.org>
---
net/sched/sch_sfq.c | 18 +++++++++++++-----
1 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 2ff6a27..3e67a68 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -75,7 +75,7 @@
It is easy to increase these values, but not in flight. */
#define SFQ_DEPTH_DEFAULT 128
-#define SFQ_HASH_DIVISOR 1024
+#define SFQ_DIVISOR_DEFAULT 1024
#define SFQ_HEAD 0
#define SFQ_TAIL 1
@@ -96,6 +96,7 @@ struct sfq_sched_data
unsigned quantum; /* Allotment per round: MUST BE >= MTU */
int limit;
unsigned depth;
+ unsigned hash_divisor;
/* Variables */
struct timer_list perturb_timer;
@@ -103,7 +104,7 @@ struct sfq_sched_data
sfq_index tail; /* Index of current slot in round */
sfq_index max_depth; /* Maximal depth */
- sfq_index ht[SFQ_HASH_DIVISOR]; /* Hash table */
+ sfq_index *ht; /* Hash table */
sfq_index *next; /* Active slots link */
short *allot; /* Current allotment per slot */
unsigned short *hash; /* Hash value indexed by slots */
@@ -118,7 +119,7 @@ static __inline__ unsigned sfq_fold_hash(struct sfq_sched_data *q, u32 h, u32 h1
/* Have we any rotation primitives? If not, WHY? */
h ^= (h1<<pert) ^ (h1>>(0x1F - pert));
h ^= h>>10;
- return h & 0x3FF;
+ return h & (q->hash_divisor-1);
}
static unsigned sfq_hash(struct sfq_sched_data *q, struct sk_buff *skb)
@@ -384,6 +385,8 @@ static void sfq_perturbation(unsigned long arg)
static void sfq_q_destroy(struct sfq_sched_data *q)
{
del_timer(&q->perturb_timer);
+ if(q->ht)
+ kfree(q->ht);
if(q->dep)
kfree(q->dep);
if(q->next)
@@ -415,12 +418,14 @@ static int sfq_q_init(struct sfq_sched_data *q, struct rtattr *opt)
q->max_depth = 0;
if (opt == NULL) {
q->perturb_period = 0;
+ q->hash_divisor = SFQ_DIVISOR_DEFAULT;
q->tail = q->limit = q->depth = SFQ_DEPTH_DEFAULT;
} else {
struct tc_sfq_qopt *ctl = RTA_DATA(opt);
if (ctl->quantum)
q->quantum = ctl->quantum;
q->perturb_period = ctl->perturb_period*HZ;
+ q->hash_divisor = ctl->divisor ? : SFQ_DIVISOR_DEFAULT;
q->tail = q->limit = q->depth = ctl->flows ? : SFQ_DEPTH_DEFAULT;
if (q->depth > p - 1)
@@ -430,6 +435,9 @@ static int sfq_q_init(struct sfq_sched_data *q, struct rtattr *opt)
q->limit = min_t(u32, ctl->limit, q->depth);
}
+ q->ht = kmalloc(q->hash_divisor*sizeof(sfq_index), GFP_KERNEL);
+ if (!q->ht)
+ goto err_case;
q->dep = kmalloc((1+q->depth*2)*sizeof(struct sfq_head), GFP_KERNEL);
if (!q->dep)
goto err_case;
@@ -446,7 +454,7 @@ static int sfq_q_init(struct sfq_sched_data *q, struct rtattr *opt)
if (!q->qs)
goto err_case;
- for (i=0; i<SFQ_HASH_DIVISOR; i++)
+ for (i=0; i<q->hash_divisor; i++)
q->ht[i] = q->depth;
for (i=0; i<q->depth; i++) {
skb_queue_head_init(&q->qs[i]);
@@ -492,7 +500,7 @@ static int sfq_dump(struct Qdisc *sch, struct sk_buff *skb)
opt.perturb_period = q->perturb_period/HZ;
opt.limit = q->limit;
- opt.divisor = SFQ_HASH_DIVISOR;
+ opt.divisor = q->hash_divisor;
opt.flows = q->depth;
RTA_PUT(skb, TCA_OPTIONS, sizeof(opt), &opt);
--
1.5.2.4
^ permalink raw reply related
* [PATCH 3/7] Move two functions.
From: Corey Hickey @ 2007-07-29 7:08 UTC (permalink / raw)
To: netdev; +Cc: Corey Hickey
In-Reply-To: <11856929352132-git-send-email-bugfood-ml@fatooh.org>
Move sfq_q_destroy() to above sfq_q_init() so that it can be used
by an error case in a later patch.
Move sfq_destroy() as well, for clarity.
Signed-off-by: Corey Hickey <bugfood-ml@fatooh.org>
---
net/sched/sch_sfq.c | 24 ++++++++++++------------
1 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 0c46938..583f925 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -380,6 +380,17 @@ static void sfq_perturbation(unsigned long arg)
}
}
+static void sfq_q_destroy(struct sfq_sched_data *q)
+{
+ del_timer(&q->perturb_timer);
+}
+
+static void sfq_destroy(struct Qdisc *sch)
+{
+ struct sfq_sched_data *q = qdisc_priv(sch);
+ sfq_q_destroy(q);
+}
+
static int sfq_q_init(struct sfq_sched_data *q, struct rtattr *opt)
{
struct tc_sfq_qopt *ctl = RTA_DATA(opt);
@@ -420,7 +431,7 @@ static int sfq_init(struct Qdisc *sch, struct rtattr *opt)
{
struct sfq_sched_data *q = qdisc_priv(sch);
int err;
-
+
q->quantum = psched_mtu(sch->dev); /* default */
if ((err = sfq_q_init(q, opt)))
return err;
@@ -436,17 +447,6 @@ static int sfq_init(struct Qdisc *sch, struct rtattr *opt)
return 0;
}
-static void sfq_q_destroy(struct sfq_sched_data *q)
-{
- del_timer(&q->perturb_timer);
-}
-
-static void sfq_destroy(struct Qdisc *sch)
-{
- struct sfq_sched_data *q = qdisc_priv(sch);
- sfq_q_destroy(q);
-}
-
static int sfq_dump(struct Qdisc *sch, struct sk_buff *skb)
{
struct sfq_sched_data *q = qdisc_priv(sch);
--
1.5.2.4
^ permalink raw reply related
* [PATCH 2/7] Preparatory refactoring part 2.
From: Corey Hickey @ 2007-07-29 7:08 UTC (permalink / raw)
To: netdev; +Cc: Corey Hickey
In-Reply-To: <11856929353322-git-send-email-bugfood-ml@fatooh.org>
Factor code out of sfq_init() and sfq_destroy(), again so that the
new functions can be used by sfq_change() later.
Actually, as the diff itself shows, most of the sfq_q_init() code
comes from the original sfq_change(), but sfq_change() is only
called by sfq_init() right now. Thus, it is safe to remove
sfq_change(); "tc qdisc change" doesn't yet work for sfq anyway.
The sfq_destroy() --> sfq_q_destroy() change looks pointless here,
but it's cleaner to split now and add code to sfq_q_destroy() in a
later patch.
Signed-off-by: Corey Hickey <bugfood-ml@fatooh.org>
---
net/sched/sch_sfq.c | 80 +++++++++++++++++++++++++-------------------------
1 files changed, 40 insertions(+), 40 deletions(-)
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 8ae077f..0c46938 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -380,71 +380,71 @@ static void sfq_perturbation(unsigned long arg)
}
}
-static int sfq_change(struct Qdisc *sch, struct rtattr *opt)
+static int sfq_q_init(struct sfq_sched_data *q, struct rtattr *opt)
{
- struct sfq_sched_data *q = qdisc_priv(sch);
struct tc_sfq_qopt *ctl = RTA_DATA(opt);
- unsigned int qlen;
+ int i;
- if (opt->rta_len < RTA_LENGTH(sizeof(*ctl)))
+ if (opt && opt->rta_len < RTA_LENGTH(sizeof(*ctl)))
return -EINVAL;
- sch_tree_lock(sch);
- q->quantum = ctl->quantum ? : psched_mtu(sch->dev);
- q->perturb_period = ctl->perturb_period*HZ;
- if (ctl->limit)
- q->limit = min_t(u32, ctl->limit, SFQ_DEPTH);
+ q->perturbation = 0;
+ q->max_depth = 0;
+ q->tail = q->limit = SFQ_DEPTH;
+ if (opt == NULL) {
+ q->perturb_period = 0;
+ } else {
+ struct tc_sfq_qopt *ctl = RTA_DATA(opt);
+ if (ctl->quantum)
+ q->quantum = ctl->quantum;
+ q->perturb_period = ctl->perturb_period*HZ;
- qlen = sch->q.qlen;
- while (sch->q.qlen >= q->limit-1)
- sfq_drop(sch);
- qdisc_tree_decrease_qlen(sch, qlen - sch->q.qlen);
+ if (ctl->limit)
+ q->limit = min_t(u32, ctl->limit, SFQ_DEPTH);
+ }
- del_timer(&q->perturb_timer);
- if (q->perturb_period) {
- q->perturb_timer.expires = jiffies + q->perturb_period;
- add_timer(&q->perturb_timer);
+ for (i=0; i<SFQ_HASH_DIVISOR; i++)
+ q->ht[i] = SFQ_DEPTH;
+ for (i=0; i<SFQ_DEPTH; i++) {
+ skb_queue_head_init(&q->qs[i]);
+ q->dep[i+SFQ_DEPTH].next = i+SFQ_DEPTH;
+ q->dep[i+SFQ_DEPTH].prev = i+SFQ_DEPTH;
}
- sch_tree_unlock(sch);
+
+ for (i=0; i<SFQ_DEPTH; i++)
+ sfq_link(q, i);
return 0;
}
static int sfq_init(struct Qdisc *sch, struct rtattr *opt)
{
struct sfq_sched_data *q = qdisc_priv(sch);
- int i;
+ int err;
+
+ q->quantum = psched_mtu(sch->dev); /* default */
+ if ((err = sfq_q_init(q, opt)))
+ return err;
init_timer(&q->perturb_timer);
q->perturb_timer.data = (unsigned long)sch;
q->perturb_timer.function = sfq_perturbation;
-
- for (i=0; i<SFQ_HASH_DIVISOR; i++)
- q->ht[i] = SFQ_DEPTH;
- for (i=0; i<SFQ_DEPTH; i++) {
- skb_queue_head_init(&q->qs[i]);
- q->dep[i+SFQ_DEPTH].next = i+SFQ_DEPTH;
- q->dep[i+SFQ_DEPTH].prev = i+SFQ_DEPTH;
- }
- q->limit = SFQ_DEPTH;
- q->max_depth = 0;
- q->tail = SFQ_DEPTH;
- if (opt == NULL) {
- q->quantum = psched_mtu(sch->dev);
- q->perturb_period = 0;
- } else {
- int err = sfq_change(sch, opt);
- if (err)
- return err;
+ if (q->perturb_period) {
+ q->perturb_timer.expires = jiffies + q->perturb_period;
+ add_timer(&q->perturb_timer);
}
- for (i=0; i<SFQ_DEPTH; i++)
- sfq_link(q, i);
+
return 0;
}
+static void sfq_q_destroy(struct sfq_sched_data *q)
+{
+ del_timer(&q->perturb_timer);
+}
+
static void sfq_destroy(struct Qdisc *sch)
{
struct sfq_sched_data *q = qdisc_priv(sch);
- del_timer(&q->perturb_timer);
+ sfq_q_destroy(q);
}
static int sfq_dump(struct Qdisc *sch, struct sk_buff *skb)
--
1.5.2.4
^ permalink raw reply related
* [PATCH 1/7] Preparatory refactoring part 1.
From: Corey Hickey @ 2007-07-29 7:08 UTC (permalink / raw)
To: netdev; +Cc: Corey Hickey
In-Reply-To: <11856929352537-git-send-email-bugfood-ml@fatooh.org>
Make a new function sfq_q_enqueue() that operates directly on the
queue data. This will be useful for implementing sfq_change() in
a later patch. A pleasant side-effect is reducing most of the
duplicate code in sfq_enqueue() and sfq_requeue().
Similarly, make a new function sfq_q_dequeue().
Signed-off-by: Corey Hickey <bugfood-ml@fatooh.org>
---
net/sched/sch_sfq.c | 70 ++++++++++++++++++++++++++------------------------
1 files changed, 36 insertions(+), 34 deletions(-)
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 9579573..8ae077f 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -77,6 +77,9 @@
#define SFQ_DEPTH 128
#define SFQ_HASH_DIVISOR 1024
+#define SFQ_HEAD 0
+#define SFQ_TAIL 1
+
/* This type should contain at least SFQ_DEPTH*2 values */
typedef unsigned char sfq_index;
@@ -244,10 +247,8 @@ static unsigned int sfq_drop(struct Qdisc *sch)
return 0;
}
-static int
-sfq_enqueue(struct sk_buff *skb, struct Qdisc* sch)
+static void sfq_q_enqueue(struct sk_buff *skb, struct sfq_sched_data *q, unsigned int end)
{
- struct sfq_sched_data *q = qdisc_priv(sch);
unsigned hash = sfq_hash(q, skb);
sfq_index x;
@@ -256,8 +257,12 @@ sfq_enqueue(struct sk_buff *skb, struct Qdisc* sch)
q->ht[hash] = x = q->dep[SFQ_DEPTH].next;
q->hash[x] = hash;
}
- sch->qstats.backlog += skb->len;
- __skb_queue_tail(&q->qs[x], skb);
+
+ if (end == SFQ_TAIL)
+ __skb_queue_tail(&q->qs[x], skb);
+ else
+ __skb_queue_head(&q->qs[x], skb);
+
sfq_inc(q, x);
if (q->qs[x].qlen == 1) { /* The flow is new */
if (q->tail == SFQ_DEPTH) { /* It is the first flow */
@@ -270,12 +275,21 @@ sfq_enqueue(struct sk_buff *skb, struct Qdisc* sch)
q->tail = x;
}
}
+}
+
+static int
+sfq_enqueue(struct sk_buff *skb, struct Qdisc* sch)
+{
+ struct sfq_sched_data *q = qdisc_priv(sch);
+ sfq_q_enqueue(skb, q, SFQ_TAIL);
+ sch->qstats.backlog += skb->len;
if (++sch->q.qlen < q->limit-1) {
sch->bstats.bytes += skb->len;
sch->bstats.packets++;
return 0;
}
+ sch->qstats.drops++;
sfq_drop(sch);
return NET_XMIT_CN;
}
@@ -284,28 +298,8 @@ static int
sfq_requeue(struct sk_buff *skb, struct Qdisc* sch)
{
struct sfq_sched_data *q = qdisc_priv(sch);
- unsigned hash = sfq_hash(q, skb);
- sfq_index x;
-
- x = q->ht[hash];
- if (x == SFQ_DEPTH) {
- q->ht[hash] = x = q->dep[SFQ_DEPTH].next;
- q->hash[x] = hash;
- }
+ sfq_q_enqueue(skb, q, SFQ_HEAD);
sch->qstats.backlog += skb->len;
- __skb_queue_head(&q->qs[x], skb);
- sfq_inc(q, x);
- if (q->qs[x].qlen == 1) { /* The flow is new */
- if (q->tail == SFQ_DEPTH) { /* It is the first flow */
- q->tail = x;
- q->next[x] = x;
- q->allot[x] = q->quantum;
- } else {
- q->next[x] = q->next[q->tail];
- q->next[q->tail] = x;
- q->tail = x;
- }
- }
if (++sch->q.qlen < q->limit - 1) {
sch->qstats.requeues++;
return 0;
@@ -316,13 +310,8 @@ sfq_requeue(struct sk_buff *skb, struct Qdisc* sch)
return NET_XMIT_CN;
}
-
-
-
-static struct sk_buff *
-sfq_dequeue(struct Qdisc* sch)
+static struct sk_buff *sfq_q_dequeue(struct sfq_sched_data *q)
{
- struct sfq_sched_data *q = qdisc_priv(sch);
struct sk_buff *skb;
sfq_index a, old_a;
@@ -335,8 +324,6 @@ sfq_dequeue(struct Qdisc* sch)
/* Grab packet */
skb = __skb_dequeue(&q->qs[a]);
sfq_dec(q, a);
- sch->q.qlen--;
- sch->qstats.backlog -= skb->len;
/* Is the slot empty? */
if (q->qs[a].qlen == 0) {
@@ -353,6 +340,21 @@ sfq_dequeue(struct Qdisc* sch)
a = q->next[a];
q->allot[a] += q->quantum;
}
+
+ return skb;
+}
+
+static struct sk_buff
+*sfq_dequeue(struct Qdisc* sch)
+{
+ struct sfq_sched_data *q = qdisc_priv(sch);
+ struct sk_buff *skb;
+
+ skb = sfq_q_dequeue(q);
+ if (skb == NULL)
+ return NULL;
+ sch->q.qlen--;
+ sch->qstats.backlog -= skb->len;
return skb;
}
--
1.5.2.4
^ permalink raw reply related
* [PATCH 0/7] SFQ: backport some features from ESFQ
From: Corey Hickey @ 2007-07-29 7:08 UTC (permalink / raw)
To: netdev
Hello,
This set of patches adds some of ESFQ's modifications to the original
SFQ. Thus far, I have received support for this approach rather than for
trying to get ESFQ included as a separate qdisc.
http://mailman.ds9a.nl/pipermail/lartc/2007q2/021056.html
My patches here implement "tc qdisc change", user-configurable depth
(number of flows), and user-configurable divisor (for setting hash table
size). I've left out the remaining ESFQ features (usage of jhash and
different hashing methods) because Patrick McHardy intends to submit a
patch that will supersede that functionality; see the URL above.
Default values remain the same, and SFQ's default behavior remains the
same, so there should be no user disruption.
A patch for iproute2 is included after the end of the kernel patch series.
Thanks for your consideration,
Corey
include/linux/pkt_sched.h | 8 --
net/sched/sch_sfq.c | 301 +++++++++++++++++++++++++++++----------------
2 files changed, 192 insertions(+), 117 deletions(-)
[PATCH 1/7] Preparatory refactoring part 1.
[PATCH 2/7] Preparatory refactoring part 2.
[PATCH 3/7] Move two functions.
[PATCH 4/7] Add "depth".
[PATCH 5/7] Add divisor.
[PATCH 6/7] Make qdisc changeable.
[PATCH 7/7] Remove comments about hardcoded values.
[PATCH] [iproute2] SFQ: Support changing depth and divisor.
^ permalink raw reply
* Re: TCP SACK issue, hung connection, tcpdump included
From: Willy Tarreau @ 2007-07-29 6:45 UTC (permalink / raw)
To: Darryl L. Miles; +Cc: linux-kernel, netdev
In-Reply-To: <46AC2CBE.5010500@netbauds.net>
Hi,
[CCing netdev as it's where people competent on the subject are]
On Sun, Jul 29, 2007 at 06:59:26AM +0100, Darryl L. Miles wrote:
> CLIENT = Linux 2.6.20.1-smp [Customer build]
> SERVER = Linux 2.6.9-55.ELsmp [Red Hat Enterprise Linux AS release 4
> (Nahant Update 5)]
>
> The problems start around time index 09:21:39.860302 when the CLIENT issues
> a TCP packet with SACK option set (seemingly for a data segment which has
> already been seen) from that point on the connection hangs.
Where was the capture taken ? on CLIENT or on SERVER (I suspect client from
the timers) ? A possible, but very unlikely reason would be an MTU limitation
somewhere, because the segment which never gets correctly ACKed is also the
largest one in this trace. It would be strange as the SACKs are send
immediately after, but it should be something to consider anyway.
Also, if everything is right with the packets on the server side, then
it would means that it's the RHEL kernel which is buggy (which does not
mean that the same bug does not exist in mainline).
Regards,
Willy
> Full dump available via email.
>
>
> 09:12:13.444999 IP CLIENT.50727 > SERVER.ssh: S 2919512080:2919512080(0)
> win 5840 <mss 1460,sackOK,timestamp 799860259 0,nop,wscale 7>
> 09:12:13.535643 IP SERVER.ssh > CLIENT.50727: S 492909547:492909547(0) ack
> 2919512081 win 5792 <mss 1460,sackOK,timestamp 7126976 799860259,nop,wscale
> 2>
> 09:12:13.535717 IP CLIENT.50727 > SERVER.ssh: . ack 1 win 46
> <nop,nop,timestamp 799860282 7126976>
> 09:12:13.665481 IP SERVER.ssh > CLIENT.50727: P 1:24(23) ack 1 win 1448
> <nop,nop,timestamp 7127074 799860282>
> 09:12:13.665895 IP CLIENT.50727 > SERVER.ssh: . ack 24 win 46
> <nop,nop,timestamp 799860314 7127074>
> 09:12:13.666044 IP CLIENT.50727 > SERVER.ssh: P 1:23(22) ack 24 win 46
> <nop,nop,timestamp 799860314 7127074>
> 09:12:13.776318 IP SERVER.ssh > CLIENT.50727: . ack 23 win 1448
> <nop,nop,timestamp 7127216 799860314>
>
> ...SNIPPED SUCCESSFUL TRAFFIC...
>
> 09:21:39.490740 IP SERVER.ssh > CLIENT.50727: P 18200:18464(264) ack 2991
> win 2728 <nop,nop,timestamp 7692910 800001727>
> 09:21:39.490775 IP CLIENT.50727 > SERVER.ssh: . ack 18464 win 378
> <nop,nop,timestamp 800001755 7692910>
> 09:21:39.860245 IP SERVER.ssh > CLIENT.50727: . 12408:13856(1448) ack 2991
> win 2728 <nop,nop,timestamp 7693293 800001749>
> 09:21:39.860302 IP CLIENT.50727 > SERVER.ssh: . ack 18464 win 378
> <nop,nop,timestamp 800001847 7692910,nop,nop,sack sack 1 {12408:13856} >
> 09:21:40.453440 IP SERVER.ssh > CLIENT.50727: . 12408:13856(1448) ack 2991
> win 2728 <nop,nop,timestamp 7693887 800001749>
> 09:21:40.453495 IP CLIENT.50727 > SERVER.ssh: . ack 18464 win 378
> <nop,nop,timestamp 800001996 7692910,nop,nop,sack sack 1 {12408:13856} >
> 09:21:41.641821 IP SERVER.ssh > CLIENT.50727: . 12408:13856(1448) ack 2991
> win 2728 <nop,nop,timestamp 7695075 800001749>
> 09:21:41.641938 IP CLIENT.50727 > SERVER.ssh: . ack 18464 win 378
> <nop,nop,timestamp 800002293 7692910,nop,nop,sack sack 1 {12408:13856} >
> 09:21:44.017552 IP SERVER.ssh > CLIENT.50727: . 12408:13856(1448) ack 2991
> win 2728 <nop,nop,timestamp 7697451 800001749>
> 09:21:44.017622 IP CLIENT.50727 > SERVER.ssh: . ack 18464 win 378
> <nop,nop,timestamp 800002887 7692910,nop,nop,sack sack 1 {12408:13856} >
> 09:21:48.770051 IP SERVER.ssh > CLIENT.50727: . 12408:13856(1448) ack 2991
> win 2728 <nop,nop,timestamp 7702203 800001749>
> 09:21:48.770099 IP CLIENT.50727 > SERVER.ssh: . ack 18464 win 378
> <nop,nop,timestamp 800004075 7692910,nop,nop,sack sack 1 {12408:13856} >
> 09:21:58.274061 IP SERVER.ssh > CLIENT.50727: . 12408:13856(1448) ack 2991
> win 2728 <nop,nop,timestamp 7711707 800001749>
> 09:21:58.274180 IP CLIENT.50727 > SERVER.ssh: . ack 18464 win 378
> <nop,nop,timestamp 800006450 7692910,nop,nop,sack sack 1 {12408:13856} >
> 09:22:17.282035 IP SERVER.ssh > CLIENT.50727: . 12408:13856(1448) ack 2991
> win 2728 <nop,nop,timestamp 7730715 800001749>
> 09:22:17.282153 IP CLIENT.50727 > SERVER.ssh: . ack 18464 win 378
> <nop,nop,timestamp 800011202 7692910,nop,nop,sack sack 1 {12408:13856} >
> 09:22:55.298955 IP SERVER.ssh > CLIENT.50727: . 12408:13856(1448) ack 2991
> win 2728 <nop,nop,timestamp 7768731 800001749>
> 09:22:55.299023 IP CLIENT.50727 > SERVER.ssh: . ack 18464 win 378
> <nop,nop,timestamp 800020705 7692910,nop,nop,sack sack 1 {12408:13856} >
> 09:24:11.329853 IP SERVER.ssh > CLIENT.50727: . 12408:13856(1448) ack 2991
> win 2728 <nop,nop,timestamp 7844763 800001749>
> 09:24:11.329923 IP CLIENT.50727 > SERVER.ssh: . ack 18464 win 378
> <nop,nop,timestamp 800039711 7692910,nop,nop,sack sack 1 {12408:13856} >
> 09:26:11.331578 IP SERVER.ssh > CLIENT.50727: . 12408:13856(1448) ack 2991
> win 2728 <nop,nop,timestamp 7964763 800001749>
> 09:26:11.331699 IP CLIENT.50727 > SERVER.ssh: . ack 18464 win 378
> <nop,nop,timestamp 800069708 7692910,nop,nop,sack sack 1 {12408:13856} >
> 09:26:13.011885 IP CLIENT.50727 > SERVER.ssh: P 2991:3039(48) ack 18464 win
> 378 <nop,nop,timestamp 800070128 7692910>
> 09:26:13.309032 IP CLIENT.50727 > SERVER.ssh: P 2991:3039(48) ack 18464 win
> 378 <nop,nop,timestamp 800070202 7692910>
> 09:26:13.901048 IP CLIENT.50727 > SERVER.ssh: P 2991:3039(48) ack 18464 win
> 378 <nop,nop,timestamp 800070350 7692910>
> 09:26:15.085103 IP CLIENT.50727 > SERVER.ssh: P 2991:3039(48) ack 18464 win
> 378 <nop,nop,timestamp 800070646 7692910>
> 09:26:17.453195 IP CLIENT.50727 > SERVER.ssh: P 2991:3039(48) ack 18464 win
> 378 <nop,nop,timestamp 800071238 7692910>
> 09:26:22.189378 IP CLIENT.50727 > SERVER.ssh: P 2991:3039(48) ack 18464 win
> 378 <nop,nop,timestamp 800072422 7692910>
> 09:26:31.661696 IP CLIENT.50727 > SERVER.ssh: P 2991:3039(48) ack 18464 win
> 378 <nop,nop,timestamp 800074790 7692910>
> 09:26:50.610374 IP CLIENT.50727 > SERVER.ssh: P 2991:3039(48) ack 18464 win
> 378 <nop,nop,timestamp 800079526 7692910>
> 09:27:28.499729 IP CLIENT.50727 > SERVER.ssh: P 2991:3039(48) ack 18464 win
> 378 <nop,nop,timestamp 800088998 7692910>
> 09:28:11.331256 IP SERVER.ssh > CLIENT.50727: . 12408:13856(1448) ack 2991
> win 2728 <nop,nop,timestamp 8084763 800001749>
> 09:28:11.331354 IP CLIENT.50727 > SERVER.ssh: . ack 18464 win 378
> <nop,nop,timestamp 800099704 7692910,nop,nop,sack sack 1 {12408:13856} >
> 09:28:44.286495 IP CLIENT.50727 > SERVER.ssh: P 2991:3039(48) ack 18464 win
> 378 <nop,nop,timestamp 800107942 7692910>
> 09:30:11.330959 IP SERVER.ssh > CLIENT.50727: . 12408:13856(1448) ack 2991
> win 2728 <nop,nop,timestamp 8204763 800001749>
> 09:30:11.331074 IP CLIENT.50727 > SERVER.ssh: . ack 18464 win 378
> <nop,nop,timestamp 800129701 7692910,nop,nop,sack sack 1 {12408:13856} >
> 09:30:44.298756 IP CLIENT.50727 > SERVER.ssh: P 2991:3039(48) ack 18464 win
> 378 <nop,nop,timestamp 800137942 7692910>
> 09:32:11.331661 IP SERVER.ssh > CLIENT.50727: . 12408:13856(1448) ack 2991
> win 2728 <nop,nop,timestamp 8324763 800001749>
> 09:32:11.331727 IP CLIENT.50727 > SERVER.ssh: . ack 18464 win 378
> <nop,nop,timestamp 800159698 7692910,nop,nop,sack sack 1 {12408:13856} >
> 09:32:44.311051 IP CLIENT.50727 > SERVER.ssh: P 2991:3039(48) ack 18464 win
> 378 <nop,nop,timestamp 800167942 7692910>
> 09:34:11.332375 IP SERVER.ssh > CLIENT.50727: . 12408:13856(1448) ack 2991
> win 2728 <nop,nop,timestamp 8444763 800001749>
> 09:34:11.332447 IP CLIENT.50727 > SERVER.ssh: . ack 18464 win 378
> <nop,nop,timestamp 800189695 7692910,nop,nop,sack sack 1 {12408:13856} >
> 09:34:44.323298 IP CLIENT.50727 > SERVER.ssh: P 2991:3039(48) ack 18464 win
> 378 <nop,nop,timestamp 800197942 7692910>
> 09:36:11.332073 IP SERVER.ssh > CLIENT.50727: . 12408:13856(1448) ack 2991
> win 2728 <nop,nop,timestamp 8564763 800001749>
> 09:36:11.332166 IP CLIENT.50727 > SERVER.ssh: . ack 18464 win 378
> <nop,nop,timestamp 800219692 7692910,nop,nop,sack sack 1 {12408:13856} >
> 09:36:44.335591 IP CLIENT.50727 > SERVER.ssh: P 2991:3039(48) ack 18464 win
> 378 <nop,nop,timestamp 800227942 7692910>
> 09:38:44.351950 IP CLIENT.50727 > SERVER.ssh: P 2991:3039(48) ack 18464 win
> 378 <nop,nop,timestamp 800257942 7692910>
> 09:40:44.368172 IP CLIENT.50727 > SERVER.ssh: P 2991:3039(48) ack 18464 win
> 378 <nop,nop,timestamp 800287943 7692910>
>
> At the end of the dump here the connection is dropped by the client side.
>
>
> Darryl
>
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* [RESEND][PATCH 3/3] PPPoE: move lock_sock() in pppoe_sendmsg() to the right location
From: Florian Zumbiehl @ 2007-07-29 6:04 UTC (permalink / raw)
To: mostrows, netdev
Hi,
and the last one for now: Acquire the sock lock in pppoe_sendmsg()
before accessing the sock - and in particular avoid releasing the lock
even though it hasn't been acquired.
Florian
---------------------------------------------------------------------------
Signed-off-by: Florian Zumbiehl <florz@florz.de>
diff --git a/drivers/net/pppoe.c b/drivers/net/pppoe.c
index 9554924..eef8a5b 100644
--- a/drivers/net/pppoe.c
+++ b/drivers/net/pppoe.c
@@ -779,6 +779,7 @@ static int pppoe_sendmsg(struct kiocb *iocb, struct socket *sock,
struct net_device *dev;
char *start;
+ lock_sock(sk);
if (sock_flag(sk, SOCK_DEAD) || !(sk->sk_state & PPPOX_CONNECTED)) {
error = -ENOTCONN;
goto end;
@@ -789,8 +790,6 @@ static int pppoe_sendmsg(struct kiocb *iocb, struct socket *sock,
hdr.code = 0;
hdr.sid = po->num;
- lock_sock(sk);
-
dev = po->pppoe_dev;
error = -EMSGSIZE;
^ permalink raw reply related
* [RESEND][PATCH 2/3] PPPoX/E: return ENOTTY on unknown ioctl requests
From: Florian Zumbiehl @ 2007-07-29 6:04 UTC (permalink / raw)
To: mostrows, netdev
Hi,
here another patch for the PPPoX/E code that makes sure that ENOTTY is
returned for unknown ioctl requests rather than 0 (and removes another
unneeded initializer which I didn't bother creating a separate patch for).
Florian
---------------------------------------------------------------------------
Signed-off-by: Florian Zumbiehl <florz@florz.de>
diff --git a/drivers/net/pppoe.c b/drivers/net/pppoe.c
index 954328c..9554924 100644
--- a/drivers/net/pppoe.c
+++ b/drivers/net/pppoe.c
@@ -669,8 +669,8 @@ static int pppoe_ioctl(struct socket *sock, unsigned int cmd,
{
struct sock *sk = sock->sk;
struct pppox_sock *po = pppox_sk(sk);
- int val = 0;
- int err = 0;
+ int val;
+ int err;
switch (cmd) {
case PPPIOCGMRU:
@@ -759,8 +759,9 @@ static int pppoe_ioctl(struct socket *sock, unsigned int cmd,
err = 0;
break;
- default:;
- };
+ default:
+ err = -ENOTTY;
+ }
return err;
}
diff --git a/drivers/net/pppox.c b/drivers/net/pppox.c
index 3f8115d..51de561 100644
--- a/drivers/net/pppox.c
+++ b/drivers/net/pppox.c
@@ -72,7 +72,7 @@ int pppox_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
{
struct sock *sk = sock->sk;
struct pppox_sock *po = pppox_sk(sk);
- int rc = 0;
+ int rc;
lock_sock(sk);
@@ -93,12 +93,9 @@ int pppox_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
break;
}
default:
- if (pppox_protos[sk->sk_protocol]->ioctl)
- rc = pppox_protos[sk->sk_protocol]->ioctl(sock, cmd,
- arg);
-
- break;
- };
+ rc = pppox_protos[sk->sk_protocol]->ioctl ?
+ pppox_protos[sk->sk_protocol]->ioctl(sock, cmd, arg) : -ENOTTY;
+ }
release_sock(sk);
return rc;
^ permalink raw reply related
* [RESEND][PATCH 1/3] PPPoE: improved hashing routine
From: Florian Zumbiehl @ 2007-07-29 6:04 UTC (permalink / raw)
To: mostrows, netdev
Hi,
I'm not sure whether this is really worth it, but it looked so
extremely inefficient that I couldn't resist - so let's hope providers
will keep PPPoE around for a while, at least until terabit dsl ;-)
The new code produces the same results as the old version and is
~ 3 to 6 times faster for 4-bit hashes on the CPUs I tested.
Florian
---------------------------------------------------------------------------
Signed-off-by: Florian Zumbiehl <florz@florz.de>
diff --git a/drivers/net/pppoe.c b/drivers/net/pppoe.c
index 9e51fcc..954328c 100644
--- a/drivers/net/pppoe.c
+++ b/drivers/net/pppoe.c
@@ -108,19 +108,24 @@ static inline int cmp_addr(struct pppoe_addr *a, unsigned long sid, char *addr)
(memcmp(a->remote,addr,ETH_ALEN) == 0));
}
-static int hash_item(unsigned long sid, unsigned char *addr)
+#if 8%PPPOE_HASH_BITS
+#error 8 must be a multiple of PPPOE_HASH_BITS
+#endif
+
+static int hash_item(unsigned int sid, unsigned char *addr)
{
- char hash = 0;
- int i, j;
+ unsigned char hash = 0;
+ unsigned int i;
- for (i = 0; i < ETH_ALEN ; ++i) {
- for (j = 0; j < 8/PPPOE_HASH_BITS ; ++j) {
- hash ^= addr[i] >> ( j * PPPOE_HASH_BITS );
- }
+ for (i = 0 ; i < ETH_ALEN ; i++) {
+ hash ^= addr[i];
+ }
+ for (i = 0 ; i < sizeof(sid_t)*8 ; i += 8 ){
+ hash ^= sid>>i;
+ }
+ for (i = 8 ; (i>>=1) >= PPPOE_HASH_BITS ; ) {
+ hash ^= hash>>i;
}
-
- for (i = 0; i < (sizeof(unsigned long)*8) / PPPOE_HASH_BITS ; ++i)
- hash ^= sid >> (i*PPPOE_HASH_BITS);
return hash & ( PPPOE_HASH_SIZE - 1 );
}
^ permalink raw reply related
* Re: [PATCH] sb1000: prevent a potential NULL pointer dereference in sb1000_dev_ioctl()
From: Satyam Sharma @ 2007-07-29 6:04 UTC (permalink / raw)
To: Domen Puncer
Cc: Jesper Juhl, Linux Kernel Mailing List, netdev, Steven Hirsch,
David S. Miller, Franco Venturi
In-Reply-To: <20070729043707.GA8343@nd47.coderock.org>
On Sun, 29 Jul 2007, Domen Puncer wrote:
> On 29/07/07 00:02 +0200, Jesper Juhl wrote:
> > Hi,
> >
> > Here's a small patch, prompted by a find by the Coverity checker,
> > that removes a potential NULL pointer dereference from
> > drivers/net/sb1000.c::sb1000_dev_ioctl().
> > The checker spotted that we do a NULL test of 'dev', yet we
> > dereference the pointer prior to that check.
> > This patch simply moves the dereference after the NULL test.
>
> But... it can't be called without a valid 'dev', no?
> A quick 'grep do_ioctl net/' confirms that all calls are in
> the form of 'dev->do_ioctl(dev, ...'.
Yup, I think so too ...
> > @@ -991,11 +991,13 @@ static int sb1000_dev_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
> > short PID[4];
> > int ioaddr[2], status, frequency;
> > unsigned int stats[5];
> > - struct sb1000_private *lp = netdev_priv(dev);
> > + struct sb1000_private *lp;
> >
> > if (!(dev && dev->flags & IFF_UP))
> > return -ENODEV;
I think we could get rid of the !dev check itself. Actually, the IFF_UP
check /also/ looks suspect to me for two reasons: (1) I remember Stephen
Hemminger once telling me dev->flags is legacy and unsafe, and one of
the netif_xxx() functions be used instead, and, (2) I wonder if we really
require the interface to be up and *running* when we do this ioctl.
Satyam
^ permalink raw reply
* Re: netdevice queueing / sendmsg issue?
From: David Miller @ 2007-07-29 5:49 UTC (permalink / raw)
To: khc; +Cc: netdev
In-Reply-To: <m3lkd0upx8.fsf@maximus.localdomain>
From: Krzysztof Halasa <khc@pm.waw.pl>
Date: Sun, 29 Jul 2007 00:25:07 +0200
> I wonder if it's dev_kfree_skb_irq() which should but fails to wake
> the thing up?
Software interrupts might be getting lost, dev_kfree_skb_irq() has to
queue the kfree_skb() to soft IRQ.
Therefore, dev_kfree_skb_irq() will only work properly from hardware
interrupt context, where we will return and thus run the scheduled
software interrupt.
So some things to check out are whether the driver is invoking
dev_kfree_skb_irq() in the right context, whether ARM might have some
software interrupt processing preculiarity, etc.
^ permalink raw reply
* Re: [PATCH RFC]: napi_struct V4
From: David Miller @ 2007-07-29 5:33 UTC (permalink / raw)
To: jeff; +Cc: shemminger, netdev, hadi, rusty
In-Reply-To: <46AB862C.9040507@garzik.org>
From: Jeff Garzik <jeff@garzik.org>
Date: Sat, 28 Jul 2007 14:08:44 -0400
> That's a performance/parallelization regression from current NAPI :(
That's not true since current NAPI will only run on one cpu, the
one that the interrupt triggers on.
The existing cases that are not guarding the sequence to be
atomic are racy, and in a way which is very hard to debug when
it triggers and the device just appears dead.
^ permalink raw reply
* Re: [PATCH RFC]: napi_struct V4
From: David Miller @ 2007-07-29 5:32 UTC (permalink / raw)
To: rdreier; +Cc: netdev, shemminger, jgarzik, hadi, rusty
In-Reply-To: <ada1wesy2eh.fsf@cisco.com>
From: Roland Dreier <rdreier@cisco.com>
Date: Sat, 28 Jul 2007 08:27:18 -0700
> > Most drivers are in good shape, although some still have very
> > questionable netif_rx_complete() handling, in that racy area that
> > Rusty and myself were discussing today.
> >
> > My inclination is to wrap those sequences around with an IRQ
> > safe spinlock to fix the race provably, and then if driver
> > authors want to optimize that away with techniques like those
> > that tg3, bnx2, sky2, skge et al. use, that's fine but can
> > be done later.
>
> Ouch... that extra lock seems pretty expensive. Also I'm having a
> hard time understanding how the techniques you're alluding to apply to
> devices that may miss events when enabling interrupts; the drivers you
> mention all seem to be for devices that didn't have the race and
> didn't use netif_rx_reschedule() in the old NAPI world. Can you
> provide a little more detail on how the lock could be optimized away?
If you have a means in the device (like tg3, bnx2, e1000, and a score
of others do) to force the device to trigger a HW interrupt, that's
what you do if you detect that events are pending after re-enabling
interrupt in the ->poll() handler.
Frankly I don't think the lock is a big deal and you need something
like it anyways typically.
^ permalink raw reply
* Re: [PATCH RFX]: napi_struct V3
From: David Miller @ 2007-07-29 5:30 UTC (permalink / raw)
To: rdreier; +Cc: netdev, shemminger, rusty, jgarzik
In-Reply-To: <ada6444y2nd.fsf@cisco.com>
From: Roland Dreier <rdreier@cisco.com>
Date: Sat, 28 Jul 2007 08:21:58 -0700
> > Another area of consternation are drivers that were using
> > netif_rx_reschedule(), as that interface was removed because it
> > doesn't fit well with the caller managing the dev->quota et al. I
> > left race conditions in the drivers that were using that interface,
> > but they should still basically work nonetheless.
>
> ...but this is a problem for IPoIB. The underlying IB stuff only
> allows us to register what is essentially a one-shot edge-triggered
> interrupt. So there is a race between calling netif_rx_complete() and
> calling ib_req_notify_cq() (which enables the interrupt), since the
> interrupt might never happen if a packet arrives between the two calls.
You will see further along in the thread that we have found
a locking sequence by which this issue can be resolved.
Please read the full thread and the most recent patch before
coming to conclusions :-)
^ permalink raw reply
* Re: [PATCH] sb1000: prevent a potential NULL pointer dereference in sb1000_dev_ioctl()
From: Domen Puncer @ 2007-07-29 4:37 UTC (permalink / raw)
To: Jesper Juhl
Cc: Linux Kernel Mailing List, netdev, Steven Hirsch, David S. Miller,
Franco Venturi
In-Reply-To: <200707290002.42722.jesper.juhl@gmail.com>
On 29/07/07 00:02 +0200, Jesper Juhl wrote:
> Hi,
>
> Here's a small patch, prompted by a find by the Coverity checker,
> that removes a potential NULL pointer dereference from
> drivers/net/sb1000.c::sb1000_dev_ioctl().
> The checker spotted that we do a NULL test of 'dev', yet we
> dereference the pointer prior to that check.
> This patch simply moves the dereference after the NULL test.
But... it can't be called without a valid 'dev', no?
A quick 'grep do_ioctl net/' confirms that all calls are in
the form of 'dev->do_ioctl(dev, ...'.
Domen
> @@ -991,11 +991,13 @@ static int sb1000_dev_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
> short PID[4];
> int ioaddr[2], status, frequency;
> unsigned int stats[5];
> - struct sb1000_private *lp = netdev_priv(dev);
> + struct sb1000_private *lp;
>
> if (!(dev && dev->flags & IFF_UP))
> return -ENODEV;
>
> + lp = netdev_priv(dev);
> +
> ioaddr[0] = dev->base_addr;
> /* mem_start holds the second I/O address */
> ioaddr[1] = dev->mem_start;
>
^ permalink raw reply
* Re: [PATCH] Merge the Sonics Silicon Backplane subsystem
From: Dmitry Torokhov @ 2007-07-29 4:45 UTC (permalink / raw)
To: Andrew Morton
Cc: Michael Buesch, linux-kernel, bcm43xx-dev, netdev, linux-wireless,
Gary Zambrano
In-Reply-To: <20070727131249.74330a3d.akpm@linux-foundation.org>
On Friday 27 July 2007 16:12, Andrew Morton wrote:
> On Fri, 27 Jul 2007 21:43:59 +0200
> Michael Buesch <mb@bu3sch.de> wrote:
>
> > > Sure, but why is the locking interruptible rather than plain old
> > > mutex_lock()?
> >
> > Hm, well. We hold this mutex for several seconds, as writing takes
> > this long. So I simply thought it was worth allowing the waiter
> > to interrupt here. If you say that's not an issue, I'll be happy
> > to use mutex_lock() and reduce code complexity in this area.
>
> So.. is that what the _interruptible() is for? To allow an impatient user to ^c
> a read?
>
> If so, that sounds reasonable. It's worth a comment explaining these decisions
> to future readers, because it is hard to work out this sort of thinking just
> from the bare C code.
I think most of sysfs ->show() and ->store() implementations use
_interruptible() variant to allow user to interrupt and return early.
--
Dmitry
^ permalink raw reply
* [ofa-general] Re: [PATCH] amso1100: QP init bug in amso driver
From: Roland Dreier @ 2007-07-29 3:09 UTC (permalink / raw)
To: Tom Tucker; +Cc: netdev, OpenFabrics General
In-Reply-To: <1185305512.20489.6.camel@trinity.ogc.int>
thanks, applied.
^ permalink raw reply
* Re: [2/3] 2.6.23-rc1: known regressions v2
From: Bartlomiej Zolnierkiewicz @ 2007-07-29 0:13 UTC (permalink / raw)
To: Michal Piotrowski
Cc: Linus Torvalds, Andrew Morton, LKML, Eric Sesterhenn, Sid Boyce,
linux-ide, dth, Netdev, Stephen Hemminger, Thomas Meyer,
Florian Lohoff
In-Reply-To: <46AA5B39.2010209@googlemail.com>
On Friday 27 July 2007, Michal Piotrowski wrote:
> IDE
>
> Subject : ide problems: 2.6.22-git17 working, 2.6.23-rc1* is not
> References : http://lkml.org/lkml/2007/7/27/298
> Last known good : ?
> Submitter : dth <dth@dth.net>
> Caused-By : ?
> Handled-By : ?
> Status : unknown
No IDE changes after 2.6.22-git17.
Despite this I will try to follow this bugreport.
Thanks,
Bart
^ permalink raw reply
* Re: [PATCH] USB Pegasus driver - avoid a potential NULL pointer dereference.
From: Jesper Juhl @ 2007-07-28 23:55 UTC (permalink / raw)
To: Satyam Sharma
Cc: Greg Kroah-Hartman, netdev, Linux Kernel Mailing List,
Petko Manolov, linux-usb-devel
In-Reply-To: <a781481a0707281637n5b123e4aib46c4e4b009b0018@mail.gmail.com>
On 29/07/07, Satyam Sharma <satyam.sharma@gmail.com> wrote:
> Hi,
>
> On 7/29/07, Jesper Juhl <jesper.juhl@gmail.com> wrote:
> > Hello,
> >
> > This patch makes sure we don't dereference a NULL pointer in
> > drivers/net/usb/pegasus.c::write_bulk_callback() in the initial
> > struct net_device *net = pegasus->net; assignment.
> > The existing code checks if 'pegasus' is NULL and bails out if
> > it is, so we better not touch that pointer until after that check.
> > [...]
> > diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
> > index a05fd97..04cba6b 100644
> > --- a/drivers/net/usb/pegasus.c
> > +++ b/drivers/net/usb/pegasus.c
> > @@ -768,11 +768,13 @@ done:
> > static void write_bulk_callback(struct urb *urb)
> > {
> > pegasus_t *pegasus = urb->context;
> > - struct net_device *net = pegasus->net;
> > + struct net_device *net;
> >
> > if (!pegasus)
> > return;
> >
> > + net = pegasus->net;
> > +
> > if (!netif_device_present(net) || !netif_running(net))
> > return;
>
> Is it really possible that we're called into this function with
> urb->context == NULL? If not, I'd suggest let's just get rid of
> the check itself, instead.
>
I'm not sure. I am not very familiar with this code. I just figured
that moving the assignment is potentially a little safer and it is
certainly no worse than the current code, so that's a safe and
potentially benneficial change. Removing the check may be safe but I'm
not certain enough to make that call...
--
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please http://www.expita.com/nomime.html
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
^ permalink raw reply
* Re: [PATCH] USB Pegasus driver - avoid a potential NULL pointer dereference.
From: Satyam Sharma @ 2007-07-28 23:37 UTC (permalink / raw)
To: Jesper Juhl
Cc: Greg Kroah-Hartman, netdev, Linux Kernel Mailing List,
Petko Manolov, linux-usb-devel
In-Reply-To: <200707290019.02591.jesper.juhl@gmail.com>
Hi,
On 7/29/07, Jesper Juhl <jesper.juhl@gmail.com> wrote:
> Hello,
>
> This patch makes sure we don't dereference a NULL pointer in
> drivers/net/usb/pegasus.c::write_bulk_callback() in the initial
> struct net_device *net = pegasus->net; assignment.
> The existing code checks if 'pegasus' is NULL and bails out if
> it is, so we better not touch that pointer until after that check.
> [...]
> diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
> index a05fd97..04cba6b 100644
> --- a/drivers/net/usb/pegasus.c
> +++ b/drivers/net/usb/pegasus.c
> @@ -768,11 +768,13 @@ done:
> static void write_bulk_callback(struct urb *urb)
> {
> pegasus_t *pegasus = urb->context;
> - struct net_device *net = pegasus->net;
> + struct net_device *net;
>
> if (!pegasus)
> return;
>
> + net = pegasus->net;
> +
> if (!netif_device_present(net) || !netif_running(net))
> return;
Is it really possible that we're called into this function with
urb->context == NULL? If not, I'd suggest let's just get rid of
the check itself, instead.
Satyam
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
^ permalink raw reply
* netdevice queueing / sendmsg issue?
From: Krzysztof Halasa @ 2007-07-28 22:25 UTC (permalink / raw)
To: netdev
Hi,
I have noticed an unexpected behaviour of a userland program sending
packets with AF_PACKET through a network device driver. The problem
is that the userland program waits on sock_wait_for_wmem() for a long
time even if the transmitter is ready and all skb packets have been
transmitted and freed by the driver. Perhaps some clues?
Does it work as designed?
The driver is actually ARM Intel IXP425 Ethernet doing bus mastering
TX, it basically does:
xmit()
{
send_skb_to_hw(skb);
if (no_more_tx_skb_slots) /* there are 16 TX skb slots total */
netif_stop_queue(dev);
return NETDEV_TX_OK;
}
xmit_ready_irq()
{
count = free_tx_skb_slots;
while (packets_transmitted) {
dev_kfree_skb_irq(get_skb_from_hw());
free_tx_skb_slot();
}
if (count == 0)
netif_start_queue(dev);
}
Now the userland program does something like:
struct sockaddr_ll tx_addr;
ip_sock = socket(PF_INET, SOCK_DGRAM, IPPROTO_IP);
strcpy(ifr.ifr_name, "eth0");
ioctl(ip_sock, SIOCGIFINDEX, &ifr);
memset(&tx_addr, 0, sizeof(tx_addr));
tx_addr.sll_family = AF_PACKET;
tx_addr.sll_protocol = htons(ETH_P_ALL);
tx_addr.sll_ifindex = ifr->ifr_ifindex;
tx_sock = socket(PF_PACKET, SOCK_RAW, htons(ETH_P_ALL))
while (1) {
sendto(tx_sock, valid_packet_data, 1514, 0, tx_addr,
sizeof(tx_addr));
print('X');
}
The userland program sends multiple packets and then stops for a period
of several seconds.
What does it wait for?
It seems it's waiting in sock_wait_for_wmem(), at the end of
sock_alloc_send_pskb():
(schedule+0x0/0x6a0) from (schedule_timeout+0x90/0xd0)
(schedule_timeout+0x0/0xd0) from (sock_alloc_send_skb+0x178/0x268)
r7:c6d01d2c r6:7fffffff r5:c6d00000 r4:c6c13800
(sock_alloc_send_skb+0x0/0x268) from (packet_sendmsg+0x100/0x28c)
(packet_sendmsg+0x0/0x28c) from (sock_sendmsg+0xb4/0xe4)
(sock_sendmsg+0x0/0xe4) from (sys_sendto+0xc8/0xf0)
r9:c7b5c500 r8:beeb6dac r7:000005ea r6:c73a5580 r5:c6d01e9c r4:00000000
(sys_sendto+0x0/0xf0) from (sys_socketcall+0x154/0x1f4)
(sys_socketcall+0x0/0x1f4) from (ret_fast_syscall+0x0/0x2c)
r4:00000014
The sequence of events from the device driver POV is:
...
xmit entering and using last skb slot
xmit queue full, netif_stop_queue(dev);
xmit exiting
(now the userland program waits)
xmit_ready_irq entering
xmit_ready_irq dev_kfree_skb_irq()
xmit_ready_irq xmit ready, netif_start_queue(dev);
xmit_ready_irq exiting
(now the TX restarts and the userland program sends another packets)
The above is repeated multiple times, then:
xmit entering and using last skb slot
xmit queue full, netif_stop_queue(dev);
xmit_ready_irq entering
xmit_ready_irq dev_kfree_skb_irq() (1 slot empty and ready for TX)
xmit_ready_irq xmit ready, netif_start_queue(dev);
xmit_ready_irq
xmit_ready_irq dev_kfree_skb_irq() (2 slots empty)
...
xmit_ready_irq dev_kfree_skb_irq() (15 slots empty)
xmit_ready_irq
xmit_ready_irq dev_kfree_skb_irq() (all 16 slots empty)
xmit_ready_irq exiting
(transmitter idle, but the userland program doesn't wake up)
The xmit() is not called again for several seconds, despite
netif_start_queue(dev) called from IRQ handler, all TX skb slots are
ready to be used for transmit.
I wonder if it's dev_kfree_skb_irq() which should but fails to wake
the thing up?
Doing "echo 197665 > /proc/sys/net/core/wmem_default" or
"echo 52824 > /proc/sys/net/core/wmem_default" apparently
"fixes" the problem, anything < 197665 and >= 52825 doesn't.
197665 = 65 * 3041, 52825 = 25 * 2113.
Doing "echo 25560 > /proc/sys/net/core/wmem_default" causes the driver
to never become "TX queue full" (IOW max 15 skb being transmitted),
25561 allows for "TX queue full".
25560 = 16 * 1597.5.
--
Krzysztof Halasa
^ permalink raw reply
* [PATCH] USB Pegasus driver - avoid a potential NULL pointer dereference.
From: Jesper Juhl @ 2007-07-28 22:19 UTC (permalink / raw)
To: Petko Manolov
Cc: Greg Kroah-Hartman, Jesper Juhl, netdev,
Linux Kernel Mailing List, linux-usb-devel
Hello,
This patch makes sure we don't dereference a NULL pointer in
drivers/net/usb/pegasus.c::write_bulk_callback() in the initial
struct net_device *net = pegasus->net; assignment.
The existing code checks if 'pegasus' is NULL and bails out if
it is, so we better not touch that pointer until after that check.
Please consider merging.
Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
---
drivers/net/usb/pegasus.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index a05fd97..04cba6b 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -768,11 +768,13 @@ done:
static void write_bulk_callback(struct urb *urb)
{
pegasus_t *pegasus = urb->context;
- struct net_device *net = pegasus->net;
+ struct net_device *net;
if (!pegasus)
return;
+ net = pegasus->net;
+
if (!netif_device_present(net) || !netif_running(net))
return;
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
^ permalink raw reply related
* Removing the prism54 module
From: Edward Ando @ 2007-07-28 20:40 UTC (permalink / raw)
To: linux-wireless-u79uwXL29TY76Z2rM5mHXA; +Cc: netdev-u79uwXL29TY76Z2rM5mHXA
Dear All,
I am trying to set up software suspend 2 (TuxOnIce now it seems). This
has decided it wants to remove the prism54 module before starting the
hibernate process.
When it tries to do this, (or when I manually do: "ifconfig eth1
down"), I start getting these messages on all terminals, ad infinitum:
kernel: unregister_netdevice: waiting for eth1 to become free. Usage
count = 4
I asked a bit on #prism54 on freenode, and so I upgraded to
linux 2.6.22.1 from kernel.org, always building prism54 as a module,
but the problem continues.
I have tried doing "ifconfig eth1 down" before rmmod, but this does not
help.
If you have any hints or answers as to what I should do, that would be
great. I am not subscribed, so could you answer to this address please.
Thanks in advance,
Yours,
Edward Ando
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox