| Message ID | 5270ECB5.2030601@mojatatu.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, 2013-10-30 at 07:25 -0400, Jamal Hadi Salim wrote: > Attached. Tested with simple action. > > cheers, > jamal Why not setting .lookup to tcf_hash_search in the few actions not already doing that ? This would be more consistent. # git grep -n tcf_hash_search include/net/act_api.h:92:int tcf_hash_search(struct tc_action *a, u32 index); net/sched/act_api.c:198:int tcf_hash_search(struct tc_action *a, u32 index) net/sched/act_api.c:209:EXPORT_SYMBOL(tcf_hash_search); net/sched/act_csum.c:588: .lookup = tcf_hash_search, net/sched/act_gact.c:209: .lookup = tcf_hash_search, net/sched/act_ipt.c:301: .lookup = tcf_hash_search, net/sched/act_ipt.c:315: .lookup = tcf_hash_search, net/sched/act_mirred.c:274: .lookup = tcf_hash_search, net/sched/act_nat.c:311: .lookup = tcf_hash_search, net/sched/act_pedit.c:246: .lookup = tcf_hash_search, net/sched/act_police.c:410: .lookup = tcf_hash_search, -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Wed, 30 Oct 2013 07:00:05 -0700 > On Wed, 2013-10-30 at 07:25 -0400, Jamal Hadi Salim wrote: >> Attached. Tested with simple action. >> >> cheers, >> jamal > > Why not setting .lookup to tcf_hash_search > in the few actions not already doing that ? > > This would be more consistent. > # git grep -n tcf_hash_search > include/net/act_api.h:92:int tcf_hash_search(struct tc_action *a, u32 index); > net/sched/act_api.c:198:int tcf_hash_search(struct tc_action *a, u32 index) > net/sched/act_api.c:209:EXPORT_SYMBOL(tcf_hash_search); > net/sched/act_csum.c:588: .lookup = tcf_hash_search, > net/sched/act_gact.c:209: .lookup = tcf_hash_search, > net/sched/act_ipt.c:301: .lookup = tcf_hash_search, > net/sched/act_ipt.c:315: .lookup = tcf_hash_search, > net/sched/act_mirred.c:274: .lookup = tcf_hash_search, > net/sched/act_nat.c:311: .lookup = tcf_hash_search, > net/sched/act_pedit.c:246: .lookup = tcf_hash_search, > net/sched/act_police.c:410: .lookup = tcf_hash_search, Right, and BUG() if we try to register and action with a NULL .lookup member. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: David Miller <davem@davemloft.net> Date: Sun, 03 Nov 2013 23:12:32 -0500 (EST) > Right, and BUG() if we try to register and action with a NULL .lookup > member. I return an error, BUG() is too harsh. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Apologies for the latency My intention was to eventually remove it everywhere since this is needed by all actions. An action could override it, otherwise they get the default. cheers, jamal On 11/03/13 23:12, David Miller wrote: > From: Eric Dumazet <eric.dumazet@gmail.com> > Date: Wed, 30 Oct 2013 07:00:05 -0700 > >> On Wed, 2013-10-30 at 07:25 -0400, Jamal Hadi Salim wrote: >>> Attached. Tested with simple action. >>> >>> cheers, >>> jamal >> >> Why not setting .lookup to tcf_hash_search >> in the few actions not already doing that ? >> >> This would be more consistent. >> # git grep -n tcf_hash_search >> include/net/act_api.h:92:int tcf_hash_search(struct tc_action *a, u32 index); >> net/sched/act_api.c:198:int tcf_hash_search(struct tc_action *a, u32 index) >> net/sched/act_api.c:209:EXPORT_SYMBOL(tcf_hash_search); >> net/sched/act_csum.c:588: .lookup = tcf_hash_search, >> net/sched/act_gact.c:209: .lookup = tcf_hash_search, >> net/sched/act_ipt.c:301: .lookup = tcf_hash_search, >> net/sched/act_ipt.c:315: .lookup = tcf_hash_search, >> net/sched/act_mirred.c:274: .lookup = tcf_hash_search, >> net/sched/act_nat.c:311: .lookup = tcf_hash_search, >> net/sched/act_pedit.c:246: .lookup = tcf_hash_search, >> net/sched/act_police.c:410: .lookup = tcf_hash_search, > > Right, and BUG() if we try to register and action with a NULL .lookup > member. > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/03/13 23:17, David Miller wrote: > From: David Miller <davem@davemloft.net> > Date: Sun, 03 Nov 2013 23:12:32 -0500 (EST) > >> Right, and BUG() if we try to register and action with a NULL .lookup >> member. > > I return an error, BUG() is too harsh. > Sorry - I was distracted, but have time now. I wanted to fix this then send the other patches - but confused. We are setting the default if someone registers an action with a NULL .lookup. Why do we want to return an error? cheers, jamal -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/sched/act_api.c b/net/sched/act_api.c index fd70728..3c974a0 100644 --- a/net/sched/act_api.c +++ b/net/sched/act_api.c @@ -279,6 +279,10 @@ int tcf_register_action(struct tc_action_ops *act) } act->next = NULL; *ap = act; + + if (!act->lookup) + act->lookup = tcf_hash_search; + write_unlock(&act_mod_lock); return 0; }
Attached. Tested with simple action. cheers, jamal commit 80f80120ba4b85daf00c53eceebe7f0ad0b58a57 Author: Jamal Hadi Salim <jhs@mojatatu.com> Date: Wed Oct 30 07:03:55 2013 -0400 Provide default lookup function for actions that dont provide one Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>