Re: [PATCH] deinline a few large functions in vlan code v2

[Ingo Oeser <netdev@axxeo.de>]

Hi Denis,

Denis Vlasenko wrote:
> On Tuesday 11 April 2006 12:49, Ingo Oeser wrote:
> > #if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
> > static inline  has_vlan_group(...) {
> >   /* get VLAN group */
> > }
> > #else
> > static inline  has_vlan_group(...) {return 0;}
> > #endif
> > 
> > With this and similiar changes in the drivers, 
> > your patch might be less intrusive and thus more acceptable to maintainers.
> > 
> > Just let the compiler remove the extra code with constant folding and dead
> > code elemination. The result will be even cleaner code, I think.
> > 
> > What do you think?

I thought more of introducing functions, which just fold away 
all the "if ()" blocks in normal code paths, 
which you wrapped into "#if" here. 

I don't think people have problems, if you #ifdef out complete functions,
linear setup code or structure members. People seem to have more problems
with #ifdef in control flow code, because there the condition is nothing else but
a compile time constant (the CONFIG_FOO value) which should be expressed as such.

Instead of 

#ifdef CONFIG_FOO
if (condition) {
}
#endif

just do

#ifdef CONFIG_FOO
#define foo 1
#else
#define foo 0
#endif

if  (foo && condition) {
}

Just do nothing or return a compile time constant in those inlines.

> 
> Addresses of these functions are stored into netdevice members:
>
> @@ -2549,8 +2559,10 @@ typhoon_init_one(struct pci_dev *pdev, c
>         dev->watchdog_timeo     = TX_TIMEOUT;
>         dev->get_stats          = typhoon_get_stats;
>         dev->set_mac_address    = typhoon_set_mac_address;
> +#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
>         dev->vlan_rx_register   = typhoon_vlan_rx_register;
>         dev->vlan_rx_kill_vid   = typhoon_vlan_rx_kill_vid;
> +#endif
> 
> Even empty inline would not be "optimized out to nothing" here.

No problem, just optimize out the assignment itself:

#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
static inline void typhoon_setup_vlan_hooks(struct netdev *dev) {
         dev->vlan_rx_register = typhoon_vlan_rx_register;
         dev->vlan_rx_kill_vid = typhoon_vlan_rx_kill_vid;
}
#else
static inline void typhoon_setup_vlan_hooks(struct netdev *dev) { (void)dev; }
#endif

The whole linux/if_vlan.h stuff misses this style of code.
There is simply no fallback code for CONFIG_VLAN_8021Q not being defined.


Regards

Ingo Oeser