* [PATCH v10 net-next 0/3] net/sched: fix over mtu packet of defrag in
@ 2020-11-12 3:24 wenxu
2020-11-12 3:24 ` [PATCH v10 net-next 1/3] net/sched: fix miss init the mru in qdisc_skb_cb wenxu
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: wenxu @ 2020-11-12 3:24 UTC (permalink / raw)
To: kuba, marcelo.leitner, vladbu; +Cc: netdev
From: wenxu <wenxu@ucloud.cn>
Currently kernel tc subsystem can do conntrack in act_ct. But when several
fragment packets go through the act_ct, function tcf_ct_handle_fragments
will defrag the packets to a big one. But the last action will redirect
mirred to a device which maybe lead the reassembly big packet over the mtu
of target device.
The first patch fix miss init the qdisc_skb_cb->mru
The send one refactor the hanle of xmit in act_mirred and prepare for the
third one
The last one add implict packet fragment support to fix the over mtu for
defrag in act_ct.
wenxu (3):
net/sched: fix miss init the mru in qdisc_skb_cb
net/sched: act_mirred: refactor the handle of xmit
net/sched: act_frag: add implict packet fragment support.
include/net/act_api.h | 18 +++++
include/net/sch_generic.h | 5 --
net/core/dev.c | 2 +
net/sched/Kconfig | 13 ++++
net/sched/Makefile | 1 +
net/sched/act_api.c | 44 +++++++++++++
net/sched/act_ct.c | 7 ++
net/sched/act_frag.c | 164 ++++++++++++++++++++++++++++++++++++++++++++++
net/sched/act_mirred.c | 21 ++++--
9 files changed, 264 insertions(+), 11 deletions(-)
create mode 100644 net/sched/act_frag.c
--
1.8.3.1
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v10 net-next 1/3] net/sched: fix miss init the mru in qdisc_skb_cb
2020-11-12 3:24 [PATCH v10 net-next 0/3] net/sched: fix over mtu packet of defrag in wenxu
@ 2020-11-12 3:24 ` wenxu
2020-11-12 3:24 ` [PATCH v10 net-next 2/3] net/sched: act_mirred: refactor the handle of xmit wenxu
2020-11-12 3:24 ` [PATCH v10 net-next 3/3] net/sched: act_frag: add implict packet fragment support wenxu
2 siblings, 0 replies; 17+ messages in thread
From: wenxu @ 2020-11-12 3:24 UTC (permalink / raw)
To: kuba, marcelo.leitner, vladbu; +Cc: netdev
From: wenxu <wenxu@ucloud.cn>
The mru in the qdisc_skb_cb should be init as 0. Only defrag packets in the
act_ct will set the value.
Fixes: 038ebb1a713d ("net/sched: act_ct: fix miss set mru for ovs after defrag in act_ct")
Signed-off-by: wenxu <wenxu@ucloud.cn>
---
v10: no change
net/core/dev.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/core/dev.c b/net/core/dev.c
index 751e526..a40de66 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3865,6 +3865,7 @@ int dev_loopback_xmit(struct net *net, struct sock *sk, struct sk_buff *skb)
return skb;
/* qdisc_skb_cb(skb)->pkt_len was already set by the caller. */
+ qdisc_skb_cb(skb)->mru = 0;
mini_qdisc_bstats_cpu_update(miniq, skb);
switch (tcf_classify(skb, miniq->filter_list, &cl_res, false)) {
@@ -4950,6 +4951,7 @@ static __latent_entropy void net_tx_action(struct softirq_action *h)
}
qdisc_skb_cb(skb)->pkt_len = skb->len;
+ qdisc_skb_cb(skb)->mru = 0;
skb->tc_at_ingress = 1;
mini_qdisc_bstats_cpu_update(miniq, skb);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v10 net-next 2/3] net/sched: act_mirred: refactor the handle of xmit
2020-11-12 3:24 [PATCH v10 net-next 0/3] net/sched: fix over mtu packet of defrag in wenxu
2020-11-12 3:24 ` [PATCH v10 net-next 1/3] net/sched: fix miss init the mru in qdisc_skb_cb wenxu
@ 2020-11-12 3:24 ` wenxu
2020-11-12 3:24 ` [PATCH v10 net-next 3/3] net/sched: act_frag: add implict packet fragment support wenxu
2 siblings, 0 replies; 17+ messages in thread
From: wenxu @ 2020-11-12 3:24 UTC (permalink / raw)
To: kuba, marcelo.leitner, vladbu; +Cc: netdev
From: wenxu <wenxu@ucloud.cn>
This one is prepare for the next patch.
Signed-off-by: wenxu <wenxu@ucloud.cn>
---
v10: no change
include/net/sch_generic.h | 5 -----
net/sched/act_mirred.c | 21 +++++++++++++++------
2 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index d8fd867..dd74f06 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -1281,9 +1281,4 @@ void mini_qdisc_pair_init(struct mini_Qdisc_pair *miniqp, struct Qdisc *qdisc,
void mini_qdisc_pair_block_init(struct mini_Qdisc_pair *miniqp,
struct tcf_block *block);
-static inline int skb_tc_reinsert(struct sk_buff *skb, struct tcf_result *res)
-{
- return res->ingress ? netif_receive_skb(skb) : dev_queue_xmit(skb);
-}
-
#endif
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index e24b7e2..17d0095 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -205,6 +205,18 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
return err;
}
+static int tcf_mirred_forward(bool want_ingress, struct sk_buff *skb)
+{
+ int err;
+
+ if (!want_ingress)
+ err = dev_queue_xmit(skb);
+ else
+ err = netif_receive_skb(skb);
+
+ return err;
+}
+
static int tcf_mirred_act(struct sk_buff *skb, const struct tc_action *a,
struct tcf_result *res)
{
@@ -287,18 +299,15 @@ static int tcf_mirred_act(struct sk_buff *skb, const struct tc_action *a,
/* let's the caller reinsert the packet, if possible */
if (use_reinsert) {
res->ingress = want_ingress;
- if (skb_tc_reinsert(skb, res))
+ err = tcf_mirred_forward(res->ingress, skb);
+ if (err)
tcf_action_inc_overlimit_qstats(&m->common);
__this_cpu_dec(mirred_rec_level);
return TC_ACT_CONSUMED;
}
}
- if (!want_ingress)
- err = dev_queue_xmit(skb2);
- else
- err = netif_receive_skb(skb2);
-
+ err = tcf_mirred_forward(want_ingress, skb2);
if (err) {
out:
tcf_action_inc_overlimit_qstats(&m->common);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v10 net-next 3/3] net/sched: act_frag: add implict packet fragment support.
2020-11-12 3:24 [PATCH v10 net-next 0/3] net/sched: fix over mtu packet of defrag in wenxu
2020-11-12 3:24 ` [PATCH v10 net-next 1/3] net/sched: fix miss init the mru in qdisc_skb_cb wenxu
2020-11-12 3:24 ` [PATCH v10 net-next 2/3] net/sched: act_mirred: refactor the handle of xmit wenxu
@ 2020-11-12 3:24 ` wenxu
2020-11-12 22:20 ` Jakub Kicinski
2020-11-14 18:05 ` Cong Wang
2 siblings, 2 replies; 17+ messages in thread
From: wenxu @ 2020-11-12 3:24 UTC (permalink / raw)
To: kuba, marcelo.leitner, vladbu; +Cc: netdev
From: wenxu <wenxu@ucloud.cn>
Currently kernel tc subsystem can do conntrack in cat_ct. But when several
fragment packets go through the act_ct, function tcf_ct_handle_fragments
will defrag the packets to a big one. But the last action will redirect
mirred to a device which maybe lead the reassembly big packet over the mtu
of target device.
This patch add support for a xmit hook to mirred, that gets executed before
xmiting the packet. Then, when act_ct gets loaded, it configs that hook.
The frag xmit hook maybe reused by other modules.
Signed-off-by: wenxu <wenxu@ucloud.cn>
---
v2: Fix the crash for act_frag module without load
v3: modify the kconfig describe and put tcf_xmit_hook_is_enabled
in the tcf_dev_queue_xmit, and xchg atomic for tcf_xmit_hook
v4: using skb_protocol and fix line length exceeds 80 columns
v5: no change
v6: protect the tcf_xmit_hook with rcu lock
v7-v10: fix __rcu warning
include/net/act_api.h | 18 ++++++
net/sched/Kconfig | 13 ++++
net/sched/Makefile | 1 +
net/sched/act_api.c | 44 +++++++++++++
net/sched/act_ct.c | 7 +++
net/sched/act_frag.c | 164 +++++++++++++++++++++++++++++++++++++++++++++++++
net/sched/act_mirred.c | 2 +-
7 files changed, 248 insertions(+), 1 deletion(-)
create mode 100644 net/sched/act_frag.c
diff --git a/include/net/act_api.h b/include/net/act_api.h
index 8721492..87ea1df 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -239,6 +239,24 @@ int tcf_action_check_ctrlact(int action, struct tcf_proto *tp,
struct netlink_ext_ack *newchain);
struct tcf_chain *tcf_action_set_ctrlact(struct tc_action *a, int action,
struct tcf_chain *newchain);
+
+typedef int xmit_hook_func(struct sk_buff *skb,
+ int (*xmit)(struct sk_buff *skb));
+
+int tcf_dev_queue_xmit(struct sk_buff *skb, int (*xmit)(struct sk_buff *skb));
+int tcf_set_xmit_hook(xmit_hook_func *xmit_hook);
+void tcf_clear_xmit_hook(void);
+
+#if IS_ENABLED(CONFIG_NET_ACT_FRAG)
+int tcf_frag_xmit_hook(struct sk_buff *skb, int (*xmit)(struct sk_buff *skb));
+#else
+static inline int tcf_frag_xmit_hook(struct sk_buff *skb,
+ int (*xmit)(struct sk_buff *skb))
+{
+ return 0;
+}
+#endif
+
#endif /* CONFIG_NET_CLS_ACT */
static inline void tcf_action_stats_update(struct tc_action *a, u64 bytes,
diff --git a/net/sched/Kconfig b/net/sched/Kconfig
index a3b37d8..9a240c7 100644
--- a/net/sched/Kconfig
+++ b/net/sched/Kconfig
@@ -974,9 +974,22 @@ config NET_ACT_TUNNEL_KEY
To compile this code as a module, choose M here: the
module will be called act_tunnel_key.
+config NET_ACT_FRAG
+ tristate "Packet fragmentation"
+ depends on NET_CLS_ACT
+ help
+ Say Y here to allow fragmenting big packets when outputting
+ with the mirred action.
+
+ If unsure, say N.
+
+ To compile this code as a module, choose M here: the
+ module will be called act_frag.
+
config NET_ACT_CT
tristate "connection tracking tc action"
depends on NET_CLS_ACT && NF_CONNTRACK && NF_NAT && NF_FLOW_TABLE
+ depends on NET_ACT_FRAG
help
Say Y here to allow sending the packets to conntrack module.
diff --git a/net/sched/Makefile b/net/sched/Makefile
index 66bbf9a..c146186 100644
--- a/net/sched/Makefile
+++ b/net/sched/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_NET_IFE_SKBMARK) += act_meta_mark.o
obj-$(CONFIG_NET_IFE_SKBPRIO) += act_meta_skbprio.o
obj-$(CONFIG_NET_IFE_SKBTCINDEX) += act_meta_skbtcindex.o
obj-$(CONFIG_NET_ACT_TUNNEL_KEY)+= act_tunnel_key.o
+obj-$(CONFIG_NET_ACT_FRAG) += act_frag.o
obj-$(CONFIG_NET_ACT_CT) += act_ct.o
obj-$(CONFIG_NET_ACT_GATE) += act_gate.o
obj-$(CONFIG_NET_SCH_FIFO) += sch_fifo.o
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index f66417d..93868b7 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -22,6 +22,50 @@
#include <net/act_api.h>
#include <net/netlink.h>
+static xmit_hook_func __rcu *tcf_xmit_hook;
+static DEFINE_SPINLOCK(tcf_xmit_hook_lock);
+static u16 tcf_xmit_hook_count;
+
+int tcf_set_xmit_hook(xmit_hook_func *xmit_hook)
+{
+ spin_lock(&tcf_xmit_hook_lock);
+ if (!tcf_xmit_hook_count) {
+ rcu_assign_pointer(tcf_xmit_hook, xmit_hook);
+ } else if (xmit_hook != tcf_xmit_hook) {
+ spin_unlock(&tcf_xmit_hook_lock);
+ return -EBUSY;
+ }
+
+ tcf_xmit_hook_count++;
+ spin_unlock(&tcf_xmit_hook_lock);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(tcf_set_xmit_hook);
+
+void tcf_clear_xmit_hook(void)
+{
+ spin_lock(&tcf_xmit_hook_lock);
+ if (--tcf_xmit_hook_count == 0)
+ rcu_assign_pointer(tcf_xmit_hook, NULL);
+ spin_unlock(&tcf_xmit_hook_lock);
+
+ synchronize_rcu();
+}
+EXPORT_SYMBOL_GPL(tcf_clear_xmit_hook);
+
+int tcf_dev_queue_xmit(struct sk_buff *skb, int (*xmit)(struct sk_buff *skb))
+{
+ xmit_hook_func *xmit_hook;
+
+ xmit_hook = rcu_dereference(tcf_xmit_hook);
+ if (xmit_hook)
+ return xmit_hook(skb, xmit);
+ else
+ return xmit(skb);
+}
+EXPORT_SYMBOL_GPL(tcf_dev_queue_xmit);
+
static void tcf_action_goto_chain_exec(const struct tc_action *a,
struct tcf_result *res)
{
diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
index 9c79fb9..dff3c40 100644
--- a/net/sched/act_ct.c
+++ b/net/sched/act_ct.c
@@ -1541,8 +1541,14 @@ static int __init ct_init_module(void)
if (err)
goto err_register;
+ err = tcf_set_xmit_hook(tcf_frag_xmit_hook);
+ if (err)
+ goto err_action;
+
return 0;
+err_action:
+ tcf_unregister_action(&act_ct_ops, &ct_net_ops);
err_register:
tcf_ct_flow_tables_uninit();
err_tbl_init:
@@ -1552,6 +1558,7 @@ static int __init ct_init_module(void)
static void __exit ct_cleanup_module(void)
{
+ tcf_clear_xmit_hook();
tcf_unregister_action(&act_ct_ops, &ct_net_ops);
tcf_ct_flow_tables_uninit();
destroy_workqueue(act_ct_wq);
diff --git a/net/sched/act_frag.c b/net/sched/act_frag.c
new file mode 100644
index 0000000..3a7ab92
--- /dev/null
+++ b/net/sched/act_frag.c
@@ -0,0 +1,164 @@
+// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
+#include <net/netlink.h>
+#include <net/act_api.h>
+#include <net/dst.h>
+#include <net/ip.h>
+#include <net/ip6_fib.h>
+
+struct tcf_frag_data {
+ unsigned long dst;
+ struct qdisc_skb_cb cb;
+ __be16 inner_protocol;
+ u16 vlan_tci;
+ __be16 vlan_proto;
+ unsigned int l2_len;
+ u8 l2_data[VLAN_ETH_HLEN];
+ int (*xmit)(struct sk_buff *skb);
+};
+
+static DEFINE_PER_CPU(struct tcf_frag_data, tcf_frag_data_storage);
+
+static int tcf_frag_xmit(struct net *net, struct sock *sk, struct sk_buff *skb)
+{
+ struct tcf_frag_data *data = this_cpu_ptr(&tcf_frag_data_storage);
+
+ if (skb_cow_head(skb, data->l2_len) < 0) {
+ kfree_skb(skb);
+ return -ENOMEM;
+ }
+
+ __skb_dst_copy(skb, data->dst);
+ *qdisc_skb_cb(skb) = data->cb;
+ skb->inner_protocol = data->inner_protocol;
+ if (data->vlan_tci & VLAN_CFI_MASK)
+ __vlan_hwaccel_put_tag(skb, data->vlan_proto,
+ data->vlan_tci & ~VLAN_CFI_MASK);
+ else
+ __vlan_hwaccel_clear_tag(skb);
+
+ /* Reconstruct the MAC header. */
+ skb_push(skb, data->l2_len);
+ memcpy(skb->data, &data->l2_data, data->l2_len);
+ skb_postpush_rcsum(skb, skb->data, data->l2_len);
+ skb_reset_mac_header(skb);
+
+ data->xmit(skb);
+
+ return 0;
+}
+
+static void tcf_frag_prepare_frag(struct sk_buff *skb,
+ int (*xmit)(struct sk_buff *skb))
+{
+ unsigned int hlen = skb_network_offset(skb);
+ struct tcf_frag_data *data;
+
+ data = this_cpu_ptr(&tcf_frag_data_storage);
+ data->dst = skb->_skb_refdst;
+ data->cb = *qdisc_skb_cb(skb);
+ data->xmit = xmit;
+ data->inner_protocol = skb->inner_protocol;
+ if (skb_vlan_tag_present(skb))
+ data->vlan_tci = skb_vlan_tag_get(skb) | VLAN_CFI_MASK;
+ else
+ data->vlan_tci = 0;
+ data->vlan_proto = skb->vlan_proto;
+ data->l2_len = hlen;
+ memcpy(&data->l2_data, skb->data, hlen);
+
+ memset(IPCB(skb), 0, sizeof(struct inet_skb_parm));
+ skb_pull(skb, hlen);
+}
+
+static unsigned int
+tcf_frag_dst_get_mtu(const struct dst_entry *dst)
+{
+ return dst->dev->mtu;
+}
+
+static struct dst_ops tcf_frag_dst_ops = {
+ .family = AF_UNSPEC,
+ .mtu = tcf_frag_dst_get_mtu,
+};
+
+static int tcf_fragment(struct net *net, struct sk_buff *skb,
+ u16 mru, int (*xmit)(struct sk_buff *skb))
+{
+ if (skb_network_offset(skb) > VLAN_ETH_HLEN) {
+ net_warn_ratelimited("L2 header too long to fragment\n");
+ goto err;
+ }
+
+ if (skb_protocol(skb, true) == htons(ETH_P_IP)) {
+ struct dst_entry tcf_frag_dst;
+ unsigned long orig_dst;
+
+ tcf_frag_prepare_frag(skb, xmit);
+ dst_init(&tcf_frag_dst, &tcf_frag_dst_ops, NULL, 1,
+ DST_OBSOLETE_NONE, DST_NOCOUNT);
+ tcf_frag_dst.dev = skb->dev;
+
+ orig_dst = skb->_skb_refdst;
+ skb_dst_set_noref(skb, &tcf_frag_dst);
+ IPCB(skb)->frag_max_size = mru;
+
+ ip_do_fragment(net, skb->sk, skb, tcf_frag_xmit);
+ refdst_drop(orig_dst);
+ } else if (skb_protocol(skb, true) == htons(ETH_P_IPV6)) {
+ unsigned long orig_dst;
+ struct rt6_info tcf_frag_rt;
+
+ tcf_frag_prepare_frag(skb, xmit);
+ memset(&tcf_frag_rt, 0, sizeof(tcf_frag_rt));
+ dst_init(&tcf_frag_rt.dst, &tcf_frag_dst_ops, NULL, 1,
+ DST_OBSOLETE_NONE, DST_NOCOUNT);
+ tcf_frag_rt.dst.dev = skb->dev;
+
+ orig_dst = skb->_skb_refdst;
+ skb_dst_set_noref(skb, &tcf_frag_rt.dst);
+ IP6CB(skb)->frag_max_size = mru;
+
+ ipv6_stub->ipv6_fragment(net, skb->sk, skb, tcf_frag_xmit);
+ refdst_drop(orig_dst);
+ } else {
+ net_warn_ratelimited("Fail frag %s: eth=%x, MRU=%d, MTU=%d\n",
+ netdev_name(skb->dev),
+ ntohs(skb_protocol(skb, true)), mru,
+ skb->dev->mtu);
+ goto err;
+ }
+
+ qdisc_skb_cb(skb)->mru = 0;
+ return 0;
+err:
+ kfree_skb(skb);
+ return -1;
+}
+
+int tcf_frag_xmit_hook(struct sk_buff *skb, int (*xmit)(struct sk_buff *skb))
+{
+ u16 mru = qdisc_skb_cb(skb)->mru;
+ int err;
+
+ if (mru && skb->len > mru + skb->dev->hard_header_len)
+ err = tcf_fragment(dev_net(skb->dev), skb, mru, xmit);
+ else
+ err = xmit(skb);
+
+ return err;
+}
+EXPORT_SYMBOL_GPL(tcf_frag_xmit_hook);
+
+static int __init frag_init_module(void)
+{
+ return 0;
+}
+
+static void __exit frag_cleanup_module(void)
+{
+}
+
+module_init(frag_init_module);
+module_exit(frag_cleanup_module);
+MODULE_AUTHOR("wenxu <wenxu@ucloud.cn>");
+MODULE_LICENSE("GPL v2");
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 17d0095..7153c67 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -210,7 +210,7 @@ static int tcf_mirred_forward(bool want_ingress, struct sk_buff *skb)
int err;
if (!want_ingress)
- err = dev_queue_xmit(skb);
+ err = tcf_dev_queue_xmit(skb, dev_queue_xmit);
else
err = netif_receive_skb(skb);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v10 net-next 3/3] net/sched: act_frag: add implict packet fragment support.
2020-11-12 3:24 ` [PATCH v10 net-next 3/3] net/sched: act_frag: add implict packet fragment support wenxu
@ 2020-11-12 22:20 ` Jakub Kicinski
2020-11-13 2:25 ` Marcelo Ricardo Leitner
2020-11-14 18:05 ` Cong Wang
1 sibling, 1 reply; 17+ messages in thread
From: Jakub Kicinski @ 2020-11-12 22:20 UTC (permalink / raw)
To: wenxu; +Cc: marcelo.leitner, vladbu, netdev
On Thu, 12 Nov 2020 11:24:57 +0800 wenxu@ucloud.cn wrote:
> v7-v10: fix __rcu warning
Are you reposting stuff just to get it build tested?
This is absolutely unacceptable.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v10 net-next 3/3] net/sched: act_frag: add implict packet fragment support.
2020-11-12 22:20 ` Jakub Kicinski
@ 2020-11-13 2:25 ` Marcelo Ricardo Leitner
2020-11-13 17:04 ` Jakub Kicinski
0 siblings, 1 reply; 17+ messages in thread
From: Marcelo Ricardo Leitner @ 2020-11-13 2:25 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: wenxu, vladbu, netdev
On Thu, Nov 12, 2020 at 02:20:58PM -0800, Jakub Kicinski wrote:
> On Thu, 12 Nov 2020 11:24:57 +0800 wenxu@ucloud.cn wrote:
> > v7-v10: fix __rcu warning
>
> Are you reposting stuff just to get it build tested?
>
> This is absolutely unacceptable.
I don't know if that's the case, but maybe we could have a shadow
mailing list just for that? So that bots would monitor and would run
(almost) the same tests are they do here. Then when patches are posted
here, a list that people actually subscribe, they are already more
ready. The bots would have to email an "ok" as well, but that's
implementation detail already. Not that developers shouldn't test
before posting, but the bots are already doing some tests that may be
beyond of what one can think of testing before posting.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v10 net-next 3/3] net/sched: act_frag: add implict packet fragment support.
2020-11-13 2:25 ` Marcelo Ricardo Leitner
@ 2020-11-13 17:04 ` Jakub Kicinski
0 siblings, 0 replies; 17+ messages in thread
From: Jakub Kicinski @ 2020-11-13 17:04 UTC (permalink / raw)
To: Marcelo Ricardo Leitner; +Cc: wenxu, vladbu, netdev
On Thu, 12 Nov 2020 23:25:22 -0300 Marcelo Ricardo Leitner wrote:
> On Thu, Nov 12, 2020 at 02:20:58PM -0800, Jakub Kicinski wrote:
> > On Thu, 12 Nov 2020 11:24:57 +0800 wenxu@ucloud.cn wrote:
> > > v7-v10: fix __rcu warning
> >
> > Are you reposting stuff just to get it build tested?
> >
> > This is absolutely unacceptable.
>
> I don't know if that's the case, but maybe we could have a shadow
> mailing list just for that? So that bots would monitor and would run
> (almost) the same tests are they do here. Then when patches are posted
> here, a list that people actually subscribe, they are already more
> ready. The bots would have to email an "ok" as well, but that's
> implementation detail already. Not that developers shouldn't test
> before posting, but the bots are already doing some tests that may be
> beyond of what one can think of testing before posting.
The code for the entire system is right here:
https://github.com/kuba-moo/nipa
It depends on a patchwork instance to report results to.
I have a script there to feed patches in locally from a maildir but
haven't tested that in a while so it's probably broken. You can also
just run the build bash script without running the whole bot:
https://github.com/kuba-moo/nipa/blob/master/tests/patch/build_allmodconfig_warn/build_allmodconfig.sh
Hardly rocket science.
I have no preference on what people do to test their code, and I'm
happy to take patches for the bot, too.
But we can't have people posting 11 versions of patches to netdev which
is already too high traffic for people to follow.
Not to mention that someone needs to pay for the CPU cycles of the bot,
and we don't want to block getting results for legitimate patches.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v10 net-next 3/3] net/sched: act_frag: add implict packet fragment support.
2020-11-12 3:24 ` [PATCH v10 net-next 3/3] net/sched: act_frag: add implict packet fragment support wenxu
2020-11-12 22:20 ` Jakub Kicinski
@ 2020-11-14 18:05 ` Cong Wang
2020-11-14 22:46 ` Marcelo Ricardo Leitner
2020-11-15 13:05 ` wenxu
1 sibling, 2 replies; 17+ messages in thread
From: Cong Wang @ 2020-11-14 18:05 UTC (permalink / raw)
To: wenxu
Cc: Jakub Kicinski, Marcelo Ricardo Leitner, Vlad Buslov,
Linux Kernel Network Developers
On Wed, Nov 11, 2020 at 9:44 PM <wenxu@ucloud.cn> wrote:
> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> index 9c79fb9..dff3c40 100644
> --- a/net/sched/act_ct.c
> +++ b/net/sched/act_ct.c
> @@ -1541,8 +1541,14 @@ static int __init ct_init_module(void)
> if (err)
> goto err_register;
>
> + err = tcf_set_xmit_hook(tcf_frag_xmit_hook);
Yeah, this approach is certainly much better than extending act_mirred.
Just one comment below.
> diff --git a/net/sched/act_frag.c b/net/sched/act_frag.c
> new file mode 100644
> index 0000000..3a7ab92
> --- /dev/null
> +++ b/net/sched/act_frag.c
It is kinda confusing to see this is a module. It provides some
wrappers and hooks the dev_xmit_queue(), it belongs more to
the core tc code than any modularized code. How about putting
this into net/sched/sch_generic.c?
Thanks.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v10 net-next 3/3] net/sched: act_frag: add implict packet fragment support.
2020-11-14 18:05 ` Cong Wang
@ 2020-11-14 22:46 ` Marcelo Ricardo Leitner
2020-11-16 18:57 ` Cong Wang
2020-11-15 13:05 ` wenxu
1 sibling, 1 reply; 17+ messages in thread
From: Marcelo Ricardo Leitner @ 2020-11-14 22:46 UTC (permalink / raw)
To: Cong Wang
Cc: wenxu, Jakub Kicinski, Vlad Buslov, Linux Kernel Network Developers
On Sat, Nov 14, 2020 at 10:05:39AM -0800, Cong Wang wrote:
> On Wed, Nov 11, 2020 at 9:44 PM <wenxu@ucloud.cn> wrote:
> > diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> > index 9c79fb9..dff3c40 100644
> > --- a/net/sched/act_ct.c
> > +++ b/net/sched/act_ct.c
> > @@ -1541,8 +1541,14 @@ static int __init ct_init_module(void)
> > if (err)
> > goto err_register;
> >
> > + err = tcf_set_xmit_hook(tcf_frag_xmit_hook);
>
> Yeah, this approach is certainly much better than extending act_mirred.
> Just one comment below.
Nice. :-)
>
>
> > diff --git a/net/sched/act_frag.c b/net/sched/act_frag.c
> > new file mode 100644
> > index 0000000..3a7ab92
> > --- /dev/null
> > +++ b/net/sched/act_frag.c
>
> It is kinda confusing to see this is a module. It provides some
> wrappers and hooks the dev_xmit_queue(), it belongs more to
> the core tc code than any modularized code. How about putting
> this into net/sched/sch_generic.c?
Davide had shared similar concerns with regards of the new module too.
The main idea behind the new module was to keep it as
isolated/contained as possible, and only so. So thumbs up from my
side.
To be clear, you're only talking about the module itself, right? It
would still need to have the Kconfig to enable this feature, or not?
Thanks,
Marcelo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v10 net-next 3/3] net/sched: act_frag: add implict packet fragment support.
2020-11-14 18:05 ` Cong Wang
2020-11-14 22:46 ` Marcelo Ricardo Leitner
@ 2020-11-15 13:05 ` wenxu
2020-11-15 16:26 ` Jamal Hadi Salim
2020-11-16 19:01 ` Cong Wang
1 sibling, 2 replies; 17+ messages in thread
From: wenxu @ 2020-11-15 13:05 UTC (permalink / raw)
To: Cong Wang
Cc: Jakub Kicinski, Marcelo Ricardo Leitner, Vlad Buslov,
Linux Kernel Network Developers
在 2020/11/15 2:05, Cong Wang 写道:
> On Wed, Nov 11, 2020 at 9:44 PM <wenxu@ucloud.cn> wrote:
>> diff --git a/net/sched/act_frag.c b/net/sched/act_frag.c
>> new file mode 100644
>> index 0000000..3a7ab92
>> --- /dev/null
>> +++ b/net/sched/act_frag.c
> It is kinda confusing to see this is a module. It provides some
> wrappers and hooks the dev_xmit_queue(), it belongs more to
> the core tc code than any modularized code. How about putting
> this into net/sched/sch_generic.c?
>
> Thanks.
All the operations in the act_frag are single L3 action.
So we put in a single module. to keep it as isolated/contained as possible
Maybe put this in a single file is better than a module? Buildin in the tc core code or not.
Enable this feature in Kconifg with NET_ACT_FRAG?
+config NET_ACT_FRAG
+ bool "Packet fragmentation"
+ depends on NET_CLS_ACT
+ help
+ Say Y here to allow fragmenting big packets when outputting
+ with the mirred action.
+
+ If unsure, say N.
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v10 net-next 3/3] net/sched: act_frag: add implict packet fragment support.
2020-11-15 13:05 ` wenxu
@ 2020-11-15 16:26 ` Jamal Hadi Salim
2020-11-16 1:09 ` wenxu
2020-11-16 19:01 ` Cong Wang
1 sibling, 1 reply; 17+ messages in thread
From: Jamal Hadi Salim @ 2020-11-15 16:26 UTC (permalink / raw)
To: wenxu, Cong Wang
Cc: Jakub Kicinski, Marcelo Ricardo Leitner, Vlad Buslov,
Linux Kernel Network Developers
This nagged me:
What happens if all the frags dont make it out?
Should you at least return an error code(from tcf_fragment?)
and get the action err counters incremented?
cheers,
jamal
On 2020-11-15 8:05 a.m., wenxu wrote:
>
> 在 2020/11/15 2:05, Cong Wang 写道:
>> On Wed, Nov 11, 2020 at 9:44 PM <wenxu@ucloud.cn> wrote:
>>> diff --git a/net/sched/act_frag.c b/net/sched/act_frag.c
>>> new file mode 100644
>>> index 0000000..3a7ab92
>>> --- /dev/null
>>> +++ b/net/sched/act_frag.c
>> It is kinda confusing to see this is a module. It provides some
>> wrappers and hooks the dev_xmit_queue(), it belongs more to
>> the core tc code than any modularized code. How about putting
>> this into net/sched/sch_generic.c?
>>
>> Thanks.
>
> All the operations in the act_frag are single L3 action.
>
> So we put in a single module. to keep it as isolated/contained as possible
>
> Maybe put this in a single file is better than a module? Buildin in the tc core code or not.
>
> Enable this feature in Kconifg with NET_ACT_FRAG?
>
> +config NET_ACT_FRAG
> + bool "Packet fragmentation"
> + depends on NET_CLS_ACT
> + help
> + Say Y here to allow fragmenting big packets when outputting
> + with the mirred action.
> +
> + If unsure, say N.
>
>
>>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v10 net-next 3/3] net/sched: act_frag: add implict packet fragment support.
2020-11-15 16:26 ` Jamal Hadi Salim
@ 2020-11-16 1:09 ` wenxu
0 siblings, 0 replies; 17+ messages in thread
From: wenxu @ 2020-11-16 1:09 UTC (permalink / raw)
To: Jamal Hadi Salim, Cong Wang
Cc: Jakub Kicinski, Marcelo Ricardo Leitner, Vlad Buslov,
Linux Kernel Network Developers
On 11/16/2020 12:26 AM, Jamal Hadi Salim wrote:
> This nagged me:
> What happens if all the frags dont make it out?
> Should you at least return an error code(from tcf_fragment?)
> and get the action err counters incremented?
Thanks, Will do.
>
> cheers,
> jamal
>
> On 2020-11-15 8:05 a.m., wenxu wrote:
>>
>> 在 2020/11/15 2:05, Cong Wang 写道:
>>> On Wed, Nov 11, 2020 at 9:44 PM <wenxu@ucloud.cn> wrote:
>>>> diff --git a/net/sched/act_frag.c b/net/sched/act_frag.c
>>>> new file mode 100644
>>>> index 0000000..3a7ab92
>>>> --- /dev/null
>>>> +++ b/net/sched/act_frag.c
>>> It is kinda confusing to see this is a module. It provides some
>>> wrappers and hooks the dev_xmit_queue(), it belongs more to
>>> the core tc code than any modularized code. How about putting
>>> this into net/sched/sch_generic.c?
>>>
>>> Thanks.
>>
>> All the operations in the act_frag are single L3 action.
>>
>> So we put in a single module. to keep it as isolated/contained as possible
>>
>> Maybe put this in a single file is better than a module? Buildin in the tc core code or not.
>>
>> Enable this feature in Kconifg with NET_ACT_FRAG?
>>
>> +config NET_ACT_FRAG
>> + bool "Packet fragmentation"
>> + depends on NET_CLS_ACT
>> + help
>> + Say Y here to allow fragmenting big packets when outputting
>> + with the mirred action.
>> +
>> + If unsure, say N.
>>
>>
>>>
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v10 net-next 3/3] net/sched: act_frag: add implict packet fragment support.
2020-11-14 22:46 ` Marcelo Ricardo Leitner
@ 2020-11-16 18:57 ` Cong Wang
0 siblings, 0 replies; 17+ messages in thread
From: Cong Wang @ 2020-11-16 18:57 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: wenxu, Jakub Kicinski, Vlad Buslov, Linux Kernel Network Developers
On Sat, Nov 14, 2020 at 2:46 PM Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
> Davide had shared similar concerns with regards of the new module too.
> The main idea behind the new module was to keep it as
> isolated/contained as possible, and only so. So thumbs up from my
> side.
>
> To be clear, you're only talking about the module itself, right? It
> would still need to have the Kconfig to enable this feature, or not?
Both. The code itself doesn't look like a module, and it doesn't
look like an optional feature for act_ct either, does it? If not, there is
no need to have a user visible Kconfig, we just select it, or no Kconfig
at all.
Thanks.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v10 net-next 3/3] net/sched: act_frag: add implict packet fragment support.
2020-11-15 13:05 ` wenxu
2020-11-15 16:26 ` Jamal Hadi Salim
@ 2020-11-16 19:01 ` Cong Wang
2020-11-17 4:01 ` wenxu
1 sibling, 1 reply; 17+ messages in thread
From: Cong Wang @ 2020-11-16 19:01 UTC (permalink / raw)
To: wenxu
Cc: Jakub Kicinski, Marcelo Ricardo Leitner, Vlad Buslov,
Linux Kernel Network Developers
On Sun, Nov 15, 2020 at 5:06 AM wenxu <wenxu@ucloud.cn> wrote:
>
>
> 在 2020/11/15 2:05, Cong Wang 写道:
> > On Wed, Nov 11, 2020 at 9:44 PM <wenxu@ucloud.cn> wrote:
> >> diff --git a/net/sched/act_frag.c b/net/sched/act_frag.c
> >> new file mode 100644
> >> index 0000000..3a7ab92
> >> --- /dev/null
> >> +++ b/net/sched/act_frag.c
> > It is kinda confusing to see this is a module. It provides some
> > wrappers and hooks the dev_xmit_queue(), it belongs more to
> > the core tc code than any modularized code. How about putting
> > this into net/sched/sch_generic.c?
> >
> > Thanks.
>
> All the operations in the act_frag are single L3 action.
>
> So we put in a single module. to keep it as isolated/contained as possible
Yeah, but you hook dev_queue_xmit() which is L2.
>
> Maybe put this in a single file is better than a module? Buildin in the tc core code or not.
>
> Enable this feature in Kconifg with NET_ACT_FRAG?
Sort of... If this is not an optional feature, that is a must-have
feature for act_ct,
we should just get rid of this Kconfig.
Also, you need to depend on CONFIG_INET somewhere to use the IP
fragment, no?
Thanks.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v10 net-next 3/3] net/sched: act_frag: add implict packet fragment support.
2020-11-16 19:01 ` Cong Wang
@ 2020-11-17 4:01 ` wenxu
2020-11-17 22:43 ` Cong Wang
0 siblings, 1 reply; 17+ messages in thread
From: wenxu @ 2020-11-17 4:01 UTC (permalink / raw)
To: Cong Wang
Cc: Jakub Kicinski, Marcelo Ricardo Leitner, Vlad Buslov,
Linux Kernel Network Developers
On 11/17/2020 3:01 AM, Cong Wang wrote:
> On Sun, Nov 15, 2020 at 5:06 AM wenxu <wenxu@ucloud.cn> wrote:
>>
>> 在 2020/11/15 2:05, Cong Wang 写道:
>>> On Wed, Nov 11, 2020 at 9:44 PM <wenxu@ucloud.cn> wrote:
>>>> diff --git a/net/sched/act_frag.c b/net/sched/act_frag.c
>>>> new file mode 100644
>>>> index 0000000..3a7ab92
>>>> --- /dev/null
>>>> +++ b/net/sched/act_frag.c
>>> It is kinda confusing to see this is a module. It provides some
>>> wrappers and hooks the dev_xmit_queue(), it belongs more to
>>> the core tc code than any modularized code. How about putting
>>> this into net/sched/sch_generic.c?
>>>
>>> Thanks.
>> All the operations in the act_frag are single L3 action.
>>
>> So we put in a single module. to keep it as isolated/contained as possible
> Yeah, but you hook dev_queue_xmit() which is L2.
>
>> Maybe put this in a single file is better than a module? Buildin in the tc core code or not.
>>
>> Enable this feature in Kconifg with NET_ACT_FRAG?
> Sort of... If this is not an optional feature, that is a must-have
> feature for act_ct,
> we should just get rid of this Kconfig.
>
> Also, you need to depend on CONFIG_INET somewhere to use the IP
> fragment, no?
>
> Thanks.
Maybe the act_frag should rename to sch_frag and buildin kernel.
This fcuntion can be used for all tc subsystem. There is no need for
CONFIG_INET. The sched system depends on NET.
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v10 net-next 3/3] net/sched: act_frag: add implict packet fragment support.
2020-11-17 4:01 ` wenxu
@ 2020-11-17 22:43 ` Cong Wang
2020-11-17 23:21 ` wenxu
0 siblings, 1 reply; 17+ messages in thread
From: Cong Wang @ 2020-11-17 22:43 UTC (permalink / raw)
To: wenxu
Cc: Jakub Kicinski, Marcelo Ricardo Leitner, Vlad Buslov,
Linux Kernel Network Developers
On Mon, Nov 16, 2020 at 8:06 PM wenxu <wenxu@ucloud.cn> wrote:
>
>
> On 11/17/2020 3:01 AM, Cong Wang wrote:
> > On Sun, Nov 15, 2020 at 5:06 AM wenxu <wenxu@ucloud.cn> wrote:
> >>
> >> 在 2020/11/15 2:05, Cong Wang 写道:
> >>> On Wed, Nov 11, 2020 at 9:44 PM <wenxu@ucloud.cn> wrote:
> >>>> diff --git a/net/sched/act_frag.c b/net/sched/act_frag.c
> >>>> new file mode 100644
> >>>> index 0000000..3a7ab92
> >>>> --- /dev/null
> >>>> +++ b/net/sched/act_frag.c
> >>> It is kinda confusing to see this is a module. It provides some
> >>> wrappers and hooks the dev_xmit_queue(), it belongs more to
> >>> the core tc code than any modularized code. How about putting
> >>> this into net/sched/sch_generic.c?
> >>>
> >>> Thanks.
> >> All the operations in the act_frag are single L3 action.
> >>
> >> So we put in a single module. to keep it as isolated/contained as possible
> > Yeah, but you hook dev_queue_xmit() which is L2.
> >
> >> Maybe put this in a single file is better than a module? Buildin in the tc core code or not.
> >>
> >> Enable this feature in Kconifg with NET_ACT_FRAG?
> > Sort of... If this is not an optional feature, that is a must-have
> > feature for act_ct,
> > we should just get rid of this Kconfig.
> >
> > Also, you need to depend on CONFIG_INET somewhere to use the IP
> > fragment, no?
> >
> > Thanks.
>
> Maybe the act_frag should rename to sch_frag and buildin kernel.
sch_frag still sounds like a module. ;) This is why I proposed putting
it into sch_generic.c.
>
> This fcuntion can be used for all tc subsystem. There is no need for
>
> CONFIG_INET. The sched system depends on NET.
CONFIG_INET is different from CONFIG_NET, right?
Thanks.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v10 net-next 3/3] net/sched: act_frag: add implict packet fragment support.
2020-11-17 22:43 ` Cong Wang
@ 2020-11-17 23:21 ` wenxu
0 siblings, 0 replies; 17+ messages in thread
From: wenxu @ 2020-11-17 23:21 UTC (permalink / raw)
To: Cong Wang
Cc: Jakub Kicinski, Marcelo Ricardo Leitner, Vlad Buslov,
Linux Kernel Network Developers
在 2020/11/18 6:43, Cong Wang 写道:
> On Mon, Nov 16, 2020 at 8:06 PM wenxu <wenxu@ucloud.cn> wrote:
>>
>> On 11/17/2020 3:01 AM, Cong Wang wrote:
>>> On Sun, Nov 15, 2020 at 5:06 AM wenxu <wenxu@ucloud.cn> wrote:
>>>> 在 2020/11/15 2:05, Cong Wang 写道:
>>>>> On Wed, Nov 11, 2020 at 9:44 PM <wenxu@ucloud.cn> wrote:
>>>>>> diff --git a/net/sched/act_frag.c b/net/sched/act_frag.c
>>>>>> new file mode 100644
>>>>>> index 0000000..3a7ab92
>>>>>> --- /dev/null
>>>>>> +++ b/net/sched/act_frag.c
>>>>> It is kinda confusing to see this is a module. It provides some
>>>>> wrappers and hooks the dev_xmit_queue(), it belongs more to
>>>>> the core tc code than any modularized code. How about putting
>>>>> this into net/sched/sch_generic.c?
>>>>>
>>>>> Thanks.
>>>> All the operations in the act_frag are single L3 action.
>>>>
>>>> So we put in a single module. to keep it as isolated/contained as possible
>>> Yeah, but you hook dev_queue_xmit() which is L2.
>>>
>>>> Maybe put this in a single file is better than a module? Buildin in the tc core code or not.
>>>>
>>>> Enable this feature in Kconifg with NET_ACT_FRAG?
>>> Sort of... If this is not an optional feature, that is a must-have
>>> feature for act_ct,
>>> we should just get rid of this Kconfig.
>>>
>>> Also, you need to depend on CONFIG_INET somewhere to use the IP
>>> fragment, no?
>>>
>>> Thanks.
>> Maybe the act_frag should rename to sch_frag and buildin kernel.
> sch_frag still sounds like a module. ;) This is why I proposed putting
> it into sch_generic.c.
>
>> This fcuntion can be used for all tc subsystem. There is no need for
>>
>> CONFIG_INET. The sched system depends on NET.
> CONFIG_INET is different from CONFIG_NET, right?
you are right. ip_do_fragment depends on this!
>
> Thanks.
>
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-11-17 23:59 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-12 3:24 [PATCH v10 net-next 0/3] net/sched: fix over mtu packet of defrag in wenxu
2020-11-12 3:24 ` [PATCH v10 net-next 1/3] net/sched: fix miss init the mru in qdisc_skb_cb wenxu
2020-11-12 3:24 ` [PATCH v10 net-next 2/3] net/sched: act_mirred: refactor the handle of xmit wenxu
2020-11-12 3:24 ` [PATCH v10 net-next 3/3] net/sched: act_frag: add implict packet fragment support wenxu
2020-11-12 22:20 ` Jakub Kicinski
2020-11-13 2:25 ` Marcelo Ricardo Leitner
2020-11-13 17:04 ` Jakub Kicinski
2020-11-14 18:05 ` Cong Wang
2020-11-14 22:46 ` Marcelo Ricardo Leitner
2020-11-16 18:57 ` Cong Wang
2020-11-15 13:05 ` wenxu
2020-11-15 16:26 ` Jamal Hadi Salim
2020-11-16 1:09 ` wenxu
2020-11-16 19:01 ` Cong Wang
2020-11-17 4:01 ` wenxu
2020-11-17 22:43 ` Cong Wang
2020-11-17 23:21 ` wenxu
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.