Re: [Patch net-next v2 2/2] net_sched: add network namespace support for tc actions

On Tue, Feb 23, 2016 at 5:14 AM, Jamal Hadi Salim [off-list ref] wrote:

On 16-02-22 06:57 PM, Cong Wang wrote:

[..]

quoted

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 8c4e3ff..342be6c 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h

@@ -7,6 +7,8 @@

  #include <net/sch_generic.h>
  #include <net/pkt_sched.h>
+#include <net/net_namespace.h>
+#include <net/netns/generic.h>

  struct tcf_common {
        struct hlist_node               tcfc_head;

@@ -87,31 +89,65 @@ struct tc_action {
        __u32                   type; /* for backward

compat(TCA_OLD_COMPAT) */
        __u32                   order;
        struct list_head        list;
+       struct tcf_hashinfo     *hinfo;
  };

It doesnt seem neccessary to have hinfo in tc_action. Quick scan:
__tcf_hash_release() seems to be the only other place that uses it.
And the callers to that appear capable of passing the struct
net or tn  which eventually propagates up...

The tcf_action_destroy() callchain still can't find out hinfo yet.

I know this is one of the ugly parts, this is why I mentioned it
in the changelog that we should refactor it. Do you mind if I
refactor this later?

quoted

  struct tc_action_ops {
        struct list_head head;
-       struct tcf_hashinfo *hinfo;
        char    kind[IFNAMSIZ];
        __u32   type; /* TBD to match kind */
        struct module           *owner;
        int     (*act)(struct sk_buff *, const struct tc_action *, struct
tcf_result *);
        int     (*dump)(struct sk_buff *, struct tc_action *, int, int);
        void    (*cleanup)(struct tc_action *, int bind);
-       int     (*lookup)(struct tc_action *, u32);
+       int     (*lookup)(struct net *, struct tc_action *, u32);
        int     (*init)(struct net *net, struct nlattr *nla,
                        struct nlattr *est, struct tc_action *act, int
ovr,
                        int bind);
-       int     (*walk)(struct sk_buff *, struct netlink_callback *, int,
struct tc_action *);
+       int     (*walk)(struct net *, struct sk_buff *,
+                       struct netlink_callback *, int, struct tc_action
*);
+};
+

Do you really need to pass struct net to walk(); is deriving from skb
not sufficient?

Yes, seems you are right.

quoted

+       int err = 0;

quoted

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index acafaf7..9606666 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c

@@ -36,10 +36,9 @@ static void free_tcf(struct rcu_head *head)
        kfree(p);
  }

-static void tcf_hash_destroy(struct tc_action *a)
+static void tcf_hash_destroy(struct tcf_hashinfo *hinfo, struct tc_action

*a)
  {
        struct tcf_common *p = a->priv;
-       struct tcf_hashinfo *hinfo = a->ops->hinfo;

        spin_lock_bh(&hinfo->lock);
        hlist_del(&p->tcfc_head);

@@ -68,7 +67,7 @@ int __tcf_hash_release(struct tc_action *a, bool bind,

bool strict)
                if (p->tcfc_bindcnt <= 0 && p->tcfc_refcnt <= 0) {
                        if (a->ops->cleanup)
                                a->ops->cleanup(a, bind);
-                       tcf_hash_destroy(a);
+                       tcf_hash_destroy(a->hinfo, a);

So this seems to be the only place where a->hinfo is read from. The
rest seems to just set a->hinfo.
I took a quick look at __tcf_hash_release() and all calling sites
are net/tn aware already.

tcf_action_destroy().

quoted

-u32 tcf_hash_new_index(struct tcf_hashinfo *hinfo)
+u32 tcf_hash_new_index(struct tc_action_net *tn)
  {
+       struct tcf_hashinfo *hinfo = tn->hinfo;
        u32 val = hinfo->index;

That also seemed unneeded. You could have derived hinfo
from tn.

This is a pure taste of the API, I want to hide the hinfo as much as
I can and expose tn to callers.

Otherwise looks reasonable. I was hoping we could get rid of the per
action pernet ops but that could come later.

That is hard (if not impossible), because we have to allocate the pernet
ops on heap, which seems not doable.


Thanks!