* [PATCH net-next v4 0/4] TC: refactor act_mirred packets re-injection
@ 2018-07-26 14:34 Paolo Abeni
2018-07-26 14:34 ` [PATCH net-next v4 1/4] net/sched: user-space can't set unknown tcfa_action values Paolo Abeni
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Paolo Abeni @ 2018-07-26 14:34 UTC (permalink / raw)
To: netdev
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, Daniel Borkmann,
Marcelo Ricardo Leitner, Eyal Birger, David S. Miller
This series is aimed at improving the act_mirred redirect performances.
Such action is used by OVS to represent TC S/W flows, and it's current largest
bottle-neck is the need for a skb_clone() for each packet.
The first 2 patches introduce some cleanup and safeguards to allow extending
tca_result - we will use it to store RCU protected redirect information - and
introduce a clear separation between user-space accessible tcfa_action
values and internal values accessible only by the kernel.
Then a new tcfa_action value is introduced: TC_ACT_REINJECT, similar to
TC_ACT_REDIRECT, but preserving the mirred semantic. Such value is not
accessible from user-space.
The last patch exploits the newly introduced infrastructure in the act_mirred
action, to avoid a skb_clone, when possible.
Overall this the above gives a ~10% performance improvement in forwarding tput,
when using the TC S/W datapath.
v1 -> v2:
- preserve the rcu lock in act_bpf
- add and use a new action value to reinject the packets, preserving the mirred
semantic
v2 -> v3:
- renamed to new action as TC_ACT_REINJECT
- TC_ACT_REINJECT is not exposed to user-space
v3 -> v4:
- dropped the TC_ACT_REDIRECT patch
- report failure via extack, too
- rename the new action as TC_ACT_REINSERT
- skip clone only if the control action don't touch tcf_result
Paolo Abeni (4):
net/sched: user-space can't set unknown tcfa_action values
tc/act: remove unneeded RCU lock in action callback
net/tc: introduce TC_ACT_REINSERT.
act_mirred: use TC_ACT_REINSERT when possible
include/net/act_api.h | 2 +-
include/net/pkt_cls.h | 3 ++
include/net/sch_generic.h | 21 +++++++++++++
include/uapi/linux/pkt_cls.h | 6 ++--
net/core/dev.c | 6 +++-
net/sched/act_api.c | 17 +++++++++++
net/sched/act_csum.c | 12 ++------
net/sched/act_ife.c | 5 +--
net/sched/act_mirred.c | 59 +++++++++++++++++++++++++++---------
net/sched/act_sample.c | 4 +--
net/sched/act_skbedit.c | 10 ++----
net/sched/act_skbmod.c | 21 ++++++-------
net/sched/act_tunnel_key.c | 6 +---
net/sched/act_vlan.c | 19 +++++-------
14 files changed, 121 insertions(+), 70 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH net-next v4 1/4] net/sched: user-space can't set unknown tcfa_action values
2018-07-26 14:34 [PATCH net-next v4 0/4] TC: refactor act_mirred packets re-injection Paolo Abeni
@ 2018-07-26 14:34 ` Paolo Abeni
2018-07-27 0:28 ` Marcelo Ricardo Leitner
2018-07-26 14:34 ` [PATCH net-next v4 2/4] tc/act: remove unneeded RCU lock in action callback Paolo Abeni
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Paolo Abeni @ 2018-07-26 14:34 UTC (permalink / raw)
To: netdev
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, Daniel Borkmann,
Marcelo Ricardo Leitner, Eyal Birger, David S. Miller
Currently, when initializing an action, the user-space can specify
and use arbitrary values for the tcfa_action field. If the value
is unknown by the kernel, is implicitly threaded as TC_ACT_UNSPEC.
This change explicitly checks for unknown values at action creation
time, and explicitly convert them to TC_ACT_UNSPEC. No functional
changes are introduced, but this will allow introducing tcfa_action
values not exposed to user-space in a later patch.
Note: we can't use the above to hide TC_ACT_REDIRECT from user-space,
as the latter is already part of uAPI.
v3 -> v4:
- use an helper to check for action validity (JiriP)
- emit an extack for invalid actions (JiriP)
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
include/uapi/linux/pkt_cls.h | 6 ++++--
net/sched/act_api.c | 17 +++++++++++++++++
2 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index b4512254036b..48e5b5d49a34 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -45,6 +45,7 @@ enum {
* the skb and act like everything
* is alright.
*/
+#define TC_ACT_VALUE_MAX TC_ACT_TRAP
/* There is a special kind of actions called "extended actions",
* which need a value parameter. These have a local opcode located in
@@ -55,11 +56,12 @@ enum {
#define __TC_ACT_EXT_SHIFT 28
#define __TC_ACT_EXT(local) ((local) << __TC_ACT_EXT_SHIFT)
#define TC_ACT_EXT_VAL_MASK ((1 << __TC_ACT_EXT_SHIFT) - 1)
-#define TC_ACT_EXT_CMP(combined, opcode) \
- (((combined) & (~TC_ACT_EXT_VAL_MASK)) == opcode)
+#define TC_ACT_EXT_OPCODE(combined) ((combined) & (~TC_ACT_EXT_VAL_MASK))
+#define TC_ACT_EXT_CMP(combined, opcode) (TC_ACT_EXT_OPCODE(combined) == opcode)
#define TC_ACT_JUMP __TC_ACT_EXT(1)
#define TC_ACT_GOTO_CHAIN __TC_ACT_EXT(2)
+#define TC_ACT_EXT_OPCODE_MAX TC_ACT_GOTO_CHAIN
/* Action type identifiers*/
enum {
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 148a89ab789b..bdccad583daf 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -786,6 +786,15 @@ static struct tc_cookie *nla_memdup_cookie(struct nlattr **tb)
return c;
}
+static bool tcf_action_valid(int action)
+{
+ int opcode = TC_ACT_EXT_OPCODE(action);
+
+ if (!opcode)
+ return action <= TC_ACT_VALUE_MAX;
+ return opcode <= TC_ACT_EXT_OPCODE_MAX || action == TC_ACT_UNSPEC;
+}
+
struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
struct nlattr *nla, struct nlattr *est,
char *name, int ovr, int bind,
@@ -895,6 +904,14 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
}
}
+ if (!tcf_action_valid(a->tcfa_action)) {
+ net_warn_ratelimited("invalid %d action value, using "
+ "TC_ACT_UNSPEC instead", a->tcfa_action);
+ NL_SET_ERR_MSG(extack, "invalid action value, using "
+ "TC_ACT_UNSPEC instead");
+ a->tcfa_action = TC_ACT_UNSPEC;
+ }
+
return a;
err_mod:
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH net-next v4 2/4] tc/act: remove unneeded RCU lock in action callback
2018-07-26 14:34 [PATCH net-next v4 0/4] TC: refactor act_mirred packets re-injection Paolo Abeni
2018-07-26 14:34 ` [PATCH net-next v4 1/4] net/sched: user-space can't set unknown tcfa_action values Paolo Abeni
@ 2018-07-26 14:34 ` Paolo Abeni
2018-07-26 14:34 ` [PATCH net-next v4 3/4] net/tc: introduce TC_ACT_REINSERT Paolo Abeni
2018-07-26 14:35 ` [PATCH net-next v4 4/4] act_mirred: use TC_ACT_REINSERT when possible Paolo Abeni
3 siblings, 0 replies; 12+ messages in thread
From: Paolo Abeni @ 2018-07-26 14:34 UTC (permalink / raw)
To: netdev
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, Daniel Borkmann,
Marcelo Ricardo Leitner, Eyal Birger, David S. Miller
Each lockless action currently does its own RCU locking in ->act().
This allows using plain RCU accessor, even if the context
is really RCU BH.
This change drops the per action RCU lock, replace the accessors
with the _bh variant, cleans up a bit the surrounding code and
documents the RCU status in the relevant header.
No functional nor performance change is intended.
The goal of this patch is clarifying that the RCU critical section
used by the tc actions extends up to the classifier's caller.
v1 -> v2:
- preserve rcu lock in act_bpf: it's needed by eBPF helpers,
as pointed out by Daniel
v3 -> v4:
- fixed some typos in the commit message (JiriP)
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
include/net/act_api.h | 2 +-
include/net/sch_generic.h | 2 ++
net/sched/act_csum.c | 12 +++---------
net/sched/act_ife.c | 5 +----
net/sched/act_mirred.c | 4 +---
net/sched/act_sample.c | 4 +---
net/sched/act_skbedit.c | 10 +++-------
net/sched/act_skbmod.c | 21 +++++++++------------
net/sched/act_tunnel_key.c | 6 +-----
net/sched/act_vlan.c | 19 +++++++------------
10 files changed, 29 insertions(+), 56 deletions(-)
diff --git a/include/net/act_api.h b/include/net/act_api.h
index 683ce41053d9..8c9bc02d05e1 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -85,7 +85,7 @@ struct tc_action_ops {
size_t size;
struct module *owner;
int (*act)(struct sk_buff *, const struct tc_action *,
- struct tcf_result *);
+ struct tcf_result *); /* called under RCU BH lock*/
int (*dump)(struct sk_buff *, struct tc_action *, int, int);
void (*cleanup)(struct tc_action *);
int (*lookup)(struct net *net, struct tc_action **a, u32 index,
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 085c509c8674..c9af9ce33055 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -285,6 +285,8 @@ struct tcf_proto {
/* Fast access part */
struct tcf_proto __rcu *next;
void __rcu *root;
+
+ /* called under RCU BH lock*/
int (*classify)(struct sk_buff *,
const struct tcf_proto *,
struct tcf_result *);
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index 4e8c383f379e..648a3a35b720 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -561,15 +561,14 @@ static int tcf_csum(struct sk_buff *skb, const struct tc_action *a,
u32 update_flags;
int action;
- rcu_read_lock();
- params = rcu_dereference(p->params);
+ params = rcu_dereference_bh(p->params);
tcf_lastuse_update(&p->tcf_tm);
bstats_cpu_update(this_cpu_ptr(p->common.cpu_bstats), skb);
action = READ_ONCE(p->tcf_action);
if (unlikely(action == TC_ACT_SHOT))
- goto drop_stats;
+ goto drop;
update_flags = params->update_flags;
switch (tc_skb_protocol(skb)) {
@@ -583,16 +582,11 @@ static int tcf_csum(struct sk_buff *skb, const struct tc_action *a,
break;
}
-unlock:
- rcu_read_unlock();
return action;
drop:
- action = TC_ACT_SHOT;
-
-drop_stats:
qstats_drop_inc(this_cpu_ptr(p->common.cpu_qstats));
- goto unlock;
+ return TC_ACT_SHOT;
}
static int tcf_csum_dump(struct sk_buff *skb, struct tc_action *a, int bind,
diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index 3d6e265758c0..df4060e32d43 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -820,14 +820,11 @@ static int tcf_ife_act(struct sk_buff *skb, const struct tc_action *a,
struct tcf_ife_params *p;
int ret;
- rcu_read_lock();
- p = rcu_dereference(ife->params);
+ p = rcu_dereference_bh(ife->params);
if (p->flags & IFE_ENCODE) {
ret = tcf_ife_encode(skb, a, res, p);
- rcu_read_unlock();
return ret;
}
- rcu_read_unlock();
return tcf_ife_decode(skb, a, res);
}
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 6afd89a36c69..eeb335f03102 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -181,11 +181,10 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
tcf_lastuse_update(&m->tcf_tm);
bstats_cpu_update(this_cpu_ptr(m->common.cpu_bstats), skb);
- rcu_read_lock();
m_mac_header_xmit = READ_ONCE(m->tcfm_mac_header_xmit);
m_eaction = READ_ONCE(m->tcfm_eaction);
retval = READ_ONCE(m->tcf_action);
- dev = rcu_dereference(m->tcfm_dev);
+ dev = rcu_dereference_bh(m->tcfm_dev);
if (unlikely(!dev)) {
pr_notice_once("tc mirred: target device is gone\n");
goto out;
@@ -236,7 +235,6 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
if (tcf_mirred_is_act_redirect(m_eaction))
retval = TC_ACT_SHOT;
}
- rcu_read_unlock();
return retval;
}
diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
index 3079e7be5bde..2608ccc83e5e 100644
--- a/net/sched/act_sample.c
+++ b/net/sched/act_sample.c
@@ -140,8 +140,7 @@ static int tcf_sample_act(struct sk_buff *skb, const struct tc_action *a,
bstats_cpu_update(this_cpu_ptr(s->common.cpu_bstats), skb);
retval = READ_ONCE(s->tcf_action);
- rcu_read_lock();
- psample_group = rcu_dereference(s->psample_group);
+ psample_group = rcu_dereference_bh(s->psample_group);
/* randomly sample packets according to rate */
if (psample_group && (prandom_u32() % s->rate == 0)) {
@@ -165,7 +164,6 @@ static int tcf_sample_act(struct sk_buff *skb, const struct tc_action *a,
skb_pull(skb, skb->mac_len);
}
- rcu_read_unlock();
return retval;
}
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index da56e6938c9e..a6db47ebec11 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -43,8 +43,7 @@ static int tcf_skbedit(struct sk_buff *skb, const struct tc_action *a,
tcf_lastuse_update(&d->tcf_tm);
bstats_cpu_update(this_cpu_ptr(d->common.cpu_bstats), skb);
- rcu_read_lock();
- params = rcu_dereference(d->params);
+ params = rcu_dereference_bh(d->params);
action = READ_ONCE(d->tcf_action);
if (params->flags & SKBEDIT_F_PRIORITY)
@@ -77,14 +76,11 @@ static int tcf_skbedit(struct sk_buff *skb, const struct tc_action *a,
}
if (params->flags & SKBEDIT_F_PTYPE)
skb->pkt_type = params->ptype;
-
-unlock:
- rcu_read_unlock();
return action;
+
err:
qstats_drop_inc(this_cpu_ptr(d->common.cpu_qstats));
- action = TC_ACT_SHOT;
- goto unlock;
+ return TC_ACT_SHOT;
}
static const struct nla_policy skbedit_policy[TCA_SKBEDIT_MAX + 1] = {
diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
index cdc6bacfb190..c437c6d51a71 100644
--- a/net/sched/act_skbmod.c
+++ b/net/sched/act_skbmod.c
@@ -41,20 +41,14 @@ static int tcf_skbmod_run(struct sk_buff *skb, const struct tc_action *a,
* then MAX_EDIT_LEN needs to change appropriately
*/
err = skb_ensure_writable(skb, MAX_EDIT_LEN);
- if (unlikely(err)) { /* best policy is to drop on the floor */
- qstats_overlimit_inc(this_cpu_ptr(d->common.cpu_qstats));
- return TC_ACT_SHOT;
- }
+ if (unlikely(err)) /* best policy is to drop on the floor */
+ goto drop;
- rcu_read_lock();
action = READ_ONCE(d->tcf_action);
- if (unlikely(action == TC_ACT_SHOT)) {
- qstats_overlimit_inc(this_cpu_ptr(d->common.cpu_qstats));
- rcu_read_unlock();
- return action;
- }
+ if (unlikely(action == TC_ACT_SHOT))
+ goto drop;
- p = rcu_dereference(d->skbmod_p);
+ p = rcu_dereference_bh(d->skbmod_p);
flags = p->flags;
if (flags & SKBMOD_F_DMAC)
ether_addr_copy(eth_hdr(skb)->h_dest, p->eth_dst);
@@ -62,7 +56,6 @@ static int tcf_skbmod_run(struct sk_buff *skb, const struct tc_action *a,
ether_addr_copy(eth_hdr(skb)->h_source, p->eth_src);
if (flags & SKBMOD_F_ETYPE)
eth_hdr(skb)->h_proto = p->eth_type;
- rcu_read_unlock();
if (flags & SKBMOD_F_SWAPMAC) {
u16 tmpaddr[ETH_ALEN / 2]; /* ether_addr_copy() requirement */
@@ -73,6 +66,10 @@ static int tcf_skbmod_run(struct sk_buff *skb, const struct tc_action *a,
}
return action;
+
+drop:
+ qstats_overlimit_inc(this_cpu_ptr(d->common.cpu_qstats));
+ return TC_ACT_SHOT;
}
static const struct nla_policy skbmod_policy[TCA_SKBMOD_MAX + 1] = {
diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
index f811850fd1d0..d42d9e112789 100644
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -31,9 +31,7 @@ static int tunnel_key_act(struct sk_buff *skb, const struct tc_action *a,
struct tcf_tunnel_key_params *params;
int action;
- rcu_read_lock();
-
- params = rcu_dereference(t->params);
+ params = rcu_dereference_bh(t->params);
tcf_lastuse_update(&t->tcf_tm);
bstats_cpu_update(this_cpu_ptr(t->common.cpu_bstats), skb);
@@ -53,8 +51,6 @@ static int tunnel_key_act(struct sk_buff *skb, const struct tc_action *a,
break;
}
- rcu_read_unlock();
-
return action;
}
diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
index ad37f308175a..15a0ee214c9c 100644
--- a/net/sched/act_vlan.c
+++ b/net/sched/act_vlan.c
@@ -40,11 +40,9 @@ static int tcf_vlan(struct sk_buff *skb, const struct tc_action *a,
if (skb_at_tc_ingress(skb))
skb_push_rcsum(skb, skb->mac_len);
- rcu_read_lock();
-
action = READ_ONCE(v->tcf_action);
- p = rcu_dereference(v->vlan_p);
+ p = rcu_dereference_bh(v->vlan_p);
switch (p->tcfv_action) {
case TCA_VLAN_ACT_POP:
@@ -61,7 +59,7 @@ static int tcf_vlan(struct sk_buff *skb, const struct tc_action *a,
case TCA_VLAN_ACT_MODIFY:
/* No-op if no vlan tag (either hw-accel or in-payload) */
if (!skb_vlan_tagged(skb))
- goto unlock;
+ goto out;
/* extract existing tag (and guarantee no hw-accel tag) */
if (skb_vlan_tag_present(skb)) {
tci = skb_vlan_tag_get(skb);
@@ -86,18 +84,15 @@ static int tcf_vlan(struct sk_buff *skb, const struct tc_action *a,
BUG();
}
- goto unlock;
-
-drop:
- action = TC_ACT_SHOT;
- qstats_drop_inc(this_cpu_ptr(v->common.cpu_qstats));
-
-unlock:
- rcu_read_unlock();
+out:
if (skb_at_tc_ingress(skb))
skb_pull_rcsum(skb, skb->mac_len);
return action;
+
+drop:
+ qstats_drop_inc(this_cpu_ptr(v->common.cpu_qstats));
+ return TC_ACT_SHOT;
}
static const struct nla_policy vlan_policy[TCA_VLAN_MAX + 1] = {
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH net-next v4 3/4] net/tc: introduce TC_ACT_REINSERT.
2018-07-26 14:34 [PATCH net-next v4 0/4] TC: refactor act_mirred packets re-injection Paolo Abeni
2018-07-26 14:34 ` [PATCH net-next v4 1/4] net/sched: user-space can't set unknown tcfa_action values Paolo Abeni
2018-07-26 14:34 ` [PATCH net-next v4 2/4] tc/act: remove unneeded RCU lock in action callback Paolo Abeni
@ 2018-07-26 14:34 ` Paolo Abeni
2018-07-26 23:29 ` Cong Wang
` (2 more replies)
2018-07-26 14:35 ` [PATCH net-next v4 4/4] act_mirred: use TC_ACT_REINSERT when possible Paolo Abeni
3 siblings, 3 replies; 12+ messages in thread
From: Paolo Abeni @ 2018-07-26 14:34 UTC (permalink / raw)
To: netdev
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, Daniel Borkmann,
Marcelo Ricardo Leitner, Eyal Birger, David S. Miller
This is similar TC_ACT_REDIRECT, but with a slightly different
semantic:
- on ingress the mirred skbs are passed to the target device
network stack without any additional check not scrubbing.
- the rcu-protected stats provided via the tcf_result struct
are updated on error conditions.
This new tcfa_action value is not exposed to the user-space
and can be used only internally by clsact.
v1 -> v2: do not touch TC_ACT_REDIRECT code path, introduce
a new action type instead
v2 -> v3:
- rename the new action value TC_ACT_REINJECT, update the
helper accordingly
- take care of uncloned reinjected packets in XDP generic
hook
v3 -> v4:
- renamed again the new action value (JiriP)
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
Note: this patch still touch only overlimits, even there is some
agreement to touch (also) drops on reinsert/mirred failure, but
such change is independent to this series
---
include/net/pkt_cls.h | 3 +++
include/net/sch_generic.h | 19 +++++++++++++++++++
net/core/dev.c | 6 +++++-
3 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index a3101582f642..d64915273358 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -7,6 +7,9 @@
#include <net/sch_generic.h>
#include <net/act_api.h>
+/* TC action not accessible from user space */
+#define TC_ACT_REINSERT (TC_ACT_VALUE_MAX + 1)
+
/* Basic packet classifier frontend definitions. */
struct tcf_walker {
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index c9af9ce33055..70270d9c1bb1 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -235,6 +235,12 @@ struct tcf_result {
u32 classid;
};
const struct tcf_proto *goto_tp;
+
+ /* used by the TC_ACT_REINSERT action */
+ struct {
+ bool ingress;
+ struct gnet_stats_queue *qstats;
+ };
};
};
@@ -1107,4 +1113,17 @@ void mini_qdisc_pair_swap(struct mini_Qdisc_pair *miniqp,
void mini_qdisc_pair_init(struct mini_Qdisc_pair *miniqp, struct Qdisc *qdisc,
struct mini_Qdisc __rcu **p_miniq);
+static inline void skb_tc_reinsert(struct sk_buff *skb, struct tcf_result *res)
+{
+ struct gnet_stats_queue *stats = res->qstats;
+ int ret;
+
+ if (res->ingress)
+ ret = netif_receive_skb(skb);
+ else
+ ret = dev_queue_xmit(skb);
+ if (ret && stats)
+ qstats_overlimit_inc(res->qstats);
+}
+
#endif
diff --git a/net/core/dev.c b/net/core/dev.c
index 87c42c8249ae..d5faa6449fb2 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4252,7 +4252,7 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
/* Reinjected packets coming from act_mirred or similar should
* not get XDP generic processing.
*/
- if (skb_cloned(skb))
+ if (skb_cloned(skb) || skb->tc_redirected)
return XDP_PASS;
/* XDP packets must be linear and must have sufficient headroom
@@ -4602,6 +4602,10 @@ sch_handle_ingress(struct sk_buff *skb, struct packet_type **pt_prev, int *ret,
__skb_push(skb, skb->mac_len);
skb_do_redirect(skb);
return NULL;
+ case TC_ACT_REINSERT:
+ /* this does not scrub the packet, and updates stats on error */
+ skb_tc_reinsert(skb, &cl_res);
+ return NULL;
default:
break;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH net-next v4 4/4] act_mirred: use TC_ACT_REINSERT when possible
2018-07-26 14:34 [PATCH net-next v4 0/4] TC: refactor act_mirred packets re-injection Paolo Abeni
` (2 preceding siblings ...)
2018-07-26 14:34 ` [PATCH net-next v4 3/4] net/tc: introduce TC_ACT_REINSERT Paolo Abeni
@ 2018-07-26 14:35 ` Paolo Abeni
2018-07-26 23:27 ` Cong Wang
2018-07-29 2:58 ` kbuild test robot
3 siblings, 2 replies; 12+ messages in thread
From: Paolo Abeni @ 2018-07-26 14:35 UTC (permalink / raw)
To: netdev
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, Daniel Borkmann,
Marcelo Ricardo Leitner, Eyal Birger, David S. Miller
When mirred is invoked from the ingress path, and it wants to redirect
the processed packet, it can now use the TC_ACT_REINSERT action,
filling the tcf_result accordingly, and avoiding a per packet
skb_clone().
Overall this gives a ~10% improvement in forwarding performance for the
TC S/W data path and TC S/W performances are now comparable to the
kernel openvswitch datapath.
v1 -> v2: use ACT_MIRRED instead of ACT_REDIRECT
v2 -> v3: updated after action rename, fixed typo into the commit
message
v3 -> v4: updated again after action rename, added more comments to
the code (JiriP), skip the optimization if the control action
need to touch the tcf_result (Paolo)
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/sched/act_mirred.c | 55 +++++++++++++++++++++++++++++++++---------
1 file changed, 44 insertions(+), 11 deletions(-)
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index eeb335f03102..d5f1a8102b7b 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -25,6 +25,7 @@
#include <net/net_namespace.h>
#include <net/netlink.h>
#include <net/pkt_sched.h>
+#include <net/pkt_cls.h>
#include <linux/tc_act/tc_mirred.h>
#include <net/tc_act/tc_mirred.h>
@@ -49,6 +50,18 @@ static bool tcf_mirred_act_wants_ingress(int action)
}
}
+static bool tcf_mirred_can_reinsert(int action)
+{
+ switch (action) {
+ case TC_ACT_SHOT:
+ case TC_ACT_STOLEN:
+ case TC_ACT_QUEUED:
+ case TC_ACT_TRAP:
+ return true;
+ }
+ return false;
+}
+
static void tcf_mirred_release(struct tc_action *a)
{
struct tcf_mirred *m = to_mirred(a);
@@ -171,10 +184,13 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
struct tcf_result *res)
{
struct tcf_mirred *m = to_mirred(a);
+ struct sk_buff *skb2 = skb;
bool m_mac_header_xmit;
struct net_device *dev;
- struct sk_buff *skb2;
int retval, err = 0;
+ bool use_reinsert;
+ bool want_ingress;
+ bool is_redirect;
int m_eaction;
int mac_len;
@@ -196,16 +212,25 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
goto out;
}
- skb2 = skb_clone(skb, GFP_ATOMIC);
- if (!skb2)
- goto out;
+ /* we could easily avoid the clone only if called by ingress and clsact;
+ * since we can't easily detect the clsact caller, skip clone only for
+ * ingress - that covers the TC S/W datapath.
+ */
+ is_redirect = tcf_mirred_is_act_redirect(m_eaction);
+ use_reinsert = skb_at_tc_ingress(skb) && is_redirect &&
+ tcf_mirred_can_reinsert(retval);
+ if (!use_reinsert) {
+ skb2 = skb_clone(skb, GFP_ATOMIC);
+ if (!skb2)
+ goto out;
+ }
/* If action's target direction differs than filter's direction,
* and devices expect a mac header on xmit, then mac push/pull is
* needed.
*/
- if (skb_at_tc_ingress(skb) != tcf_mirred_act_wants_ingress(m_eaction) &&
- m_mac_header_xmit) {
+ want_ingress = tcf_mirred_act_wants_ingress(m_eaction);
+ if (skb_at_tc_ingress(skb) != want_ingress && m_mac_header_xmit) {
if (!skb_at_tc_ingress(skb)) {
/* caught at egress, act ingress: pull mac */
mac_len = skb_network_header(skb) - skb_mac_header(skb);
@@ -216,15 +241,23 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
}
}
+ skb2->skb_iif = skb->dev->ifindex;
+ skb2->dev = dev;
+
/* mirror is always swallowed */
- if (tcf_mirred_is_act_redirect(m_eaction)) {
+ if (is_redirect) {
skb2->tc_redirected = 1;
skb2->tc_from_ingress = skb2->tc_at_ingress;
+
+ /* let's the caller reinsert the packet, if possible */
+ if (use_reinsert) {
+ res->ingress = want_ingress;
+ res->qstats = this_cpu_ptr(m->common.cpu_qstats);
+ return TC_ACT_REINSERT;
+ }
}
- skb2->skb_iif = skb->dev->ifindex;
- skb2->dev = dev;
- if (!tcf_mirred_act_wants_ingress(m_eaction))
+ if (!want_ingress)
err = dev_queue_xmit(skb2);
else
err = netif_receive_skb(skb2);
@@ -232,7 +265,7 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
if (err) {
out:
qstats_overlimit_inc(this_cpu_ptr(m->common.cpu_qstats));
- if (tcf_mirred_is_act_redirect(m_eaction))
+ if (is_redirect)
retval = TC_ACT_SHOT;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH net-next v4 4/4] act_mirred: use TC_ACT_REINSERT when possible
2018-07-26 14:35 ` [PATCH net-next v4 4/4] act_mirred: use TC_ACT_REINSERT when possible Paolo Abeni
@ 2018-07-26 23:27 ` Cong Wang
2018-07-29 2:58 ` kbuild test robot
1 sibling, 0 replies; 12+ messages in thread
From: Cong Wang @ 2018-07-26 23:27 UTC (permalink / raw)
To: Paolo Abeni
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
Daniel Borkmann, Marcelo Ricardo Leitner, Eyal Birger,
David Miller
On Thu, Jul 26, 2018 at 7:35 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> When mirred is invoked from the ingress path, and it wants to redirect
> the processed packet, it can now use the TC_ACT_REINSERT action,
> filling the tcf_result accordingly, and avoiding a per packet
> skb_clone().
>
> Overall this gives a ~10% improvement in forwarding performance for the
> TC S/W data path and TC S/W performances are now comparable to the
> kernel openvswitch datapath.
>
> v1 -> v2: use ACT_MIRRED instead of ACT_REDIRECT
> v2 -> v3: updated after action rename, fixed typo into the commit
> message
> v3 -> v4: updated again after action rename, added more comments to
> the code (JiriP), skip the optimization if the control action
> need to touch the tcf_result (Paolo)
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
Overall it looks good to me now.
Acked-by: Cong Wang <xiyou.wangcong@gmail.com>
Thanks!
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next v4 3/4] net/tc: introduce TC_ACT_REINSERT.
2018-07-26 14:34 ` [PATCH net-next v4 3/4] net/tc: introduce TC_ACT_REINSERT Paolo Abeni
@ 2018-07-26 23:29 ` Cong Wang
2018-07-29 3:21 ` kbuild test robot
2018-07-29 3:34 ` kbuild test robot
2 siblings, 0 replies; 12+ messages in thread
From: Cong Wang @ 2018-07-26 23:29 UTC (permalink / raw)
To: Paolo Abeni
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
Daniel Borkmann, Marcelo Ricardo Leitner, Eyal Birger,
David Miller
On Thu, Jul 26, 2018 at 7:35 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> This is similar TC_ACT_REDIRECT, but with a slightly different
> semantic:
> - on ingress the mirred skbs are passed to the target device
> network stack without any additional check not scrubbing.
> - the rcu-protected stats provided via the tcf_result struct
> are updated on error conditions.
>
> This new tcfa_action value is not exposed to the user-space
> and can be used only internally by clsact.
>
> v1 -> v2: do not touch TC_ACT_REDIRECT code path, introduce
> a new action type instead
>
> v2 -> v3:
> - rename the new action value TC_ACT_REINJECT, update the
> helper accordingly
> - take care of uncloned reinjected packets in XDP generic
> hook
>
> v3 -> v4:
> - renamed again the new action value (JiriP)
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
Acked-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
> Note: this patch still touch only overlimits, even there is some
> agreement to touch (also) drops on reinsert/mirred failure, but
> such change is independent to this series
Totally agree.
Thanks!
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next v4 1/4] net/sched: user-space can't set unknown tcfa_action values
2018-07-26 14:34 ` [PATCH net-next v4 1/4] net/sched: user-space can't set unknown tcfa_action values Paolo Abeni
@ 2018-07-27 0:28 ` Marcelo Ricardo Leitner
2018-07-27 13:08 ` Paolo Abeni
0 siblings, 1 reply; 12+ messages in thread
From: Marcelo Ricardo Leitner @ 2018-07-27 0:28 UTC (permalink / raw)
To: Paolo Abeni
Cc: netdev, Jamal Hadi Salim, Cong Wang, Jiri Pirko, Daniel Borkmann,
Eyal Birger, David S. Miller
Hi,
On Thu, Jul 26, 2018 at 04:34:57PM +0200, Paolo Abeni wrote:
...
> @@ -895,6 +904,14 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
> }
> }
>
> + if (!tcf_action_valid(a->tcfa_action)) {
> + net_warn_ratelimited("invalid %d action value, using "
> + "TC_ACT_UNSPEC instead", a->tcfa_action);
Now that it is reporting the error via extack, do we really need this
warn net_warn?
extack will be shown as a warning by iproute2 even if the command
succeeds.
> + NL_SET_ERR_MSG(extack, "invalid action value, using "
> + "TC_ACT_UNSPEC instead");
Quoted strings shouldn't be broken down into multiple lines..
> + a->tcfa_action = TC_ACT_UNSPEC;
> + }
> +
> return a;
>
> err_mod:
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next v4 1/4] net/sched: user-space can't set unknown tcfa_action values
2018-07-27 0:28 ` Marcelo Ricardo Leitner
@ 2018-07-27 13:08 ` Paolo Abeni
0 siblings, 0 replies; 12+ messages in thread
From: Paolo Abeni @ 2018-07-27 13:08 UTC (permalink / raw)
To: Marcelo Ricardo Leitner, Jiri Pirko
Cc: netdev, Jamal Hadi Salim, Cong Wang, Daniel Borkmann,
Eyal Birger, David S. Miller
On Thu, 2018-07-26 at 21:28 -0300, Marcelo Ricardo Leitner wrote:
> Hi,
>
> On Thu, Jul 26, 2018 at 04:34:57PM +0200, Paolo Abeni wrote:
> ...
> > @@ -895,6 +904,14 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
> > }
> > }
> >
> > + if (!tcf_action_valid(a->tcfa_action)) {
> > + net_warn_ratelimited("invalid %d action value, using "
> > + "TC_ACT_UNSPEC instead", a->tcfa_action);
>
> Now that it is reporting the error via extack, do we really need this
> warn net_warn?
> extack will be shown as a warning by iproute2 even if the command
> succeeds.
That was requested by Jiri (modulo misinterpretation on my side).
My understanding is that the extact will warn the whoever tryed to push
the bad configuration, while the net_warn is targeting the hosts
administrator.
Jiri, do you have strong opinion on this or did I misinterpret your
wording/ can I drop the net_warn?
Thanks!
> > + NL_SET_ERR_MSG(extack, "invalid action value, using "
> > + "TC_ACT_UNSPEC instead");
>
> Quoted strings shouldn't be broken down into multiple lines..
Thanks,
will fix in v5 :(
Cheers,
Paolo
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next v4 4/4] act_mirred: use TC_ACT_REINSERT when possible
2018-07-26 14:35 ` [PATCH net-next v4 4/4] act_mirred: use TC_ACT_REINSERT when possible Paolo Abeni
2018-07-26 23:27 ` Cong Wang
@ 2018-07-29 2:58 ` kbuild test robot
1 sibling, 0 replies; 12+ messages in thread
From: kbuild test robot @ 2018-07-29 2:58 UTC (permalink / raw)
To: Paolo Abeni
Cc: kbuild-all, netdev, Jamal Hadi Salim, Cong Wang, Jiri Pirko,
Daniel Borkmann, Marcelo Ricardo Leitner, Eyal Birger,
David S. Miller
[-- Attachment #1: Type: text/plain, Size: 4288 bytes --]
Hi Paolo,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
url: https://github.com/0day-ci/linux/commits/Paolo-Abeni/TC-refactor-act_mirred-packets-re-injection/20180729-102154
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 8.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=8.1.0 make.cross ARCH=xtensa
Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings
All warnings (new ones prefixed by >>):
net/sched/act_mirred.c: In function 'tcf_mirred':
>> net/sched/act_mirred.c:268:6: warning: 'is_redirect' may be used uninitialized in this function [-Wmaybe-uninitialized]
if (is_redirect)
^
vim +/is_redirect +268 net/sched/act_mirred.c
182
183 static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
184 struct tcf_result *res)
185 {
186 struct tcf_mirred *m = to_mirred(a);
187 struct sk_buff *skb2 = skb;
188 bool m_mac_header_xmit;
189 struct net_device *dev;
190 int retval, err = 0;
191 bool use_reinsert;
192 bool want_ingress;
193 bool is_redirect;
194 int m_eaction;
195 int mac_len;
196
197 tcf_lastuse_update(&m->tcf_tm);
198 bstats_cpu_update(this_cpu_ptr(m->common.cpu_bstats), skb);
199
200 m_mac_header_xmit = READ_ONCE(m->tcfm_mac_header_xmit);
201 m_eaction = READ_ONCE(m->tcfm_eaction);
202 retval = READ_ONCE(m->tcf_action);
203 dev = rcu_dereference_bh(m->tcfm_dev);
204 if (unlikely(!dev)) {
205 pr_notice_once("tc mirred: target device is gone\n");
206 goto out;
207 }
208
209 if (unlikely(!(dev->flags & IFF_UP))) {
210 net_notice_ratelimited("tc mirred to Houston: device %s is down\n",
211 dev->name);
212 goto out;
213 }
214
215 /* we could easily avoid the clone only if called by ingress and clsact;
216 * since we can't easily detect the clsact caller, skip clone only for
217 * ingress - that covers the TC S/W datapath.
218 */
219 is_redirect = tcf_mirred_is_act_redirect(m_eaction);
220 use_reinsert = skb_at_tc_ingress(skb) && is_redirect &&
221 tcf_mirred_can_reinsert(retval);
222 if (!use_reinsert) {
223 skb2 = skb_clone(skb, GFP_ATOMIC);
224 if (!skb2)
225 goto out;
226 }
227
228 /* If action's target direction differs than filter's direction,
229 * and devices expect a mac header on xmit, then mac push/pull is
230 * needed.
231 */
232 want_ingress = tcf_mirred_act_wants_ingress(m_eaction);
233 if (skb_at_tc_ingress(skb) != want_ingress && m_mac_header_xmit) {
234 if (!skb_at_tc_ingress(skb)) {
235 /* caught at egress, act ingress: pull mac */
236 mac_len = skb_network_header(skb) - skb_mac_header(skb);
237 skb_pull_rcsum(skb2, mac_len);
238 } else {
239 /* caught at ingress, act egress: push mac */
240 skb_push_rcsum(skb2, skb->mac_len);
241 }
242 }
243
244 skb2->skb_iif = skb->dev->ifindex;
245 skb2->dev = dev;
246
247 /* mirror is always swallowed */
248 if (is_redirect) {
249 skb2->tc_redirected = 1;
250 skb2->tc_from_ingress = skb2->tc_at_ingress;
251
252 /* let's the caller reinsert the packet, if possible */
253 if (use_reinsert) {
254 res->ingress = want_ingress;
255 res->qstats = this_cpu_ptr(m->common.cpu_qstats);
256 return TC_ACT_REINSERT;
257 }
258 }
259
260 if (!want_ingress)
261 err = dev_queue_xmit(skb2);
262 else
263 err = netif_receive_skb(skb2);
264
265 if (err) {
266 out:
267 qstats_overlimit_inc(this_cpu_ptr(m->common.cpu_qstats));
> 268 if (is_redirect)
269 retval = TC_ACT_SHOT;
270 }
271
272 return retval;
273 }
274
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 54195 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next v4 3/4] net/tc: introduce TC_ACT_REINSERT.
2018-07-26 14:34 ` [PATCH net-next v4 3/4] net/tc: introduce TC_ACT_REINSERT Paolo Abeni
2018-07-26 23:29 ` Cong Wang
@ 2018-07-29 3:21 ` kbuild test robot
2018-07-29 3:34 ` kbuild test robot
2 siblings, 0 replies; 12+ messages in thread
From: kbuild test robot @ 2018-07-29 3:21 UTC (permalink / raw)
To: Paolo Abeni
Cc: kbuild-all, netdev, Jamal Hadi Salim, Cong Wang, Jiri Pirko,
Daniel Borkmann, Marcelo Ricardo Leitner, Eyal Birger,
David S. Miller
[-- Attachment #1: Type: text/plain, Size: 3941 bytes --]
Hi Paolo,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net-next/master]
url: https://github.com/0day-ci/linux/commits/Paolo-Abeni/TC-refactor-act_mirred-packets-re-injection/20180729-102154
config: i386-randconfig-n0-201829 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
net//core/dev.c: In function 'netif_receive_generic_xdp':
>> net//core/dev.c:4255:28: error: 'struct sk_buff' has no member named 'tc_redirected'
if (skb_cloned(skb) || skb->tc_redirected)
^~
vim +4255 net//core/dev.c
4241
4242 static u32 netif_receive_generic_xdp(struct sk_buff *skb,
4243 struct xdp_buff *xdp,
4244 struct bpf_prog *xdp_prog)
4245 {
4246 struct netdev_rx_queue *rxqueue;
4247 void *orig_data, *orig_data_end;
4248 u32 metalen, act = XDP_DROP;
4249 int hlen, off;
4250 u32 mac_len;
4251
4252 /* Reinjected packets coming from act_mirred or similar should
4253 * not get XDP generic processing.
4254 */
> 4255 if (skb_cloned(skb) || skb->tc_redirected)
4256 return XDP_PASS;
4257
4258 /* XDP packets must be linear and must have sufficient headroom
4259 * of XDP_PACKET_HEADROOM bytes. This is the guarantee that also
4260 * native XDP provides, thus we need to do it here as well.
4261 */
4262 if (skb_is_nonlinear(skb) ||
4263 skb_headroom(skb) < XDP_PACKET_HEADROOM) {
4264 int hroom = XDP_PACKET_HEADROOM - skb_headroom(skb);
4265 int troom = skb->tail + skb->data_len - skb->end;
4266
4267 /* In case we have to go down the path and also linearize,
4268 * then lets do the pskb_expand_head() work just once here.
4269 */
4270 if (pskb_expand_head(skb,
4271 hroom > 0 ? ALIGN(hroom, NET_SKB_PAD) : 0,
4272 troom > 0 ? troom + 128 : 0, GFP_ATOMIC))
4273 goto do_drop;
4274 if (skb_linearize(skb))
4275 goto do_drop;
4276 }
4277
4278 /* The XDP program wants to see the packet starting at the MAC
4279 * header.
4280 */
4281 mac_len = skb->data - skb_mac_header(skb);
4282 hlen = skb_headlen(skb) + mac_len;
4283 xdp->data = skb->data - mac_len;
4284 xdp->data_meta = xdp->data;
4285 xdp->data_end = xdp->data + hlen;
4286 xdp->data_hard_start = skb->data - skb_headroom(skb);
4287 orig_data_end = xdp->data_end;
4288 orig_data = xdp->data;
4289
4290 rxqueue = netif_get_rxqueue(skb);
4291 xdp->rxq = &rxqueue->xdp_rxq;
4292
4293 act = bpf_prog_run_xdp(xdp_prog, xdp);
4294
4295 off = xdp->data - orig_data;
4296 if (off > 0)
4297 __skb_pull(skb, off);
4298 else if (off < 0)
4299 __skb_push(skb, -off);
4300 skb->mac_header += off;
4301
4302 /* check if bpf_xdp_adjust_tail was used. it can only "shrink"
4303 * pckt.
4304 */
4305 off = orig_data_end - xdp->data_end;
4306 if (off != 0) {
4307 skb_set_tail_pointer(skb, xdp->data_end - xdp->data);
4308 skb->len -= off;
4309
4310 }
4311
4312 switch (act) {
4313 case XDP_REDIRECT:
4314 case XDP_TX:
4315 __skb_push(skb, mac_len);
4316 break;
4317 case XDP_PASS:
4318 metalen = xdp->data - xdp->data_meta;
4319 if (metalen)
4320 skb_metadata_set(skb, metalen);
4321 break;
4322 default:
4323 bpf_warn_invalid_xdp_action(act);
4324 /* fall through */
4325 case XDP_ABORTED:
4326 trace_xdp_exception(skb->dev, xdp_prog, act);
4327 /* fall through */
4328 case XDP_DROP:
4329 do_drop:
4330 kfree_skb(skb);
4331 break;
4332 }
4333
4334 return act;
4335 }
4336
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26257 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next v4 3/4] net/tc: introduce TC_ACT_REINSERT.
2018-07-26 14:34 ` [PATCH net-next v4 3/4] net/tc: introduce TC_ACT_REINSERT Paolo Abeni
2018-07-26 23:29 ` Cong Wang
2018-07-29 3:21 ` kbuild test robot
@ 2018-07-29 3:34 ` kbuild test robot
2 siblings, 0 replies; 12+ messages in thread
From: kbuild test robot @ 2018-07-29 3:34 UTC (permalink / raw)
To: Paolo Abeni
Cc: kbuild-all, netdev, Jamal Hadi Salim, Cong Wang, Jiri Pirko,
Daniel Borkmann, Marcelo Ricardo Leitner, Eyal Birger,
David S. Miller
[-- Attachment #1: Type: text/plain, Size: 5330 bytes --]
Hi Paolo,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
url: https://github.com/0day-ci/linux/commits/Paolo-Abeni/TC-refactor-act_mirred-packets-re-injection/20180729-102154
config: x86_64-randconfig-u0-07291027 (attached as .config)
compiler: gcc-5 (Debian 5.5.0-3) 5.4.1 20171010
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All warnings (new ones prefixed by >>):
In file included from arch/x86/include/asm/current.h:5:0,
from include/linux/sched.h:12,
from include/linux/uaccess.h:5,
from net/core/dev.c:75:
net/core/dev.c: In function 'netif_receive_generic_xdp':
net/core/dev.c:4255:28: error: 'struct sk_buff' has no member named 'tc_redirected'
if (skb_cloned(skb) || skb->tc_redirected)
^
include/linux/compiler.h:58:30: note: in definition of macro '__trace_if'
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^
>> net/core/dev.c:4255:2: note: in expansion of macro 'if'
if (skb_cloned(skb) || skb->tc_redirected)
^
net/core/dev.c:4255:28: error: 'struct sk_buff' has no member named 'tc_redirected'
if (skb_cloned(skb) || skb->tc_redirected)
^
include/linux/compiler.h:58:42: note: in definition of macro '__trace_if'
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^
>> net/core/dev.c:4255:2: note: in expansion of macro 'if'
if (skb_cloned(skb) || skb->tc_redirected)
^
net/core/dev.c:4255:28: error: 'struct sk_buff' has no member named 'tc_redirected'
if (skb_cloned(skb) || skb->tc_redirected)
^
include/linux/compiler.h:69:16: note: in definition of macro '__trace_if'
______r = !!(cond); \
^
>> net/core/dev.c:4255:2: note: in expansion of macro 'if'
if (skb_cloned(skb) || skb->tc_redirected)
^
vim +/if +4255 net/core/dev.c
4241
4242 static u32 netif_receive_generic_xdp(struct sk_buff *skb,
4243 struct xdp_buff *xdp,
4244 struct bpf_prog *xdp_prog)
4245 {
4246 struct netdev_rx_queue *rxqueue;
4247 void *orig_data, *orig_data_end;
4248 u32 metalen, act = XDP_DROP;
4249 int hlen, off;
4250 u32 mac_len;
4251
4252 /* Reinjected packets coming from act_mirred or similar should
4253 * not get XDP generic processing.
4254 */
> 4255 if (skb_cloned(skb) || skb->tc_redirected)
4256 return XDP_PASS;
4257
4258 /* XDP packets must be linear and must have sufficient headroom
4259 * of XDP_PACKET_HEADROOM bytes. This is the guarantee that also
4260 * native XDP provides, thus we need to do it here as well.
4261 */
4262 if (skb_is_nonlinear(skb) ||
4263 skb_headroom(skb) < XDP_PACKET_HEADROOM) {
4264 int hroom = XDP_PACKET_HEADROOM - skb_headroom(skb);
4265 int troom = skb->tail + skb->data_len - skb->end;
4266
4267 /* In case we have to go down the path and also linearize,
4268 * then lets do the pskb_expand_head() work just once here.
4269 */
4270 if (pskb_expand_head(skb,
4271 hroom > 0 ? ALIGN(hroom, NET_SKB_PAD) : 0,
4272 troom > 0 ? troom + 128 : 0, GFP_ATOMIC))
4273 goto do_drop;
4274 if (skb_linearize(skb))
4275 goto do_drop;
4276 }
4277
4278 /* The XDP program wants to see the packet starting at the MAC
4279 * header.
4280 */
4281 mac_len = skb->data - skb_mac_header(skb);
4282 hlen = skb_headlen(skb) + mac_len;
4283 xdp->data = skb->data - mac_len;
4284 xdp->data_meta = xdp->data;
4285 xdp->data_end = xdp->data + hlen;
4286 xdp->data_hard_start = skb->data - skb_headroom(skb);
4287 orig_data_end = xdp->data_end;
4288 orig_data = xdp->data;
4289
4290 rxqueue = netif_get_rxqueue(skb);
4291 xdp->rxq = &rxqueue->xdp_rxq;
4292
4293 act = bpf_prog_run_xdp(xdp_prog, xdp);
4294
4295 off = xdp->data - orig_data;
4296 if (off > 0)
4297 __skb_pull(skb, off);
4298 else if (off < 0)
4299 __skb_push(skb, -off);
4300 skb->mac_header += off;
4301
4302 /* check if bpf_xdp_adjust_tail was used. it can only "shrink"
4303 * pckt.
4304 */
4305 off = orig_data_end - xdp->data_end;
4306 if (off != 0) {
4307 skb_set_tail_pointer(skb, xdp->data_end - xdp->data);
4308 skb->len -= off;
4309
4310 }
4311
4312 switch (act) {
4313 case XDP_REDIRECT:
4314 case XDP_TX:
4315 __skb_push(skb, mac_len);
4316 break;
4317 case XDP_PASS:
4318 metalen = xdp->data - xdp->data_meta;
4319 if (metalen)
4320 skb_metadata_set(skb, metalen);
4321 break;
4322 default:
4323 bpf_warn_invalid_xdp_action(act);
4324 /* fall through */
4325 case XDP_ABORTED:
4326 trace_xdp_exception(skb->dev, xdp_prog, act);
4327 /* fall through */
4328 case XDP_DROP:
4329 do_drop:
4330 kfree_skb(skb);
4331 break;
4332 }
4333
4334 return act;
4335 }
4336
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 34296 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-07-29 6:05 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-26 14:34 [PATCH net-next v4 0/4] TC: refactor act_mirred packets re-injection Paolo Abeni
2018-07-26 14:34 ` [PATCH net-next v4 1/4] net/sched: user-space can't set unknown tcfa_action values Paolo Abeni
2018-07-27 0:28 ` Marcelo Ricardo Leitner
2018-07-27 13:08 ` Paolo Abeni
2018-07-26 14:34 ` [PATCH net-next v4 2/4] tc/act: remove unneeded RCU lock in action callback Paolo Abeni
2018-07-26 14:34 ` [PATCH net-next v4 3/4] net/tc: introduce TC_ACT_REINSERT Paolo Abeni
2018-07-26 23:29 ` Cong Wang
2018-07-29 3:21 ` kbuild test robot
2018-07-29 3:34 ` kbuild test robot
2018-07-26 14:35 ` [PATCH net-next v4 4/4] act_mirred: use TC_ACT_REINSERT when possible Paolo Abeni
2018-07-26 23:27 ` Cong Wang
2018-07-29 2:58 ` kbuild test robot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.