lkml.org 
[lkml]   [2012]   [Aug]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [patch net-next 01/16] net: introduce upper device lists
    From
    Date
    On Mon, 2012-08-13 at 17:27 +0200, Jiri Pirko wrote:
    > This lists are supposed to serve for storing pointers to all upper devices.
    > Eventually it will replace dev->master pointer which is used for
    > bonding, bridge, team but it cannot be used for vlan, macvlan where
    > there might be multiple "masters" present.
    >
    > New upper device list resolves this limitation. Also, the information
    > stored in lists is used for preventing looping setups like
    > "bond->somethingelse->samebond"
    >
    > Signed-off-by: Jiri Pirko <jiri@resnulli.us>
    [...]
    > --- a/net/core/dev.c
    > +++ b/net/core/dev.c
    > @@ -4425,6 +4425,229 @@ static int __init dev_proc_init(void)
    > #endif /* CONFIG_PROC_FS */
    >
    >
    > +struct netdev_upper {
    > + struct net_device *dev;
    > + bool unique;

    This needs a better name. It doesn't really have anything to do with
    uniqueness and doesn't ensure exclusivity. I think that it would be
    fine to keep the 'master' term.

    > + struct list_head list;
    > + struct rcu_head rcu;
    > +};
    [...]
    > +static int __netdev_upper_dev_link(struct net_device *dev,
    > + struct net_device *upper_dev, bool unique)
    > +{
    > + struct netdev_upper *upper;
    > +
    > + ASSERT_RTNL();
    > +
    > + if (dev == upper_dev)
    > + return -EBUSY;
    > + /*
    > + * To prevent loops, check if dev is not upper device to upper_dev.
    > + */
    > + if (__netdev_has_upper_dev(upper_dev, dev, true))
    > + return -EBUSY;
    > +
    > + if (__netdev_find_upper(dev, upper_dev))
    > + return -EEXIST;
    > +
    > + if (unique && netdev_unique_upper_dev_get(dev))
    > + return -EBUSY;
    > +
    > + upper = kmalloc(sizeof(*upper), GFP_KERNEL);
    > + if (!upper)
    > + return -ENOMEM;
    > +
    > + upper->dev = upper_dev;
    > + upper->unique = unique;
    > +
    > + /*
    > + * Ensure that unique upper link is always the first item in the list.
    > + */
    > + if (unique)
    > + list_add_rcu(&upper->list, &dev->upper_dev_list);
    > + else
    > + list_add_tail_rcu(&upper->list, &dev->upper_dev_list);
    > + dev_hold(upper_dev);

    This behaviour (calling dev_hold()) matches netdev_set_master(). But
    it's oddly asymmetric: generally the administrator can remove either the
    upper device or the lower device (rtnl_link_ops or unbinding a physical
    device) and the upper device driver must then unlink itself from the
    lower device (using a notifier to catch lower device removal).

    If the upper device driver fails to unlink when the upper device is
    unregistered, then this extra reference causes netdev_wait_allrefs() to
    hang... is that the intent? Or should there be a more explicit counter
    and check on unregistration, e.g. WARN_ON(dev->num_lower_devs != 0)?

    If it fails to unlink when the lower device is removed, this warning in
    rollback_registered_many() may be triggered:

    /* Notifier chain MUST detach us from master device. */
    WARN_ON(dev->master);

    I think that needs to become WARN_ON(netdev_has_upper_dev(dev)).

    > + return 0;
    > +}
    [...]

    --
    Ben Hutchings, Staff Engineer, Solarflare
    Not speaking for my employer; that's the marketing department's job.
    They asked us to note that Solarflare product names are trademarked.



    \
     
     \ /
      Last update: 2012-08-13 22:43    [W:0.029 / U:30.692 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site