lkml.org 
[lkml]   [2006]   [Apr]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    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.026 / U:33.592 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site