lkml.org 
[lkml]   [2006]   [Apr]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] deinline a few large functions in vlan code v2
Date
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
-
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/

\
 
 \ /
  Last update: 2006-04-11 15:20    [W:0.587 / U:0.412 seconds]
©2003-2014 Jasper Spaans. Advertise on this site