| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20200420141750.GL6581@nanopsycho.orion>
Date: Mon, 20 Apr 2020 16:17:50 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Maor Gottlieb <maorg@...lanox.com>
Cc: davem@...emloft.net, jgg@...lanox.com, dledford@...hat.com,
j.vosburgh@...il.com, vfalico@...il.com, andy@...yhouse.net,
kuba@...nel.org, leonro@...lanox.com, saeedm@...lanox.com,
jiri@...lanox.com, linux-rdma@...r.kernel.org,
netdev@...r.kernel.org, alexr@...lanox.com
Subject: Re: [PATCH V2 mlx5-next 02/10] bonding: Rename slave_arr to
usable_slaves
Mon, Apr 20, 2020 at 09:54:18AM CEST, maorg@...lanox.com wrote:
>This patch renames slave_arr to usable_slaves, since we will
>have two arrays, one for the usable slaves and the other to all
>slaves. In addition, exports the bond_skip_slave logic to function.
I the patch description, you should tell the codebase what to do. You
should not talk about "this patch".
>
>Signed-off-by: Maor Gottlieb <maorg@...lanox.com>
>---
> drivers/net/bonding/bond_alb.c | 4 +-
> drivers/net/bonding/bond_main.c | 85 +++++++++++++++++----------------
> include/net/bonding.h | 2 +-
> 3 files changed, 48 insertions(+), 43 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
>index c81698550e5a..7bb49b049dcc 100644
>--- a/drivers/net/bonding/bond_alb.c
>+++ b/drivers/net/bonding/bond_alb.c
>@@ -1360,7 +1360,7 @@ netdev_tx_t bond_tlb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
> struct bond_up_slave *slaves;
> unsigned int count;
>
>- slaves = rcu_dereference(bond->slave_arr);
>+ slaves = rcu_dereference(bond->usable_slaves);
> count = slaves ? READ_ONCE(slaves->count) : 0;
> if (likely(count))
> tx_slave = slaves->arr[hash_index %
>@@ -1494,7 +1494,7 @@ netdev_tx_t bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
> struct bond_up_slave *slaves;
> unsigned int count;
>
>- slaves = rcu_dereference(bond->slave_arr);
>+ slaves = rcu_dereference(bond->usable_slaves);
> count = slaves ? READ_ONCE(slaves->count) : 0;
> if (likely(count))
> tx_slave = slaves->arr[bond_xmit_hash(bond, skb) %
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 2e70e43c5df5..2cb41d480ae2 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -4087,6 +4087,29 @@ static void bond_slave_arr_handler(struct work_struct *work)
> bond_slave_arr_work_rearm(bond, 1);
> }
>
>+static void bond_skip_slave(struct bond_up_slave *slaves,
>+ struct slave *skipslave)
>+{
>+ int idx;
>+
>+ /* Rare situation where caller has asked to skip a specific
>+ * slave but allocation failed (most likely!). BTW this is
>+ * only possible when the call is initiated from
>+ * __bond_release_one(). In this situation; overwrite the
>+ * skipslave entry in the array with the last entry from the
>+ * array to avoid a situation where the xmit path may choose
>+ * this to-be-skipped slave to send a packet out.
>+ */
>+ for (idx = 0; slaves && idx < slaves->count; idx++) {
>+ if (skipslave == slaves->arr[idx]) {
>+ slaves->arr[idx] =
>+ slaves->arr[slaves->count - 1];
>+ slaves->count--;
>+ break;
>+ }
>+ }
Do this move in a separate patch. Is not related to the rename.
>+}
>+
> /* Build the usable slaves array in control path for modes that use xmit-hash
> * to determine the slave interface -
> * (a) BOND_MODE_8023AD
>@@ -4097,9 +4120,9 @@ static void bond_slave_arr_handler(struct work_struct *work)
> */
> int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
> {
>+ struct bond_up_slave *usable_slaves, *old_usable_slaves;
> struct slave *slave;
> struct list_head *iter;
>- struct bond_up_slave *new_arr, *old_arr;
> int agg_id = 0;
> int ret = 0;
>
>@@ -4107,11 +4130,10 @@ int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
> WARN_ON(lockdep_is_held(&bond->mode_lock));
> #endif
>
>- new_arr = kzalloc(offsetof(struct bond_up_slave, arr[bond->slave_cnt]),
>- GFP_KERNEL);
>- if (!new_arr) {
>+ usable_slaves = kzalloc(struct_size(usable_slaves, arr,
>+ bond->slave_cnt), GFP_KERNEL);
>+ if (!usable_slaves) {
> ret = -ENOMEM;
>- pr_err("Failed to build slave-array.\n");
> goto out;
> }
> if (BOND_MODE(bond) == BOND_MODE_8023AD) {
>@@ -4119,14 +4141,14 @@ int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
>
> if (bond_3ad_get_active_agg_info(bond, &ad_info)) {
> pr_debug("bond_3ad_get_active_agg_info failed\n");
>- kfree_rcu(new_arr, rcu);
>+ kfree_rcu(usable_slaves, rcu);
> /* No active aggragator means it's not safe to use
> * 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);
>+ old_usable_slaves = rtnl_dereference(bond->usable_slaves);
>+ if (old_usable_slaves) {
>+ RCU_INIT_POINTER(bond->usable_slaves, NULL);
>+ kfree_rcu(old_usable_slaves, rcu);
> }
> goto out;
> }
>@@ -4146,37 +4168,20 @@ int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
> continue;
>
> slave_dbg(bond->dev, slave->dev, "Adding slave to tx hash array[%d]\n",
>- new_arr->count);
>+ usable_slaves->count);
>
>- new_arr->arr[new_arr->count++] = slave;
>+ usable_slaves->arr[usable_slaves->count++] = slave;
> }
>
>- old_arr = rtnl_dereference(bond->slave_arr);
>- rcu_assign_pointer(bond->slave_arr, new_arr);
>- if (old_arr)
>- kfree_rcu(old_arr, rcu);
>+ old_usable_slaves = rtnl_dereference(bond->usable_slaves);
>+ rcu_assign_pointer(bond->usable_slaves, usable_slaves);
>+ if (old_usable_slaves)
>+ kfree_rcu(old_usable_slaves, rcu);
> out:
>- if (ret != 0 && skipslave) {
>- int idx;
>-
>- /* Rare situation where caller has asked to skip a specific
>- * slave but allocation failed (most likely!). BTW this is
>- * only possible when the call is initiated from
>- * __bond_release_one(). In this situation; overwrite the
>- * skipslave entry in the array with the last entry from the
>- * array to avoid a situation where the xmit path may choose
>- * this to-be-skipped slave to send a packet out.
>- */
>- old_arr = rtnl_dereference(bond->slave_arr);
>- for (idx = 0; old_arr != NULL && idx < old_arr->count; idx++) {
>- if (skipslave == old_arr->arr[idx]) {
>- old_arr->arr[idx] =
>- old_arr->arr[old_arr->count-1];
>- old_arr->count--;
>- break;
>- }
>- }
>- }
>+ if (ret != 0 && skipslave)
>+ bond_skip_slave(rtnl_dereference(bond->usable_slaves),
>+ skipslave);
>+
> return ret;
> }
>
>@@ -4192,7 +4197,7 @@ static netdev_tx_t bond_3ad_xor_xmit(struct sk_buff *skb,
> struct bond_up_slave *slaves;
> unsigned int count;
>
>- slaves = rcu_dereference(bond->slave_arr);
>+ slaves = rcu_dereference(bond->usable_slaves);
> count = slaves ? READ_ONCE(slaves->count) : 0;
> if (likely(count)) {
> slave = slaves->arr[bond_xmit_hash(bond, skb) % count];
>@@ -4483,9 +4488,9 @@ static void bond_uninit(struct net_device *bond_dev)
> __bond_release_one(bond_dev, slave->dev, true, true);
> netdev_info(bond_dev, "Released all slaves\n");
>
>- arr = rtnl_dereference(bond->slave_arr);
>+ arr = rtnl_dereference(bond->usable_slaves);
> if (arr) {
>- RCU_INIT_POINTER(bond->slave_arr, NULL);
>+ RCU_INIT_POINTER(bond->usable_slaves, NULL);
> kfree_rcu(arr, rcu);
> }
>
>diff --git a/include/net/bonding.h b/include/net/bonding.h
>index dc2ce31a1f52..33bdb6d5182d 100644
>--- a/include/net/bonding.h
>+++ b/include/net/bonding.h
>@@ -200,7 +200,7 @@ struct bonding {
> struct slave __rcu *curr_active_slave;
> struct slave __rcu *current_arp_slave;
> struct slave __rcu *primary_slave;
>- struct bond_up_slave __rcu *slave_arr; /* Array of usable slaves */
>+ struct bond_up_slave __rcu *usable_slaves; /* Array of usable slaves */
> bool force_primary;
> s32 slave_cnt; /* never change this value outside the attach/detach wrappers */
> int (*recv_probe)(const struct sk_buff *, struct bonding *,
>--
>2.17.2
>
Powered by blists - more mailing lists