On Wed, Mar 7, 2012 at 10:17 AM, Ben Pfaff <[email protected]> wrote:
> 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 <[email protected]>
>
> 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.
ok, I was not sure these are only two events
>
> 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().
>
right.
> 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).
>
ok. I will send updated patch.
> 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);
>
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
