From: Jamal Hadi Salim <jhs@mojatatu•com>
To: Cong Wang <xiyou.wangcong@gmail•com>, netdev@vger•kernel.org
Subject: Re: [Patch net-next v2 2/2] net_sched: add network namespace support for tc actions
Date: Tue, 23 Feb 2016 08:14:29 -0500 [thread overview]
Message-ID: <56CC5B35.4060408@mojatatu.com> (raw)
In-Reply-To: <1456185473-25403-3-git-send-email-xiyou.wangcong@gmail.com>
On 16-02-22 06:57 PM, Cong Wang wrote:
[..]
>
> 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...
> 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?
> + int err = 0;
> 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.
> -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.
Otherwise looks reasonable. I was hoping we could get rid of the per
action pernet ops but that could come later.
cheers,
jamal
next prev parent reply other threads:[~2016-02-23 13:14 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-22 23:57 [Patch net-next v2 0/2] net_sched: add network namespace support for tc actions Cong Wang
2016-02-22 23:57 ` [Patch net-next v2 1/2] net_sched: prepare tcf_hashinfo_destroy() for netns support Cong Wang
2016-02-23 13:01 ` Jamal Hadi Salim
2016-02-22 23:57 ` [Patch net-next v2 2/2] net_sched: add network namespace support for tc actions Cong Wang
2016-02-23 13:14 ` Jamal Hadi Salim [this message]
2016-02-23 22:23 ` Cong Wang
2016-02-24 13:19 ` Jamal Hadi Salim
2016-02-24 17:21 ` Cong Wang
2016-02-25 12:17 ` Jamal Hadi Salim
2016-02-24 18:37 ` Cong Wang
2016-02-25 19:16 ` [Patch net-next v2 0/2] " David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56CC5B35.4060408@mojatatu.com \
--to=jhs@mojatatu$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=xiyou.wangcong@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox