* Re: gso: Attempt to handle mega-GRO packets
From: Eric Dumazet @ 2013-11-07 2:37 UTC (permalink / raw)
To: Herbert Xu
Cc: Ben Hutchings, David Miller, christoph.paasch, netdev, hkchu,
mwdalton, mst, Jason Wang
In-Reply-To: <20131107021519.GA29081@gondor.apana.org.au>
On Thu, 2013-11-07 at 10:15 +0800, Herbert Xu wrote:
> On Wed, Nov 06, 2013 at 06:02:38PM -0800, Eric Dumazet wrote:
> >
> > 4K page will contain 2 frags and they will coalesce.
> >
> > Performance will still be quite good.
> >
> > We probably add a tweak, to not have any hole in this case.
>
> Also have you considered the security aspect of this? If you have
> two skbs sharing a page, and one gets transmitted to a third party
> using zero-copy, the other unrelated skb's content may become visible
> where it shouldn't.
If the hypervisor is doomed, there is nothing we can do.
virtio_net owns the pages, and relies on hypervisor doing the right
thing.
That you use part of the page, is really irrelevant.
It seems you are speaking of virtio_net sending frames, but its about
receiving frames here.
We receive frames, delivered by the trusted hypervisor.
OK, I will shut up now, since apparently I really upset you.
^ permalink raw reply
* [PATCH net-next v2 0/3] net_sched: make tbf support 64bit rates
From: Yang Yingliang @ 2013-11-07 2:13 UTC (permalink / raw)
To: davem, netdev; +Cc: eric.dumazet, jhs, stephen
After this patch(commit 3e1e3aae1f5d4e8e5:
net_sched: add u64 rate to psched_ratecfg_precompute())
which is from Eric, tbf can deal with 64bit rates.
Add two new attributes so that tc can use them to break
the 32bit limit.
And, fix some checkpatch errors;
replace printk(KERN_DEBUG ...) with pr_debug.
v1 -> v2:
- patch 2/3: return an assignment on seperate line
in tcf_hash_new_index() suggested by Stephen.
Yang Yingliang (3):
net_sched: tbf: support of 64bit rates
net_sched: fix some checkpatch errors
net_sched: Use pr_debug replace printk(KERN_DEBUG ...)
include/uapi/linux/pkt_sched.h | 2 ++
net/sched/act_api.c | 5 +++--
net/sched/cls_bpf.c | 2 +-
net/sched/cls_u32.c | 2 +-
net/sched/sch_cbq.c | 5 +++--
net/sched/sch_dsmark.c | 2 +-
net/sched/sch_generic.c | 4 ++--
net/sched/sch_gred.c | 4 ++--
net/sched/sch_htb.c | 19 ++++++++++---------
net/sched/sch_netem.c | 2 +-
net/sched/sch_sfq.c | 10 ++++++----
net/sched/sch_tbf.c | 22 ++++++++++++++++++----
12 files changed, 50 insertions(+), 29 deletions(-)
--
1.7.12
^ permalink raw reply
* [PATCH net-next v2 3/3] net_sched: Use pr_debug replace printk(KERN_DEBUG ...)
From: Yang Yingliang @ 2013-11-07 2:13 UTC (permalink / raw)
To: davem, netdev; +Cc: eric.dumazet, jhs, stephen
In-Reply-To: <1383790412-41944-1-git-send-email-yangyingliang@huawei.com>
Replace printk(KERN_DEBUG ...) with pr_debug() and
replace pr_warning() with pr_warn().
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
net/sched/sch_cbq.c | 2 +-
net/sched/sch_dsmark.c | 2 +-
net/sched/sch_gred.c | 4 ++--
net/sched/sch_htb.c | 6 +++---
4 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
index a8f40f5..9e3a9dc 100644
--- a/net/sched/sch_cbq.c
+++ b/net/sched/sch_cbq.c
@@ -1060,7 +1060,7 @@ static void cbq_normalize_quanta(struct cbq_sched_data *q, int prio)
}
if (cl->quantum <= 0 ||
cl->quantum > 32*qdisc_dev(cl->qdisc)->mtu) {
- pr_warning("CBQ: class %08x has bad quantum==%ld, repaired.\n",
+ pr_warn("CBQ: class %08x has bad quantum==%ld, repaired.\n",
cl->common.classid, cl->quantum);
cl->quantum = qdisc_dev(cl->qdisc)->mtu/2 + 1;
}
diff --git a/net/sched/sch_dsmark.c b/net/sched/sch_dsmark.c
index 3886365..190cf65 100644
--- a/net/sched/sch_dsmark.c
+++ b/net/sched/sch_dsmark.c
@@ -303,7 +303,7 @@ static struct sk_buff *dsmark_dequeue(struct Qdisc *sch)
* and don't need yet another qdisc as a bypass.
*/
if (p->mask[index] != 0xff || p->value[index])
- pr_warning("dsmark_dequeue: unsupported protocol %d\n",
+ pr_warn("dsmark_dequeue: unsupported protocol %d\n",
ntohs(skb->protocol));
break;
}
diff --git a/net/sched/sch_gred.c b/net/sched/sch_gred.c
index d42234c..19fa4dc 100644
--- a/net/sched/sch_gred.c
+++ b/net/sched/sch_gred.c
@@ -370,7 +370,7 @@ static inline int gred_change_table_def(struct Qdisc *sch, struct nlattr *dps)
for (i = table->DPs; i < MAX_DPs; i++) {
if (table->tab[i]) {
- pr_warning("GRED: Warning: Destroying "
+ pr_warn("GRED: Warning: Destroying "
"shadowed VQ 0x%x\n", i);
gred_destroy_vq(table->tab[i]);
table->tab[i] = NULL;
@@ -456,7 +456,7 @@ static int gred_change(struct Qdisc *sch, struct nlattr *opt)
if (table->tab[table->def])
def_prio = table->tab[table->def]->prio;
- printk(KERN_DEBUG "GRED: DP %u does not have a prio "
+ pr_debug("GRED: DP %u does not have a prio "
"setting default to %d\n", ctl->DP, def_prio);
prio = def_prio;
diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index f6e8a74..6586f3b 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -712,7 +712,7 @@ static s64 htb_do_events(struct htb_sched *q, const int level,
/* too much load - let's continue after a break for scheduling */
if (!(q->warned & HTB_WARN_TOOMANYEVENTS)) {
- pr_warning("htb: too many events!\n");
+ pr_warn("htb: too many events!\n");
q->warned |= HTB_WARN_TOOMANYEVENTS;
}
@@ -1484,13 +1484,13 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
if (!cl->level) {
cl->quantum = hopt->rate.rate / q->rate2quantum;
if (!hopt->quantum && cl->quantum < 1000) {
- pr_warning(
+ pr_warn(
"HTB: quantum of class %X is small. Consider r2q change.\n",
cl->common.classid);
cl->quantum = 1000;
}
if (!hopt->quantum && cl->quantum > 200000) {
- pr_warning(
+ pr_warn(
"HTB: quantum of class %X is big. Consider r2q change.\n",
cl->common.classid);
cl->quantum = 200000;
--
1.7.12
^ permalink raw reply related
* Re: gso: Attempt to handle mega-GRO packets
From: Herbert Xu @ 2013-11-07 2:15 UTC (permalink / raw)
To: Eric Dumazet
Cc: Ben Hutchings, David Miller, christoph.paasch, netdev, hkchu,
mwdalton, mst, Jason Wang
In-Reply-To: <1383789758.2878.32.camel@edumazet-glaptop2.roam.corp.google.com>
On Wed, Nov 06, 2013 at 06:02:38PM -0800, Eric Dumazet wrote:
>
> 4K page will contain 2 frags and they will coalesce.
>
> Performance will still be quite good.
>
> We probably add a tweak, to not have any hole in this case.
Also have you considered the security aspect of this? If you have
two skbs sharing a page, and one gets transmitted to a third party
using zero-copy, the other unrelated skb's content may become visible
where it shouldn't.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* [PATCH net-next v2 2/3] net_sched: fix some checkpatch errors
From: Yang Yingliang @ 2013-11-07 2:13 UTC (permalink / raw)
To: davem, netdev; +Cc: eric.dumazet, jhs, stephen
In-Reply-To: <1383790412-41944-1-git-send-email-yangyingliang@huawei.com>
There are some checkpatch errors, fix them.
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
Suggested-by: Stephen Hemminger <stephen@networkplumber.org>
---
net/sched/act_api.c | 5 +++--
net/sched/cls_bpf.c | 2 +-
net/sched/cls_u32.c | 2 +-
net/sched/sch_cbq.c | 3 ++-
net/sched/sch_generic.c | 4 ++--
net/sched/sch_htb.c | 13 +++++++------
net/sched/sch_netem.c | 2 +-
net/sched/sch_sfq.c | 10 ++++++----
8 files changed, 23 insertions(+), 18 deletions(-)
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index fd70728..d92a90e9 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -191,7 +191,8 @@ u32 tcf_hash_new_index(u32 *idx_gen, struct tcf_hashinfo *hinfo)
val = 1;
} while (tcf_hash_lookup(val, hinfo));
- return (*idx_gen = val);
+ *idx_gen = val;
+ return *idx_gen;
}
EXPORT_SYMBOL(tcf_hash_new_index);
@@ -263,7 +264,7 @@ void tcf_hash_insert(struct tcf_common *p, struct tcf_hashinfo *hinfo)
}
EXPORT_SYMBOL(tcf_hash_insert);
-static struct tc_action_ops *act_base = NULL;
+static struct tc_action_ops *act_base;
static DEFINE_RWLOCK(act_mod_lock);
int tcf_register_action(struct tc_action_ops *act)
diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index 1002a82..d7c72be 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -323,7 +323,7 @@ static int cls_bpf_dump(struct tcf_proto *tp, unsigned long fh,
if (nla == NULL)
goto nla_put_failure;
- memcpy(nla_data(nla), prog->bpf_ops, nla_len(nla));
+ memcpy(nla_data(nla), prog->bpf_ops, nla_len(nla));
if (tcf_exts_dump(skb, &prog->exts, &bpf_ext_map) < 0)
goto nla_put_failure;
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index eb07a1e..59e546c 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -352,7 +352,7 @@ static int u32_destroy_key(struct tcf_proto *tp, struct tc_u_knode *n)
return 0;
}
-static int u32_delete_key(struct tcf_proto *tp, struct tc_u_knode* key)
+static int u32_delete_key(struct tcf_proto *tp, struct tc_u_knode *key)
{
struct tc_u_knode **kp;
struct tc_u_hnode *ht = key->ht_up;
diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
index 7a42c81..a8f40f5 100644
--- a/net/sched/sch_cbq.c
+++ b/net/sched/sch_cbq.c
@@ -1058,7 +1058,8 @@ static void cbq_normalize_quanta(struct cbq_sched_data *q, int prio)
cl->quantum = (cl->weight*cl->allot*q->nclasses[prio])/
q->quanta[prio];
}
- if (cl->quantum <= 0 || cl->quantum>32*qdisc_dev(cl->qdisc)->mtu) {
+ if (cl->quantum <= 0 ||
+ cl->quantum > 32*qdisc_dev(cl->qdisc)->mtu) {
pr_warning("CBQ: class %08x has bad quantum==%ld, repaired.\n",
cl->common.classid, cl->quantum);
cl->quantum = qdisc_dev(cl->qdisc)->mtu/2 + 1;
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 7fc899a..9421350 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -338,13 +338,13 @@ EXPORT_SYMBOL(netif_carrier_off);
cheaper.
*/
-static int noop_enqueue(struct sk_buff *skb, struct Qdisc * qdisc)
+static int noop_enqueue(struct sk_buff *skb, struct Qdisc *qdisc)
{
kfree_skb(skb);
return NET_XMIT_CN;
}
-static struct sk_buff *noop_dequeue(struct Qdisc * qdisc)
+static struct sk_buff *noop_dequeue(struct Qdisc *qdisc)
{
return NULL;
}
diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 0e1e38b..f6e8a74 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -54,7 +54,7 @@
one less than their parent.
*/
-static int htb_hysteresis __read_mostly = 0; /* whether to use mode hysteresis for speedup */
+static int htb_hysteresis __read_mostly; /* whether to use mode hysteresis for speedup */
#define HTB_VER 0x30011 /* major must be matched with number suplied by TC as version */
#if HTB_VER >> 16 != TC_HTB_PROTOVER
@@ -65,7 +65,7 @@ static int htb_hysteresis __read_mostly = 0; /* whether to use mode hysteresis f
module_param (htb_hysteresis, int, 0640);
MODULE_PARM_DESC(htb_hysteresis, "Hysteresis mode, less CPU load, less accurate");
-static int htb_rate_est = 0; /* htb classes have a default rate estimator */
+static int htb_rate_est; /* htb classes have a default rate estimator */
module_param(htb_rate_est, int, 0640);
MODULE_PARM_DESC(htb_rate_est, "setup a default rate estimator (4sec 16sec) for htb classes");
@@ -846,7 +846,7 @@ next:
break;
qdisc_warn_nonwc("htb", cl->un.leaf.q);
- htb_next_rb_node(level ? &cl->parent->un.inner.clprio[prio].ptr:
+ htb_next_rb_node(level ? &cl->parent->un.inner.clprio[prio].ptr :
&q->hlevel[0].hprio[prio].ptr);
cl = htb_lookup_leaf(hprio, prio);
@@ -1276,9 +1276,10 @@ static int htb_delete(struct Qdisc *sch, unsigned long arg)
struct Qdisc *new_q = NULL;
int last_child = 0;
- // TODO: why don't allow to delete subtree ? references ? does
- // tc subsys quarantee us that in htb_destroy it holds no class
- // refs so that we can remove children safely there ?
+ /* TODO: why don't allow to delete subtree ? references ? does
+ * tc subsys quarantee us that in htb_destroy it holds no class
+ * refs so that we can remove children safely there ?
+ */
if (cl->children || cl->filter_cnt)
return -EBUSY;
diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 75c94e5..9765c3f 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -728,7 +728,7 @@ static int get_loss_clg(struct Qdisc *sch, const struct nlattr *attr)
nla_for_each_nested(la, attr, rem) {
u16 type = nla_type(la);
- switch(type) {
+ switch (type) {
case NETEM_LOSS_GI: {
const struct tc_netem_gimodel *gi = nla_data(la);
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index d3a1bc2..76f01e0 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -237,10 +237,12 @@ static inline void sfq_link(struct sfq_sched_data *q, sfq_index x)
}
#define sfq_unlink(q, x, n, p) \
- n = q->slots[x].dep.next; \
- p = q->slots[x].dep.prev; \
- sfq_dep_head(q, p)->next = n; \
- sfq_dep_head(q, n)->prev = p
+ do { \
+ n = q->slots[x].dep.next; \
+ p = q->slots[x].dep.prev; \
+ sfq_dep_head(q, p)->next = n; \
+ sfq_dep_head(q, n)->prev = p; \
+ } while (0)
static inline void sfq_dec(struct sfq_sched_data *q, sfq_index x)
--
1.7.12
^ permalink raw reply related
* [PATCH net-next v2 1/3] net_sched: tbf: support of 64bit rates
From: Yang Yingliang @ 2013-11-07 2:13 UTC (permalink / raw)
To: davem, netdev; +Cc: eric.dumazet, jhs, stephen
In-Reply-To: <1383790412-41944-1-git-send-email-yangyingliang@huawei.com>
With psched_ratecfg_precompute(), tbf can deal with 64bit rates.
Add two new attributes so that tc can use them to break the 32bit
limit.
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
include/uapi/linux/pkt_sched.h | 2 ++
net/sched/sch_tbf.c | 22 ++++++++++++++++++----
2 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
index f2624b5..307f293 100644
--- a/include/uapi/linux/pkt_sched.h
+++ b/include/uapi/linux/pkt_sched.h
@@ -171,6 +171,8 @@ enum {
TCA_TBF_PARMS,
TCA_TBF_RTAB,
TCA_TBF_PTAB,
+ TCA_TBF_RATE64,
+ TCA_TBF_PRATE64,
__TCA_TBF_MAX,
};
diff --git a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c
index b057122..b736517 100644
--- a/net/sched/sch_tbf.c
+++ b/net/sched/sch_tbf.c
@@ -266,20 +266,23 @@ static const struct nla_policy tbf_policy[TCA_TBF_MAX + 1] = {
[TCA_TBF_PARMS] = { .len = sizeof(struct tc_tbf_qopt) },
[TCA_TBF_RTAB] = { .type = NLA_BINARY, .len = TC_RTAB_SIZE },
[TCA_TBF_PTAB] = { .type = NLA_BINARY, .len = TC_RTAB_SIZE },
+ [TCA_TBF_RATE64] = { .type = NLA_U64 },
+ [TCA_TBF_PRATE64] = { .type = NLA_U64 },
};
static int tbf_change(struct Qdisc *sch, struct nlattr *opt)
{
int err;
struct tbf_sched_data *q = qdisc_priv(sch);
- struct nlattr *tb[TCA_TBF_PTAB + 1];
+ struct nlattr *tb[TCA_TBF_MAX + 1];
struct tc_tbf_qopt *qopt;
struct qdisc_rate_table *rtab = NULL;
struct qdisc_rate_table *ptab = NULL;
struct Qdisc *child = NULL;
int max_size, n;
+ u64 rate64 = 0, prate64 = 0;
- err = nla_parse_nested(tb, TCA_TBF_PTAB, opt, tbf_policy);
+ err = nla_parse_nested(tb, TCA_TBF_MAX, opt, tbf_policy);
if (err < 0)
return err;
@@ -341,9 +344,13 @@ static int tbf_change(struct Qdisc *sch, struct nlattr *opt)
q->tokens = q->buffer;
q->ptokens = q->mtu;
- psched_ratecfg_precompute(&q->rate, &rtab->rate, 0);
+ if (tb[TCA_TBF_RATE64])
+ rate64 = nla_get_u64(tb[TCA_TBF_RATE64]);
+ psched_ratecfg_precompute(&q->rate, &rtab->rate, rate64);
if (ptab) {
- psched_ratecfg_precompute(&q->peak, &ptab->rate, 0);
+ if (tb[TCA_TBF_PRATE64])
+ prate64 = nla_get_u64(tb[TCA_TBF_PRATE64]);
+ psched_ratecfg_precompute(&q->peak, &ptab->rate, prate64);
q->peak_present = true;
} else {
q->peak_present = false;
@@ -402,6 +409,13 @@ static int tbf_dump(struct Qdisc *sch, struct sk_buff *skb)
opt.buffer = PSCHED_NS2TICKS(q->buffer);
if (nla_put(skb, TCA_TBF_PARMS, sizeof(opt), &opt))
goto nla_put_failure;
+ if ((q->rate.rate_bytes_ps >= (1ULL << 32)) &&
+ nla_put_u64(skb, TCA_TBF_RATE64, q->rate.rate_bytes_ps))
+ goto nla_put_failure;
+ if (q->peak_present &&
+ (q->peak.rate_bytes_ps >= (1ULL << 32)) &&
+ nla_put_u64(skb, TCA_TBF_PRATE64, q->peak.rate_bytes_ps))
+ goto nla_put_failure;
nla_nest_end(skb, nest);
return skb->len;
--
1.7.12
^ permalink raw reply related
* Re: gso: Attempt to handle mega-GRO packets
From: Eric Dumazet @ 2013-11-07 2:08 UTC (permalink / raw)
To: Herbert Xu
Cc: Ben Hutchings, David Miller, christoph.paasch, netdev, hkchu,
mwdalton, mst, Jason Wang
In-Reply-To: <1383789758.2878.32.camel@edumazet-glaptop2.roam.corp.google.com>
On Wed, 2013-11-06 at 18:02 -0800, Eric Dumazet wrote:
> On Thu, 2013-11-07 at 09:47 +0800, Herbert Xu wrote:
>
> > Say the system is fragmented sufficiently that you'll end up with
> > 0-order pages. In that case you'll only ever be able to coalesce
> > two packets.
>
> 4K page will contain 2 frags and they will coalesce.
>
> Performance will still be quite good.
>
> We probably add a tweak, to not have any hole in this case.
After skb_page_frag_refill() call, its trivial to check if the page is
order-0.
If yes, use the whole remaining space, instead of MAX_PACKET_LEN
-> If memory is fragmented, we switch back to old behavior.
Really, its that simple.
^ permalink raw reply
* Re: [PATCH] ipv6: replace RTF_ROUTEINFO with RTF_ADDRCONF in rt6_get_route_info()
From: Duan Jiong @ 2013-11-07 2:01 UTC (permalink / raw)
To: hannes; +Cc: David Miller, netdev
In-Reply-To: <20131107015100.GF8144@order.stressinduktion.org>
于 2013年11月07日 09:51, Hannes Frederic Sowa 写道:
> On Wed, Nov 06, 2013 at 03:26:41PM +0800, Duan Jiong wrote:
>>
>> As the rfc 4191 said, the Router Preference and Lifetime values in a
>> ::/0 Route Information Option should override the preference and lifetime
>> values in the Router Advertisement header. But when the kernel deals with
>> a ::/0 Route Information Option, the rt6_get_route_info() always return
>> NULL, that means that overriding will not happen, because those default
>> routers were added without flag RTF_ROUTEINFO in rt6_add_dflt_router().
>>
>> In order to match those default routers, we can replace RTF_ROUTEINFO
>> with RTF_ADDRCONF in rt6_get_route_info().
>>
>> Signed-off-by: Duan Jiong <duanj.fnst@cn.fujitsu.com>
>
> Hmm, that looks like a bug. Nice catch!
>
> Couldn't we just replace the rt6_get_route_info in rt6_route_rcv with a call
> to rt6_get_dflt_router? Seems easier, already handles the ::/0 case and also
> does preserve the check for the RTF_ROUTEINFO flag in rt6_add_route_info.
Yeah, your idea is better. I will modify my patch.
Thanks,
Duan
>
> Greetings,
>
> Hannes
>
> --
> 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
>
^ permalink raw reply
* Re: gso: Attempt to handle mega-GRO packets
From: Hannes Frederic Sowa @ 2013-11-07 2:03 UTC (permalink / raw)
To: Eric Dumazet
Cc: Herbert Xu, Ben Hutchings, David Miller, christoph.paasch, netdev,
hkchu, mwdalton
In-Reply-To: <1383788068.2878.26.camel@edumazet-glaptop2.roam.corp.google.com>
On Wed, Nov 06, 2013 at 05:34:28PM -0800, Eric Dumazet wrote:
> On Wed, 2013-11-06 at 17:21 -0800, Eric Dumazet wrote:
> > On Thu, 2013-11-07 at 02:13 +0100, Hannes Frederic Sowa wrote:
> > > On Wed, Nov 06, 2013 at 04:15:21PM -0800, Eric Dumazet wrote:
> > > > On Wed, 2013-11-06 at 11:47 -0800, Eric Dumazet wrote:
> > > > > I'll try a different way.
> > > > >
> > > > > The frag_list would contain a bunch of frags, that we logically add to the bunch
> > > > > of frags found in the first skb shared_info structure.
> > > >
> > > > Here is the patch I came into (I tested it and it works very fine)
> > > >
> > > > The theory is that in GRO stack, all skbs use the head_frag trick,
> > > > so even if one NIC pulled some payload into skb->head, we do not have to
> > > > copy anything. Outside of GRO stack, we are not supposed to provide data
> > > > in skb->head (I am speaking of the skb found on the frag_list extension,
> > > > not the skb_head)
> > > >
> > > > I put a fallback code, just in case, with a WARN_ON_ONCE() so that we
> > > > can catch the offenders (if any) to fix them.
> > > >
> > > > I renamed @skb to @skb_head to more clearly document this code.
> > > > Same for @i renamed to @cur_frag
> > >
> > > I wanted to understand this code more closely and tried it with a test case I
> > > used for the UDP_CORK bugs and also for the tbf panic.
> > >
> > > The packet is allocated as an UFO one and gets segmented by tbf.
> > >
> > > # tc qdisc replace dev eth0 root tbf rate 200kbit latency 20ms burst 5kb
> > > # ./udptest
> > > (Just doing two writes of 200 bytes, then a write of 4096 bytes on a udp
> > > socket. I can send you the source (or a stripped down version, because it got
> > > realy noisy.))
> >
> > Interesting :
> >
> > if (cskb == head_skb)
> > cskb = skb_shinfo(head_skb)->frag_list;
> > else
> > cskb = cskb->next;
> > if (!cskb) {
> > WARN_ON_ONCE(1);
> > goto err;
> > }
> >
> > So here either head_skb->frag_list is NULL, or the frag_list chain finishes too early.
> >
> > More probably I have a bug in the code ;)
>
> Oh yes, I missed a : data_len += remain;
>
> at line 2906 :
>
> offset = remain;
> + data_len += remain;
> continue;
Hm, I still hit the WARN_ON_ONCE (same test as above) with this fixed up:
[ 27.962782] ------------[ cut here ]------------
[ 27.964728] WARNING: CPU: 1 PID: 485 at net/core/skbuff.c:2875 skb_segment+0x72d/0x7b0()
[ 27.967179] Modules linked in: sch_tbf joydev i2c_piix4 virtio_balloon i2c_core serio_raw nfsd auth_rpcgss nfs_acl lockd sunrpc virtio_blk virtio_net virtio_pci virtio_ring virtio ata_generic pata_acpi
[ 27.979403] CPU: 1 PID: 485 Comm: udptest Not tainted 3.12.0+ #5
[ 27.982402] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[ 27.984125] 0000000000000009 ffff8800d581b8a8 ffffffff816423fb 0000000000000000
[ 27.986873] ffff8800d581b8e0 ffffffff81067dcd ffff8800372c2c00 ffffea0000de5800
[ 27.990107] 00000000000005c8 0000000000000000 ffff8800372b4710 ffff8800d581b8f0
[ 27.993473] Call Trace:
[ 27.994506] [<ffffffff816423fb>] dump_stack+0x45/0x56
[ 27.996172] [<ffffffff81067dcd>] warn_slowpath_common+0x7d/0xa0
[ 27.998295] [<ffffffff81067eaa>] warn_slowpath_null+0x1a/0x20
[ 28.000127] [<ffffffff8153f6fd>] skb_segment+0x72d/0x7b0
[ 28.002090] [<ffffffff815b0332>] udp4_ufo_fragment+0xc2/0x120
[ 28.006193] [<ffffffff815b8aed>] inet_gso_segment+0x11d/0x330
[ 28.008292] [<ffffffff812a0469>] ? selinux_ip_postroute+0x99/0x2c0
[ 28.010469] [<ffffffff8154c2ac>] skb_mac_gso_segment+0x9c/0x180
[ 28.013065] [<ffffffff8154c3f0>] __skb_gso_segment+0x60/0xc0
[ 28.015326] [<ffffffffa00e1a5f>] tbf_enqueue+0x5f/0x1f0 [sch_tbf]
[ 28.017752] [<ffffffff8154cd3b>] dev_queue_xmit+0x24b/0x480
[ 28.019906] [<ffffffff81585299>] ip_finish_output+0x2c9/0x3b0
[ 28.022189] [<ffffffff815866c8>] ip_output+0x58/0x90
[ 28.024167] [<ffffffff81585e35>] ip_local_out+0x25/0x30
[ 28.026409] [<ffffffff815871d5>] ip_send_skb+0x15/0x50
[ 28.028517] [<ffffffff815ababf>] udp_send_skb+0x20f/0x2a0
[ 28.030700] [<ffffffff815842c0>] ? ip_copy_metadata+0xc0/0xc0
[ 28.033053] [<ffffffff815aca0a>] udp_sendmsg+0x2fa/0xa10
[ 28.035024] [<ffffffff8129a560>] ? sock_has_perm+0x70/0x90
[ 28.042218] [<ffffffff8153799c>] ? release_sock+0x10c/0x160
[ 28.044222] [<ffffffff815b8734>] inet_sendmsg+0x64/0xb0
[ 28.047297] [<ffffffff8129a693>] ? selinux_socket_sendmsg+0x23/0x30
[ 28.049649] [<ffffffff815335db>] sock_sendmsg+0x8b/0xc0
[ 28.052800] [<ffffffff812b193e>] ? security_netlbl_sid_to_secattr+0x6e/0xb0
[ 28.055594] [<ffffffff811bdc45>] ? __d_alloc+0x25/0x180
[ 28.057651] [<ffffffff8162fb4f>] ? netlbl_domhsh_search+0x1f/0x90
[ 28.060103] [<ffffffff81533781>] SYSC_sendto+0x121/0x1c0
[ 28.062332] [<ffffffff811a81ce>] ? alloc_file+0x1e/0xc0
[ 28.064561] [<ffffffff815305fc>] ? sock_alloc_file+0x9c/0x130
[ 28.066758] [<ffffffff8153429e>] SyS_sendto+0xe/0x10
[ 28.072865] [<ffffffff81651329>] system_call_fastpath+0x16/0x1b
[ 28.074981] ---[ end trace 351089f5102f0c6a ]---
^ permalink raw reply
* Re: gso: Attempt to handle mega-GRO packets
From: Eric Dumazet @ 2013-11-07 2:02 UTC (permalink / raw)
To: Herbert Xu
Cc: Ben Hutchings, David Miller, christoph.paasch, netdev, hkchu,
mwdalton, mst, Jason Wang
In-Reply-To: <20131107014724.GA28946@gondor.apana.org.au>
On Thu, 2013-11-07 at 09:47 +0800, Herbert Xu wrote:
> Say the system is fragmented sufficiently that you'll end up with
> 0-order pages. In that case you'll only ever be able to coalesce
> two packets.
4K page will contain 2 frags and they will coalesce.
Performance will still be quite good.
We probably add a tweak, to not have any hole in this case.
>
> Real systems that run for more than a day do end up with seriously
> fragmented memory.
Sure, but having shallow skbs in the first place help quite a bit.
There is no perfect solution, unless of course you change virtio_net to
provide different queues, with different frag sizes.
Sort of what NIU driver uses.
^ permalink raw reply
* Re: [PATCH] ipv6: replace RTF_ROUTEINFO with RTF_ADDRCONF in rt6_get_route_info()
From: Hannes Frederic Sowa @ 2013-11-07 1:51 UTC (permalink / raw)
To: Duan Jiong; +Cc: David Miller, netdev
In-Reply-To: <5279EF31.4040705@cn.fujitsu.com>
On Wed, Nov 06, 2013 at 03:26:41PM +0800, Duan Jiong wrote:
>
> As the rfc 4191 said, the Router Preference and Lifetime values in a
> ::/0 Route Information Option should override the preference and lifetime
> values in the Router Advertisement header. But when the kernel deals with
> a ::/0 Route Information Option, the rt6_get_route_info() always return
> NULL, that means that overriding will not happen, because those default
> routers were added without flag RTF_ROUTEINFO in rt6_add_dflt_router().
>
> In order to match those default routers, we can replace RTF_ROUTEINFO
> with RTF_ADDRCONF in rt6_get_route_info().
>
> Signed-off-by: Duan Jiong <duanj.fnst@cn.fujitsu.com>
Hmm, that looks like a bug. Nice catch!
Couldn't we just replace the rt6_get_route_info in rt6_route_rcv with a call
to rt6_get_dflt_router? Seems easier, already handles the ::/0 case and also
does preserve the check for the RTF_ROUTEINFO flag in rt6_add_route_info.
Greetings,
Hannes
^ permalink raw reply
* Re: gso: Attempt to handle mega-GRO packets
From: Herbert Xu @ 2013-11-07 1:47 UTC (permalink / raw)
To: Eric Dumazet
Cc: Ben Hutchings, David Miller, christoph.paasch, netdev, hkchu,
mwdalton, mst, Jason Wang
In-Reply-To: <1383786208.2878.15.camel@edumazet-glaptop2.roam.corp.google.com>
On Wed, Nov 06, 2013 at 05:03:28PM -0800, Eric Dumazet wrote:
>
> > But once the system has been running for a while, I see nothing
> > in the virtio_net code that tries to prevent fragmentation. Once
> > fragmentation sets in, you'll be back in the terrible situation
> > that we were in prior to the coalesce patch.
>
> There is no fragmentation, since we allocate 32Kb pages.
Say the system is fragmented sufficiently that you'll end up with
0-order pages. In that case you'll only ever be able to coalesce
two packets.
Real systems that run for more than a day do end up with seriously
fragmented memory.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* [PATCH v2 04/11] net ipset: use rbtree postorder iteration instead of opencoding
From: Cody P Schafer @ 2013-11-07 1:42 UTC (permalink / raw)
To: Andrew Morton, EXT4, Jan Kara, rostedt, David S. Miller,
Florian Westphal, Jozsef Kadlecsik, Pablo Neira Ayuso,
YOSHIFUJI Hideaki
Cc: LKML, Cody P Schafer, coreteam, netdev, netfilter-devel,
netfilter, Patrick McHardy
In-Reply-To: <1383788572-25938-1-git-send-email-cody@linux.vnet.ibm.com>
Use rbtree_postorder_for_each_entry_safe() to destroy the rbtree instead
of opencoding an alternate postorder iteration that modifies the tree
Signed-off-by: Cody P Schafer <cody@linux.vnet.ibm.com>
---
net/netfilter/ipset/ip_set_hash_netiface.c | 27 ++++-----------------------
1 file changed, 4 insertions(+), 23 deletions(-)
diff --git a/net/netfilter/ipset/ip_set_hash_netiface.c b/net/netfilter/ipset/ip_set_hash_netiface.c
index 7d798d5..99dba4c 100644
--- a/net/netfilter/ipset/ip_set_hash_netiface.c
+++ b/net/netfilter/ipset/ip_set_hash_netiface.c
@@ -45,31 +45,12 @@ struct iface_node {
static void
rbtree_destroy(struct rb_root *root)
{
- struct rb_node *p, *n = root->rb_node;
- struct iface_node *node;
-
- /* Non-recursive destroy, like in ext3 */
- while (n) {
- if (n->rb_left) {
- n = n->rb_left;
- continue;
- }
- if (n->rb_right) {
- n = n->rb_right;
- continue;
- }
- p = rb_parent(n);
- node = rb_entry(n, struct iface_node, node);
- if (!p)
- *root = RB_ROOT;
- else if (p->rb_left == n)
- p->rb_left = NULL;
- else if (p->rb_right == n)
- p->rb_right = NULL;
+ struct iface_node *node, *next;
+ rbtree_postorder_for_each_entry_safe(node, next, root, node)
kfree(node);
- n = p;
- }
+
+ *root = RB_ROOT;
}
static int
--
1.8.4.2
^ permalink raw reply related
* Re: [PATCH net-next 6/9] bonding: use RTNL instead of bond lock for bond_3ad_state_machine_handler()
From: Ding Tianhong @ 2013-11-07 1:40 UTC (permalink / raw)
To: Sergei Shtylyov, Jay Vosburgh, Andy Gospodarek, David S. Miller,
Nikolay Aleksandrov, Veaceslav Falico, Netdev
In-Reply-To: <527AD5BC.7040907@cogentembedded.com>
On 2013/11/7 7:50, Sergei Shtylyov wrote:
> Hello.
>
> On 11/06/2013 09:53 AM, Ding Tianhong wrote:
>
>> The bond_3ad_state_machine_handler() is a slow path, it use the bond
>> lock to protect the bond slave list and slave port, the slave list
>> could not be protect by bond lock anymore, so I need to use RTNL or
>> RCU instead of bond lock, but if I remove the bond lock, the
>> bond_3ad_state_machine_handler() may use the slave port when bond
>> releasing the slave, it will occur problems.
>
>> As the bond_3ad_unbind_slave() only protected by bond lock or RTNL,
>> so add RCU is not a good solution as it could not protect the slave
>> port, so RTNL is fit here.
>
>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>> ---
>> drivers/net/bonding/bond_3ad.c | 15 +++++++--------
>> 1 file changed, 7 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>> index 187b1b7..a4d190e 100644
>> --- a/drivers/net/bonding/bond_3ad.c
>> +++ b/drivers/net/bonding/bond_3ad.c
>> @@ -2068,18 +2068,19 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
> [...]
>> - //check if there are any slaves
>> if (!bond_has_slaves(bond))
>> goto re_arm;
>>
>> - // check if agg_select_timer timer after initialize is timed out
>> + /* check if agg_select_timer timer after initialize is timed out */
>> if (BOND_AD_INFO(bond).agg_select_timer && !(--BOND_AD_INFO(bond).agg_select_timer)) {
>> slave = bond_first_slave(bond);
>> port = slave ? &(SLAVE_AD_INFO(slave).port) : NULL;
>>
>> - // select the active aggregator for the bond
>> if (port) {
>> if (!port->slave) {
>> pr_warning("%s: Warning: bond's first port is uninitialized\n",
>> @@ -2093,7 +2094,6 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
>> bond_3ad_set_carrier(bond);
>> }
>>
>> - // for each port run the state machines
>> bond_for_each_slave(bond, slave, iter) {
>> port = &(SLAVE_AD_INFO(slave).port);
>> if (!port->slave) {
>> @@ -2114,7 +2114,7 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
>> ad_mux_machine(port);
>> ad_tx_machine(port);
>>
>> - // turn off the BEGIN bit, since we already handled it
>> + /* turn off the BEGIN bit, since we already handled it */
>> if (port->sm_vars & AD_PORT_BEGIN)
>> port->sm_vars &= ~AD_PORT_BEGIN;
>>
>
> These comment changes are not documented in the changelog and should most probably be in a separate patch.
>
> WBR, Sergei
>
thanks for your reply, I miss it in the changelog, it is already a big patch set, so I really do not want to add patch any more, I will add it in changelog.
Tnanks
Ding
>
> .
>
^ permalink raw reply
* Re: [PATCH net-next 2/9] bonding: remove bond read lock for bond_mii_monitor()
From: Ding Tianhong @ 2013-11-07 1:38 UTC (permalink / raw)
To: Jay Vosburgh
Cc: Andy Gospodarek, David S. Miller, Nikolay Aleksandrov,
Veaceslav Falico, Netdev
In-Reply-To: <29223.1383779906@death.nxdomain>
On 2013/11/7 7:18, Jay Vosburgh wrote:
> Ding Tianhong <dingtianhong@huawei.com> wrote:
>
>> The bond_mii_monitor() still use bond lock read to protect
>> bond_has_slaves() and bond_miimon_inspect(), it is no effect,
>> so I move the RTNL to the top of the function to protect the
>> whole monitor, of course, if the bond state did not changed,
>> the monitor still calling RTNL, it may bring more performance
>> loss, but as a slow path, it is negligible.
>
> I'm not sure this last part is true (about it being ok to
> acquire RTNL every pass). The reason the bond_miimon_* functions are
> arranged in the way they are is specifically to avoid taking RTNL
> unnecessarily. A common setting is miimon=100, which will acquire and
> release RTNL ten times per second.
>
> The inspect function can be make RCU safe, and then the function
> will operate pretty much as it does now (with the multiple phases). If
> a slave disappears between the phases, that's ok; one extra cycle on
> RTNL isn't a big deal, but 10 per second arguably is.
>
> My comment also applies to the later patches in the series that
> make similar "always acquire RTNL" changes to the ARP monitor, ALB
> monitoring function, and the 802.3ad state machine. That would be
> patches:
>
> Subject: [PATCH net-next 3/9] bonding: rebuild the lock use for
> bond_alb_monitor()
> Subject: [PATCH net-next 4/9] bonding: rebuild the lock use for
> bond_loadbalance_arp_mon()
> Subject: [PATCH net-next 5/9] bonding: rebuild the lock use for
> bond_activebackup_arp_mon()
> Subject: [PATCH net-next 6/9] bonding: use RTNL instead of bond lock for
> bond_3ad_state_machine_handler()
>
> The 802.3ad state machine patch or the balance-alb patch,
> combined with the miimon patch, will acquire and release the RTNL lock
> 20 times per second (10 for the 3ad state machine or alb monitor, and 10
> more for a typical miimon configuration). I don't believe this is a
> reasonable implementation.
>
> -J
>
Thansk for your reply. your opinion is reasonable and clear, I will
optimization the lock for these patch, the RTNL should be replace by
RCU in some place.
for better performance :)
Ding
>> also in bond_miimon_commit(), I remove the unwanted curr_slave_lock
>> when calling the bond_select_active_slave(), the RTNL is enough.
>>
>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>> ---
>> drivers/net/bonding/bond_main.c | 44 ++++++++++++-----------------------------
>> 1 file changed, 13 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 9c9803c..98171eb 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -2074,9 +2074,7 @@ static void bond_miimon_commit(struct bonding *bond)
>> do_failover:
>> ASSERT_RTNL();
>> block_netpoll_tx();
>> - write_lock_bh(&bond->curr_slave_lock);
>> bond_select_active_slave(bond);
>> - write_unlock_bh(&bond->curr_slave_lock);
>> unblock_netpoll_tx();
>> }
>>
>> @@ -2098,47 +2096,31 @@ void bond_mii_monitor(struct work_struct *work)
>> bool should_notify_peers = false;
>> unsigned long delay;
>>
>> - read_lock(&bond->lock);
>> -
>> delay = msecs_to_jiffies(bond->params.miimon);
>>
>> - if (!bond_has_slaves(bond))
>> + if (!rtnl_trylock()) {
>> + delay = 1;
>> goto re_arm;
>> + }
>>
>> - should_notify_peers = bond_should_notify_peers(bond);
>> -
>> - if (bond_miimon_inspect(bond)) {
>> - read_unlock(&bond->lock);
>> -
>> - /* Race avoidance with bond_close cancel of workqueue */
>> - if (!rtnl_trylock()) {
>> - read_lock(&bond->lock);
>> - delay = 1;
>> - should_notify_peers = false;
>> - goto re_arm;
>> - }
>> + if (!bond_has_slaves(bond)) {
>> + rtnl_unlock();
>> + goto re_arm;
>> + }
>>
>> - read_lock(&bond->lock);
>> + should_notify_peers = bond_should_notify_peers(bond);
>>
>> + if (bond_miimon_inspect(bond))
>> bond_miimon_commit(bond);
>>
>> - read_unlock(&bond->lock);
>> - rtnl_unlock(); /* might sleep, hold no other locks */
>> - read_lock(&bond->lock);
>> - }
>> + if (should_notify_peers)
>> + call_netdevice_notifiers(NETDEV_NOTIFY_PEERS, bond->dev);
>> +
>> + rtnl_unlock();
>>
>> re_arm:
>> if (bond->params.miimon)
>> queue_delayed_work(bond->wq, &bond->mii_work, delay);
>> -
>> - read_unlock(&bond->lock);
>> -
>> - if (should_notify_peers) {
>> - if (!rtnl_trylock())
>> - return;
>> - call_netdevice_notifiers(NETDEV_NOTIFY_PEERS, bond->dev);
>> - rtnl_unlock();
>> - }
>> }
>>
>> static bool bond_has_this_ip(struct bonding *bond, __be32 ip)
>> --
>> 1.8.2.1
>
> ---
> -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
>
>
> .
>
^ permalink raw reply
* Re: gso: Attempt to handle mega-GRO packets
From: Eric Dumazet @ 2013-11-07 1:34 UTC (permalink / raw)
To: Hannes Frederic Sowa
Cc: Herbert Xu, Ben Hutchings, David Miller, christoph.paasch, netdev,
hkchu, mwdalton
In-Reply-To: <1383787279.2878.22.camel@edumazet-glaptop2.roam.corp.google.com>
On Wed, 2013-11-06 at 17:21 -0800, Eric Dumazet wrote:
> On Thu, 2013-11-07 at 02:13 +0100, Hannes Frederic Sowa wrote:
> > On Wed, Nov 06, 2013 at 04:15:21PM -0800, Eric Dumazet wrote:
> > > On Wed, 2013-11-06 at 11:47 -0800, Eric Dumazet wrote:
> > > > I'll try a different way.
> > > >
> > > > The frag_list would contain a bunch of frags, that we logically add to the bunch
> > > > of frags found in the first skb shared_info structure.
> > >
> > > Here is the patch I came into (I tested it and it works very fine)
> > >
> > > The theory is that in GRO stack, all skbs use the head_frag trick,
> > > so even if one NIC pulled some payload into skb->head, we do not have to
> > > copy anything. Outside of GRO stack, we are not supposed to provide data
> > > in skb->head (I am speaking of the skb found on the frag_list extension,
> > > not the skb_head)
> > >
> > > I put a fallback code, just in case, with a WARN_ON_ONCE() so that we
> > > can catch the offenders (if any) to fix them.
> > >
> > > I renamed @skb to @skb_head to more clearly document this code.
> > > Same for @i renamed to @cur_frag
> >
> > I wanted to understand this code more closely and tried it with a test case I
> > used for the UDP_CORK bugs and also for the tbf panic.
> >
> > The packet is allocated as an UFO one and gets segmented by tbf.
> >
> > # tc qdisc replace dev eth0 root tbf rate 200kbit latency 20ms burst 5kb
> > # ./udptest
> > (Just doing two writes of 200 bytes, then a write of 4096 bytes on a udp
> > socket. I can send you the source (or a stripped down version, because it got
> > realy noisy.))
>
> Interesting :
>
> if (cskb == head_skb)
> cskb = skb_shinfo(head_skb)->frag_list;
> else
> cskb = cskb->next;
> if (!cskb) {
> WARN_ON_ONCE(1);
> goto err;
> }
>
> So here either head_skb->frag_list is NULL, or the frag_list chain finishes too early.
>
> More probably I have a bug in the code ;)
Oh yes, I missed a : data_len += remain;
at line 2906 :
offset = remain;
+ data_len += remain;
continue;
^ permalink raw reply
* Re: [PATCH net-next 2/3] net_sched: fix some checkpatch errors
From: Yang Yingliang @ 2013-11-07 1:33 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20131106073741.154f8590@samsung-9>
On 2013/11/6 23:37, Stephen Hemminger wrote:
> On Wed, 6 Nov 2013 16:05:08 +0800
> Yang Yingliang <yangyingliang@huawei.com> wrote:
>
>> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>> index fd70728..c8aadfa 100644
>> --- a/net/sched/act_api.c
>> +++ b/net/sched/act_api.c
>> @@ -191,7 +191,7 @@ u32 tcf_hash_new_index(u32 *idx_gen, struct tcf_hashinfo *hinfo)
>> val = 1;
>> } while (tcf_hash_lookup(val, hinfo));
>>
>> - return (*idx_gen = val);
>> + return *idx_gen = val;
>
> That is not an improvement. I would rather see the return on a assingment and return
> on separate lines.
>
>
ok, i will fix it in v2.
^ permalink raw reply
* Re: gso: Attempt to handle mega-GRO packets
From: Eric Dumazet @ 2013-11-07 1:21 UTC (permalink / raw)
To: Hannes Frederic Sowa
Cc: Herbert Xu, Ben Hutchings, David Miller, christoph.paasch, netdev,
hkchu, mwdalton
In-Reply-To: <20131107011337.GD8144@order.stressinduktion.org>
On Thu, 2013-11-07 at 02:13 +0100, Hannes Frederic Sowa wrote:
> On Wed, Nov 06, 2013 at 04:15:21PM -0800, Eric Dumazet wrote:
> > On Wed, 2013-11-06 at 11:47 -0800, Eric Dumazet wrote:
> > > I'll try a different way.
> > >
> > > The frag_list would contain a bunch of frags, that we logically add to the bunch
> > > of frags found in the first skb shared_info structure.
> >
> > Here is the patch I came into (I tested it and it works very fine)
> >
> > The theory is that in GRO stack, all skbs use the head_frag trick,
> > so even if one NIC pulled some payload into skb->head, we do not have to
> > copy anything. Outside of GRO stack, we are not supposed to provide data
> > in skb->head (I am speaking of the skb found on the frag_list extension,
> > not the skb_head)
> >
> > I put a fallback code, just in case, with a WARN_ON_ONCE() so that we
> > can catch the offenders (if any) to fix them.
> >
> > I renamed @skb to @skb_head to more clearly document this code.
> > Same for @i renamed to @cur_frag
>
> I wanted to understand this code more closely and tried it with a test case I
> used for the UDP_CORK bugs and also for the tbf panic.
>
> The packet is allocated as an UFO one and gets segmented by tbf.
>
> # tc qdisc replace dev eth0 root tbf rate 200kbit latency 20ms burst 5kb
> # ./udptest
> (Just doing two writes of 200 bytes, then a write of 4096 bytes on a udp
> socket. I can send you the source (or a stripped down version, because it got
> realy noisy.))
Interesting :
if (cskb == head_skb)
cskb = skb_shinfo(head_skb)->frag_list;
else
cskb = cskb->next;
if (!cskb) {
WARN_ON_ONCE(1);
goto err;
}
So here either head_skb->frag_list is NULL, or the frag_list chain finishes too early.
More probably I have a bug in the code ;)
I'll take a look, thanks !
^ permalink raw reply
* Re: Linux sends IPv6 NS packets with the link-local address
From: Hannes Frederic Sowa @ 2013-11-07 1:16 UTC (permalink / raw)
To: Steinar H. Gunderson; +Cc: netdev, ayourtch
In-Reply-To: <20131107005615.GA21230@sesse.net>
On Thu, Nov 07, 2013 at 01:56:15AM +0100, Steinar H. Gunderson wrote:
> On Thu, Nov 07, 2013 at 01:46:11AM +0100, Hannes Frederic Sowa wrote:
> > Problem was, if we enqueue a packet to the resolving queue we use those
> > packets source address as the address we use as the source address. This
> > patch chooses the last enqueued skb's source address to do the resolving
> > thus switching over to a global address much more early (hopefully). I
> > have seen this problem with routers doing uRPF on the source address of
> > arp packets, too.
> >
> > Does this happen when you configure addresses? Do your addresses have
> > a short lifetime advertised by radvd etc?
>
> There are no autoconfigured addresses on the machine; statically set plus
> link-layer only. It does _serve_ RA for another VLAN, though (it does
> 802.1q, some tunnels, and some BGP).
Ok, still the patch should improve the situation if the first packet queued up
used the ll-address as source.
Greetings,
Hannes
^ permalink raw reply
* Re: gso: Attempt to handle mega-GRO packets
From: Hannes Frederic Sowa @ 2013-11-07 1:13 UTC (permalink / raw)
To: Eric Dumazet
Cc: Herbert Xu, Ben Hutchings, David Miller, christoph.paasch, netdev,
hkchu, mwdalton
In-Reply-To: <1383783321.2878.6.camel@edumazet-glaptop2.roam.corp.google.com>
On Wed, Nov 06, 2013 at 04:15:21PM -0800, Eric Dumazet wrote:
> On Wed, 2013-11-06 at 11:47 -0800, Eric Dumazet wrote:
> > I'll try a different way.
> >
> > The frag_list would contain a bunch of frags, that we logically add to the bunch
> > of frags found in the first skb shared_info structure.
>
> Here is the patch I came into (I tested it and it works very fine)
>
> The theory is that in GRO stack, all skbs use the head_frag trick,
> so even if one NIC pulled some payload into skb->head, we do not have to
> copy anything. Outside of GRO stack, we are not supposed to provide data
> in skb->head (I am speaking of the skb found on the frag_list extension,
> not the skb_head)
>
> I put a fallback code, just in case, with a WARN_ON_ONCE() so that we
> can catch the offenders (if any) to fix them.
>
> I renamed @skb to @skb_head to more clearly document this code.
> Same for @i renamed to @cur_frag
I wanted to understand this code more closely and tried it with a test case I
used for the UDP_CORK bugs and also for the tbf panic.
The packet is allocated as an UFO one and gets segmented by tbf.
# tc qdisc replace dev eth0 root tbf rate 200kbit latency 20ms burst 5kb
# ./udptest
(Just doing two writes of 200 bytes, then a write of 4096 bytes on a udp
socket. I can send you the source (or a stripped down version, because it got
realy noisy.))
[ 370.372237] ------------[ cut here ]------------
[ 370.374110] WARNING: CPU: 1 PID: 9359 at net/core/skbuff.c:2875 skb_segment+0x725/0x7a0()
[ 370.382857] Modules linked in: sch_tbf(F) joydev nfsd auth_rpcgss nfs_acl lockd i2c_piix4 i2c_core virtio_balloon sunrpc serio_raw virtio_blk virtio_net virtio_pci virtio_ring ata_generic virtio pata_acpi
[ 370.393546] CPU: 1 PID: 9359 Comm: udptest Tainted: GF 3.12.0+ #1
[ 370.395541] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[ 370.397182] 0000000000000009 ffff88011804f8a8 ffffffff816423eb 0000000000000000
[ 370.401220] ffff88011804f8e0 ffffffff81067dcd ffff8800d2d52900 ffffea00045fac00
[ 370.403912] 00000000000005c8 0000000000000000 ffff8800c45b7d10 ffff88011804f8f0
[ 370.406681] Call Trace:
[ 370.407514] [<ffffffff816423eb>] dump_stack+0x45/0x56
[ 370.409094] [<ffffffff81067dcd>] warn_slowpath_common+0x7d/0xa0
[ 370.421411] [<ffffffff81067eaa>] warn_slowpath_null+0x1a/0x20
[ 370.423776] [<ffffffff8153f6f5>] skb_segment+0x725/0x7a0
[ 370.425961] [<ffffffff815b0322>] udp4_ufo_fragment+0xc2/0x120
[ 370.430097] [<ffffffff815b8add>] inet_gso_segment+0x11d/0x330
[ 370.432763] [<ffffffff812a0469>] ? selinux_ip_postroute+0x99/0x2c0
[ 370.436632] [<ffffffff8154c29c>] skb_mac_gso_segment+0x9c/0x180
[ 370.439060] [<ffffffff8154c3e0>] __skb_gso_segment+0x60/0xc0
[ 370.446392] [<ffffffffa00c9a5f>] tbf_enqueue+0x5f/0x1f0 [sch_tbf]
[ 370.451242] [<ffffffff8154cd2b>] dev_queue_xmit+0x24b/0x480
[ 370.454330] [<ffffffff81585289>] ip_finish_output+0x2c9/0x3b0
[ 370.456631] [<ffffffff815866b8>] ip_output+0x58/0x90
[ 370.458439] [<ffffffff81585e25>] ip_local_out+0x25/0x30
[ 370.461258] [<ffffffff815871c5>] ip_send_skb+0x15/0x50
[ 370.463113] [<ffffffff815abaaf>] udp_send_skb+0x20f/0x2a0
[ 370.465031] [<ffffffff815842b0>] ? ip_copy_metadata+0xc0/0xc0
[ 370.467024] [<ffffffff815ac9fa>] udp_sendmsg+0x2fa/0xa10
[ 370.468884] [<ffffffff8129a560>] ? sock_has_perm+0x70/0x90
[ 370.470922] [<ffffffff8153799c>] ? release_sock+0x10c/0x160
[ 370.473651] [<ffffffff815b8724>] inet_sendmsg+0x64/0xb0
[ 370.475621] [<ffffffff8129a693>] ? selinux_socket_sendmsg+0x23/0x30
[ 370.478025] [<ffffffff815335db>] sock_sendmsg+0x8b/0xc0
[ 370.483445] [<ffffffff812b193e>] ? security_netlbl_sid_to_secattr+0x6e/0xb0
[ 370.487287] [<ffffffff8162fae2>] ? netlbl_domhsh_hash+0x12/0x50
[ 370.489441] [<ffffffff8162fb3f>] ? netlbl_domhsh_search+0x1f/0x90
[ 370.492770] [<ffffffff81533781>] SYSC_sendto+0x121/0x1c0
[ 370.495053] [<ffffffff811a81ce>] ? alloc_file+0x1e/0xc0
[ 370.497309] [<ffffffff815305fc>] ? sock_alloc_file+0x9c/0x130
[ 370.500255] [<ffffffff8153429e>] SyS_sendto+0xe/0x10
[ 370.503751] [<ffffffff81651329>] system_call_fastpath+0x16/0x1b
[ 370.506104] ---[ end trace 117f3806fa493b38 ]---
Maybe it does help.
Greetings,
Hannes
^ permalink raw reply
* Re: gso: Attempt to handle mega-GRO packets
From: Eric Dumazet @ 2013-11-07 1:08 UTC (permalink / raw)
To: Herbert Xu
Cc: Ben Hutchings, David Miller, christoph.paasch, netdev, hkchu,
mwdalton
In-Reply-To: <20131107010037.GA28421@gondor.apana.org.au>
On Thu, 2013-11-07 at 09:00 +0800, Herbert Xu wrote:
> On Wed, Nov 06, 2013 at 04:56:03PM -0800, Eric Dumazet wrote:
> > On Thu, 2013-11-07 at 08:47 +0800, Herbert Xu wrote:
> > > On Wed, Nov 06, 2013 at 04:15:21PM -0800, Eric Dumazet wrote:
> > > >
> > > > Here is the patch I came into (I tested it and it works very fine)
> > >
> > > Thanks. However, unless I'm missing something aren't you now
> > > copying every linear skb frag_list? With the current code we
> > > just do a clone and add the headers.
> >
> > No copy at all.
> >
> > As explained, all RX skb are now backed to a page frag. (see
> > skb->head_frag)
>
> Even if we have called pskb_expand_head on it?
Well, in this case we pulled only the headers in skb->head.
Only few buggy drivers do a pull of say 64 bytes in skb->head before
calling eth_type_trans() so could possibly have a few bytes of TCP
payload in skb->head
And this case is handled without copy.
^ permalink raw reply
* Re: gso: Attempt to handle mega-GRO packets
From: Eric Dumazet @ 2013-11-07 1:03 UTC (permalink / raw)
To: Herbert Xu
Cc: Ben Hutchings, David Miller, christoph.paasch, netdev, hkchu,
mwdalton, mst, Jason Wang
In-Reply-To: <20131107003658.GA27976@gondor.apana.org.au>
On Thu, 2013-11-07 at 08:36 +0800, Herbert Xu wrote:
> On Wed, Nov 06, 2013 at 07:01:10AM -0800, Eric Dumazet wrote:
> > Have you thought about arches having PAGE_SIZE=65536, and how bad it is
> > to use a full page per network frame ? It is lazy and x86 centered.
>
> So instead if we were sending a full 64K packet on such an arch to
> another guest, we'd now chop it up into 1.5K chunks and reassemble them.
>
Yep, and speed is now better than before the patches.
I understand you do not believe it. But this is the truth.
And now your guest can receive a bunch of small UDP frames, without
having to drop them because sk->rcvbuf limit is hit.
> > So after our patches, we now have an optimal situation, even on these
> > arches.
>
> Optimal only for physical incoming packets with no jumbo frames.
Have you actually tested this ?
>
> What's worse, I now realise that the coalesce thing isn't even
> guaranteed to work. It probably works in your benchmarks because
> you're working with freshly allocated pages.
>
Oh well.
> But once the system has been running for a while, I see nothing
> in the virtio_net code that tries to prevent fragmentation. Once
> fragmentation sets in, you'll be back in the terrible situation
> that we were in prior to the coalesce patch.
>
There is no fragmentation, since we allocate 32Kb pages.
Michael Dalton worked on a patch to add EWMA for auto sizing and a
private page_frag per virtio queue, instead of using the per cpu one.
On x86 :
- All offloads enabled (average packet size should be >> MTU-size)
net-next trunk w/ virtio_net prior to 2613af0ed (PAGE_SIZE bufs): 14179.17Gb/s
net-next trunk (MTU-size bufs): 13390.69Gb/s
net-next trunk + auto-tune - 14358.41Gb/s
- guest_tso4/guest_csum disabled (forces MTU-sized packets on receiver)
net-next trunk w/ virtio_net prior to 2613af0ed: 4059.49Gb/s
net-next trunk (MTU 1500- packet takes two bufs due to sizing bug): 4174.30Gb/s
net-next trunk (MTU 1480- packet fits in one buf): 6672.16Gb/s
net-next trunk + auto-tune (MTU 1500- fixed, packet uses one buf) - 6791.28Gb/s
^ permalink raw reply
* Re: [PATCH net-next v4 3/9] static_key: WARN on usage before jump_label_init was called
From: Steven Rostedt @ 2013-11-07 1:02 UTC (permalink / raw)
To: Hannes Frederic Sowa
Cc: netdev, linux-kernel, Peter Zijlstra, Andi Kleen, Ingo Molnar,
Jason Baron
In-Reply-To: <20131107005027.GC8144@order.stressinduktion.org>
On Thu, 7 Nov 2013 01:50:27 +0100
Hannes Frederic Sowa <hannes@stressinduktion.org> wrote:
> On Wed, Nov 06, 2013 at 04:16:49PM -0500, Steven Rostedt wrote:
> > Sorry for the late reply, but this was sent while I was getting ready
> > for my two week conference trip.
> >
> > Note, this should not go through the net tree, but instead should go
> > through tip, as it deals with jump labels and not networking.
> >
> > Otherwise, this patch looks good.
> >
> > Reviewed-by: Steven Rostedt <rostedt@goodmis.org>
>
> Thanks for the review!
>
> The patch already got queued up in net-next. Is this ok or what do you
> suggest to resolve this?
>
I'm fine, but really. Changes need to go through the trees they are
maintained by. Would Dave Miller like it if I pushed patches that
touched the net directory without a single Ack?
-- Steve
^ permalink raw reply
* Re: gso: Attempt to handle mega-GRO packets
From: Herbert Xu @ 2013-11-07 1:00 UTC (permalink / raw)
To: Eric Dumazet
Cc: Ben Hutchings, David Miller, christoph.paasch, netdev, hkchu,
mwdalton
In-Reply-To: <1383785763.2878.8.camel@edumazet-glaptop2.roam.corp.google.com>
On Wed, Nov 06, 2013 at 04:56:03PM -0800, Eric Dumazet wrote:
> On Thu, 2013-11-07 at 08:47 +0800, Herbert Xu wrote:
> > On Wed, Nov 06, 2013 at 04:15:21PM -0800, Eric Dumazet wrote:
> > >
> > > Here is the patch I came into (I tested it and it works very fine)
> >
> > Thanks. However, unless I'm missing something aren't you now
> > copying every linear skb frag_list? With the current code we
> > just do a clone and add the headers.
>
> No copy at all.
>
> As explained, all RX skb are now backed to a page frag. (see
> skb->head_frag)
Even if we have called pskb_expand_head on it?
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: Linux sends IPv6 NS packets with the link-local address
From: Steinar H. Gunderson @ 2013-11-07 0:56 UTC (permalink / raw)
To: netdev, ayourtch
In-Reply-To: <20131107004610.GB8144@order.stressinduktion.org>
On Thu, Nov 07, 2013 at 01:46:11AM +0100, Hannes Frederic Sowa wrote:
> Problem was, if we enqueue a packet to the resolving queue we use those
> packets source address as the address we use as the source address. This
> patch chooses the last enqueued skb's source address to do the resolving
> thus switching over to a global address much more early (hopefully). I
> have seen this problem with routers doing uRPF on the source address of
> arp packets, too.
>
> Does this happen when you configure addresses? Do your addresses have
> a short lifetime advertised by radvd etc?
There are no autoconfigured addresses on the machine; statically set plus
link-layer only. It does _serve_ RA for another VLAN, though (it does
802.1q, some tunnels, and some BGP).
/* Steinar */
--
Homepage: http://www.sesse.net/
^ 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