net/sched: act_connmark: transition to percpu stats and rcu

The tc action act_connmark was using shared stats and taking the per
action lock in the datapath. Improve it by using percpu stats and rcu.

perf before:
- 13.55% tcf_connmark_act
   - 81.18% _raw_spin_lock
       80.46% native_queued_spin_lock_slowpath

perf after:
- 2.85% tcf_connmark_act

tdc results:
1..15
ok 1 2002 - Add valid connmark action with defaults
ok 2 56a5 - Add valid connmark action with control pass
ok 3 7c66 - Add valid connmark action with control drop
ok 4 a913 - Add valid connmark action with control pipe
ok 5 bdd8 - Add valid connmark action with control reclassify
ok 6 b8be - Add valid connmark action with control continue
ok 7 d8a6 - Add valid connmark action with control jump
ok 8 aae8 - Add valid connmark action with zone argument
ok 9 2f0b - Add valid connmark action with invalid zone argument
ok 10 9305 - Add connmark action with unsupported argument
ok 11 71ca - Add valid connmark action and replace it
ok 12 5f8f - Add valid connmark action with cookie
ok 13 c506 - Replace connmark with invalid goto chain control
ok 14 6571 - Delete connmark action with valid index
ok 15 3426 - Delete connmark action with invalid index

Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>

diff --git a/include/net/tc_act/tc_connmark.h b/include/net/tc_act/tc_connmark.h
index 1f4cb47..e8dd77a 100644 (file)
--- a/include/net/tc_act/tc_connmark.h
+++ b/include/net/tc_act/tc_connmark.h
@@ -4,10 +4,15 @@
 
 #include <net/act_api.h>
 
-struct tcf_connmark_info {
-       struct tc_action common;
+struct tcf_connmark_parms {
        struct net *net;
        u16 zone;
+       struct rcu_head rcu;
+};
+
+struct tcf_connmark_info {
+       struct tc_action common;
+       struct tcf_connmark_parms __rcu *parms;
 };
 
 #define to_connmark(a) ((struct tcf_connmark_info *)a)

diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c
index 7e63ff7..8dabfb5 100644 (file)
--- a/net/sched/act_connmark.c
+++ b/net/sched/act_connmark.c
@@ -36,13 +36,15 @@ TC_INDIRECT_SCOPE int tcf_connmark_act(struct sk_buff *skb,
        struct nf_conntrack_tuple tuple;
        enum ip_conntrack_info ctinfo;
        struct tcf_connmark_info *ca = to_connmark(a);
+       struct tcf_connmark_parms *parms;
        struct nf_conntrack_zone zone;
        struct nf_conn *c;
        int proto;
 
-       spin_lock(&ca->tcf_lock);
        tcf_lastuse_update(&ca->tcf_tm);
-       bstats_update(&ca->tcf_bstats, skb);
+       tcf_action_update_bstats(&ca->common, skb);
+
+       parms = rcu_dereference_bh(ca->parms);
 
        switch (skb_protocol(skb, true)) {
        case htons(ETH_P_IP):
@@ -64,31 +66,29 @@ TC_INDIRECT_SCOPE int tcf_connmark_act(struct sk_buff *skb,
        c = nf_ct_get(skb, &ctinfo);
        if (c) {
                skb->mark = READ_ONCE(c->mark);
-               /* using overlimits stats to count how many packets marked */
-               ca->tcf_qstats.overlimits++;
-               goto out;
+               goto count;
        }
 
-       if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb),
-                              proto, ca->net, &tuple))
+       if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb), proto, parms->net,
+                              &tuple))
                goto out;
 
-       zone.id = ca->zone;
+       zone.id = parms->zone;
        zone.dir = NF_CT_DEFAULT_ZONE_DIR;
 
-       thash = nf_conntrack_find_get(ca->net, &zone, &tuple);
+       thash = nf_conntrack_find_get(parms->net, &zone, &tuple);
        if (!thash)
                goto out;
 
        c = nf_ct_tuplehash_to_ctrack(thash);
-       /* using overlimits stats to count how many packets marked */
-       ca->tcf_qstats.overlimits++;
        skb->mark = READ_ONCE(c->mark);
        nf_ct_put(c);
 
+count:
+       /* using overlimits stats to count how many packets marked */
+       tcf_action_inc_overlimit_qstats(&ca->common);
 out:
