Re: tcf_generic_walker(): what's going on?

[Patrick McHardy <kaber@trash.net>]

Denis Vlasenko wrote:
> While hunting down oversized inlines
> I stumbled on tcf_generic_walker().
> 
> It is defined in two separate files:
> once as an inline in include/net/pkt_act.h
> (really big one, ~750 bytes of code)
> and once as a static function in net/sched/act_police.c
> 
> These two instances are not identical.
> Second one has one extra parameter (int type)
> and it uses it like this:
> 
>         for (i = 0; i < MY_TAB_SIZE; i++) {
>                 p = tcf_police_ht[tcf_police_hash(i)];
> 
>                 for (; p; p = p->next) {
>                         index++;
>                         if (index < s_i)
>                                 continue;
>                         a->priv = p;
>                         a->order = index;
>                         r = (struct rtattr*) skb->tail;
>                         RTA_PUT(skb, a->order, 0, NULL);
> +                       if (type == RTM_DELACTION)
> +                               err = tcf_action_dump_1(skb, a, 0, 1);
> +                       else
> +                               err = tcf_action_dump_1(skb, a, 0, 0);
> 
> Having two functions with same name is rather confusing.
> Worse, they are both are called via five different
> tc_action_ops structs: 
> 
> static struct tc_action_ops act_ipt_ops = {
> ...
>         .walk           =       tcf_generic_walker
> 
> and I fail to understand how it is supposed to work,
> considering the fact that these two tcf_generic_walker's
> have different prototypes.

This code is a lazy hack to achieve "genericness" by using
a "generic" inline functions and #defines to create
specialized instances.

> 1) What should I do with tcf_generic_walker?
> 2) Should I deinline huge inlines in include/net/pkt_act.h?
>    If yes, to which .c file should I move them?

Unforunately you can't do this without further restructuring,
look at how the tc actions use that file:

/* use generic hash table */
#define MY_TAB_SIZE     8
#define MY_TAB_MASK     (MY_TAB_SIZE - 1)
static u32 idx_gen;
static struct tcf_mirred *tcf_mirred_ht[MY_TAB_SIZE];
static DEFINE_RWLOCK(mirred_lock);

/* ovewrride the defaults */
#define tcf_st          tcf_mirred
#define tc_st           tc_mirred
#define tcf_t_lock      mirred_lock
#define tcf_ht          tcf_mirred_ht

#define CONFIG_NET_ACT_INIT 1
#include <net/pkt_act.h>

What needs to be done is to put a pointer to the hash, its size
and its lock in struct tc_action_ops and move the "generic"
functions to a seperate .c file and make them work on a struct
tcf_act_common.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/