From mboxrd@z Thu Jan 1 00:00:00 1970 From: John Fastabend Subject: Re: [Patch net-next] net_sched: refactor out tcf_exts Date: Sun, 05 Oct 2014 20:04:20 -0700 Message-ID: <543206B4.7090504@gmail.com> References: <1412376709-25564-1-git-send-email-xiyou.wangcong@gmail.com> <5431F4A1.3040107@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, Jamal Hadi Salim , John Fastabend , "David S. Miller" To: Cong Wang Return-path: Received: from mail-ob0-f170.google.com ([209.85.214.170]:64543 "EHLO mail-ob0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751859AbaJFDEl (ORCPT ); Sun, 5 Oct 2014 23:04:41 -0400 Received: by mail-ob0-f170.google.com with SMTP id uz6so3262956obc.15 for ; Sun, 05 Oct 2014 20:04:40 -0700 (PDT) In-Reply-To: <5431F4A1.3040107@gmail.com> Sender: netdev-owner@vger.kernel.org List-ID: On 10/05/2014 06:47 PM, John Fastabend wrote: > On 10/03/2014 03:51 PM, Cong Wang wrote: >> As Jamal pointed it out, tcf_exts is really unnecessary, >> we can definitely refactor it out without losing any functionality. >> This could also remove an indirect layer which makes the code >> much easier to read. >> >> This patch: >> >> 1) moves exts->action and exts->police into tp->ops, since they >> are statically assigned >> >> 2) moves exts->actions list head out >> >> 3) removes exts->type, act->type does the same thing >> >> 4) renames tcf_exts_*() functions to tcf_act_*() >> >> Cc: Jamal Hadi Salim >> Cc: John Fastabend >> Cc: "David S. Miller" >> Signed-off-by: Cong Wang >> --- > > Looks OK to me and removes a layer of abstraction without changing > the code much. This is going to conflict with my series so I'll hold > off resubmitting it until this is dealt with. I need to respin that > ematch fix up to drop the ingress lock. > > Acked-by: John Fastabend > > [...] > But after running my test kit I see a null pointer dereference in cls_cgroup in tcf_act_change(). Looks like you dropped an initializer... @@ -116,7 +116,6 @@ static int cls_cgroup_change(struct net *net, struct sk_buff *in_skb, if (!new) return -ENOBUFS; - tcf_exts_init(&new->exts, TCA_CGROUP_ACT, TCA_CGROUP_POLICE); if (head) new->handle = head->handle; else >> >> -void tcf_exts_change(struct tcf_proto *tp, struct tcf_exts *dst, >> - struct tcf_exts *src) >> +void tcf_act_change(struct tcf_proto *tp, struct list_head *dst, >> + struct list_head *src) >> { >> #ifdef CONFIG_NET_CLS_ACT >> LIST_HEAD(tmp); >> tcf_tree_lock(tp); >> - list_splice_init(&dst->actions, &tmp); >> - list_splice(&src->actions, &dst->actions); >> + list_splice_init(dst, &tmp); >> + list_splice(src, dst); >> tcf_tree_unlock(tp); >> tcf_action_destroy(&tmp, TCA_ACT_UNBIND); > > > This is overly complex now that tcf_act_change only > occurs on null lists. And unattached ones because of the > RCU semantics so I'm fairly sure we can drop the lock and > double splice. > > > [...] > -- John Fastabend Intel Corporation