-       spin_unlock(&ca->tcf_lock);
-       return ca->tcf_action;
+       return READ_ONCE(ca->tcf_action);
 }
 
 static const struct nla_policy connmark_policy[TCA_CONNMARK_MAX + 1] = {
@@ -101,6 +101,7 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla,
                             struct netlink_ext_ack *extack)
 {
        struct tc_action_net *tn = net_generic(net, act_connmark_ops.net_id);
+       struct tcf_connmark_parms *nparms, *oparms;
        struct nlattr *tb[TCA_CONNMARK_MAX + 1];
        bool bind = flags & TCA_ACT_FLAGS_BIND;
        struct tcf_chain *goto_ch = NULL;
@@ -120,52 +121,66 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla,
        if (!tb[TCA_CONNMARK_PARMS])
                return -EINVAL;
 
+       nparms = kzalloc(sizeof(*nparms), GFP_KERNEL);
+       if (!nparms)
+               return -ENOMEM;
+
        parm = nla_data(tb[TCA_CONNMARK_PARMS]);
        index = parm->index;
        ret = tcf_idr_check_alloc(tn, &index, a, bind);
        if (!ret) {
-               ret = tcf_idr_create(tn, index, est, a,
-                                    &act_connmark_ops, bind, false, flags);
+               ret = tcf_idr_create_from_flags(tn, index, est, a,
+                                               &act_connmark_ops, bind, flags);
                if (ret) {
                        tcf_idr_cleanup(tn, index);
-                       return ret;
+                       err = ret;
+                       goto out_free;
                }
 
                ci = to_connmark(*a);
-               err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch,
-                                              extack);
-               if (err < 0)
-                       goto release_idr;
-               tcf_action_set_ctrlact(*a, parm->action, goto_ch);
-               ci->net = net;
-               ci->zone = parm->zone;
+
+               nparms->net = net;
+               nparms->zone = parm->zone;
 
                ret = ACT_P_CREATED;
        } else if (ret > 0) {
                ci = to_connmark(*a);
-               if (bind)
-                       return 0;
-               if (!(flags & TCA_ACT_FLAGS_REPLACE)) {
-                       tcf_idr_release(*a, bind);
-                       return -EEXIST;
+               if (bind) {
+                       err = 0;
+                       goto out_free;
                }
-               err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch,
-                                              extack);
-               if (err < 0)
+               if (!(flags & TCA_ACT_FLAGS_REPLACE)) {
+                       err = -EEXIST;
                        goto release_idr;
-               /* replacing action and zone */
-               spin_lock_bh(&ci->tcf_lock);
-               goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
-               ci->zone = parm->zone;
-               spin_unlock_bh(&ci->tcf_lock);
-               if (goto_ch)
-                       tcf_chain_put_by_act(goto_ch);
+               }
+
+               nparms->net = rtnl_dereference(ci->parms)->net;
+               nparms->zone = parm->zone;
+
                ret = 0;
        }
 
+       err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch, extack);
+       if (err < 0)
+               goto release_idr;
+
+       spin_lock_bh(&ci->tcf_lock);
+       goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
+       oparms = rcu_replace_pointer(ci->parms, nparms, lockdep_is_held(&ci->tcf_lock));
+       spin_unlock_bh(&ci->tcf_lock);
+
+       if (goto_ch)
+               tcf_chain_put_by_act(goto_ch);
+
+       if (oparms)
+               kfree_rcu(oparms, rcu);
+
        return ret;
+
 release_idr:
        tcf_idr_release(*a, bind);
+out_free:
+       kfree(nparms);
        return err;
 }
 
@@ -179,11 +194,14 @@ static inline int tcf_connmark_dump(struct sk_buff *skb, struct tc_action *a,
                .refcnt  = refcount_read(&ci->tcf_refcnt) - ref,
                .bindcnt = atomic_read(&ci->tcf_bindcnt) - bind,
        };
+       struct tcf_connmark_parms *parms;
        struct tcf_t t;
 
        spin_lock_bh(&ci->tcf_lock);
+       parms = rcu_dereference_protected(ci->parms, lockdep_is_held(&ci->tcf_lock));
+
        opt.action = ci->tcf_action;
-       opt.zone = ci->zone;
+       opt.zone = parms->zone;
        if (nla_put(skb, TCA_CONNMARK_PARMS, sizeof(opt), &opt))
                goto nla_put_failure;
 
@@ -201,6 +219,16 @@ nla_put_failure:
        return -1;
 }
 
+static void tcf_connmark_cleanup(struct tc_action *a)
+{
+       struct tcf_connmark_info *ci = to_connmark(a);
+       struct tcf_connmark_parms *parms;
+
+       parms = rcu_dereference_protected(ci->parms, 1);
+       if (parms)
+               kfree_rcu(parms, rcu);
+}
+
 static struct tc_action_ops act_connmark_ops = {
        .kind           =       "connmark",
        .id             =       TCA_ID_CONNMARK,
@@ -208,6 +236,7 @@ static struct tc_action_ops act_connmark_ops = {
        .act            =       tcf_connmark_act,
        .dump           =       tcf_connmark_dump,
        .init           =       tcf_connmark_init,
+       .cleanup        =       tcf_connmark_cleanup,
        .size           =       sizeof(struct tcf_connmark_info),
 };