On Thu, Oct 2, 2014 at 10:10 AM, Cong Wang [off-list ref] wrote:

On Wed, Oct 1, 2014 at 1:38 AM, Mahesh Bandewar [off-list ref] wrote:

quoted

+int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
+{
+       struct slave *slave;
+       struct list_head *iter;
+       struct bond_up_slave *new_arr, *old_arr;
+       int slaves_in_agg;
+       int agg_id = 0;
+       int ret = 0;
+
+#ifdef CONFIG_LOCKDEP
+       WARN_ON(lockdep_is_held(&bond->mode_lock));
+#endif

I think you can use lockdep_is_held().

quoted

+
+       new_arr = kzalloc(offsetof(struct bond_up_slave, arr[bond->slave_cnt]),
+                         GFP_KERNEL);
+       if (!new_arr) {
+               ret = -ENOMEM;
+               pr_err("Failed to build slave-array.\n");
+               goto out;
+       }

No need to print an error message for OOM, it is already noisy. :)

Agreed OOM condition is noisy but failing silently would not help
debugging hence adding the message.

quoted

+       if (BOND_MODE(bond) == BOND_MODE_8023AD) {
+               struct ad_info ad_info;
+
+               if (bond_3ad_get_active_agg_info(bond, &ad_info)) {
+                       pr_debug("bond_3ad_get_active_agg_info failed\n");

I suspect how useful this debug info is since your patch is almost ready
to merge.

It could be useful for someone else too :)

quoted

+                       kfree_rcu(new_arr, rcu);
+                       /* No active aggragator means its not safe to use

s/its/it's/

thanks

quoted

+                        * the previous array.
+                        */
+                       old_arr = rtnl_dereference(bond->slave_arr);
+                       if (old_arr) {
+                               RCU_INIT_POINTER(bond->slave_arr, NULL);
+                               kfree_rcu(old_arr, rcu);
+                       }
+                       goto out;
+               }
+               slaves_in_agg = ad_info.ports;
+               agg_id = ad_info.aggregator_id;
+       }

Thanks.