[ovs-dev] [PATCH 2/5] netdev-linux: Cache mtu ioctl error code along with mtu.

[Ben Pfaff]

On Tue, Mar 06, 2012 at 02:37:08PM -0800, Pravin B Shelar wrote:
> netdev linux devices uses mtu ioctl to get and set MTU for a device.
> By caching error code from ioctl we can reduce number of ioctl calls
> for device which is unregistered from system.
> netdev notification is used to update mtu which saves get-mtu-ioctl.
> 
> Signed-off-by: Pravin B Shelar <pshelar at nicira.com>

Thanks.

In netdev_dev_linux_changed(), I don't understand the distinction
between 
    if (change->nlmsg_type == RTM_NEWLINK) {
and
    if (change->nlmsg_type != RTM_DELLINK) {
I think that RTM_NEWLINK and RTM_DELLINK are the only two kinds of
notifications that the kernel will send for the RTNLGRP_LINK group
that rtnetlink-link.c monitors, so these seem equivalent.

Maybe it has to do with the call to netdev_dev_linux_changed() in
netdev_linux_caceh_cb() that uses a "synthesized"
rtnetlink_link_change struct and sets nlmsg_type to 0?  Actually, the
more I look at that usage, the more it worries me.  I think that it
will, for example, always cause the MTU to be reset to zero.  The same
goes for the similar usage in netdev_linux_miimon_run().

Here is one idea.  Break netdev_dev_linux_changed() into two
functions, one that contains essentially this:

    dev->change_seq++;
    if (!dev->change_seq) {
        dev->change_seq++;
    }

    if ((dev->ifi_flags ^ change->ifi_flags) & IFF_RUNNING) {
        dev->carrier_resets++;
    }
    dev->ifi_flags = change->ifi_flags;

    /* Always cache driver-info. */
    dev->cache_valid &= VALID_DRVINFO;

and another one that takes an rtnetlink_link_change struct (I think
there's only one place where that is the case, so maybe the additional
code could even be integrated into that place).  In other words, we'd
end up with a function much like before patch 1/5 (and maybe a
different structuring of the patch series might be good, then, but I
won't insist on it).

In case this change doesn't make sense to you, then: in OVS userspace, we
do not usually write parentheses around the operand of "sizeof" when
they are not required, as below.  Would you mind removing them in this
case?  Thanks.
> +        struct rtnetlink_link_change dev_change;
> +
> +        memset(&dev_change, 0, sizeof(dev_change));
>  
>          shash_init(&device_shash);
>          netdev_dev_get_devices(&netdev_linux_class, &device_shash);