| 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
| ||
|
Date: Mon, 20 Apr 2020 21:59:36 +0300
From: Maor Gottlieb <maorg@...lanox.com>
To: Jay Vosburgh <jay.vosburgh@...onical.com>,
Jiri Pirko <jiri@...nulli.us>
Cc: davem@...emloft.net, jgg@...lanox.com, dledford@...hat.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 03/10] bonding: Add helpers to get xmit slave
On 4/20/2020 9:46 PM, Jay Vosburgh wrote:
> Jiri Pirko <jiri@...nulli.us> wrote:
>
>> Mon, Apr 20, 2020 at 09:54:19AM CEST, maorg@...lanox.com wrote:
>>> This helpers will be used by both the xmit function
>>> and the get xmit slave ndo.
>> Be more verbose about what you are doing please. From this I have no
>> clue what is going on.
> Agreed, and also with Jiri's comment further down to split this
> into multiple patches. The current series is difficult to follow.
>
> -J
Agree, I am splitting this series to more patches so it will be easier
to follow.
>>> Signed-off-by: Maor Gottlieb <maorg@...lanox.com>
>>> ---
>>> drivers/net/bonding/bond_alb.c | 35 ++++++++----
>>> drivers/net/bonding/bond_main.c | 94 +++++++++++++++++++++------------
>>> include/net/bond_alb.h | 4 ++
>>> 3 files changed, 89 insertions(+), 44 deletions(-)
>>>
>>> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
>>> index 7bb49b049dcc..e863c694c309 100644
>>> --- a/drivers/net/bonding/bond_alb.c
>>> +++ b/drivers/net/bonding/bond_alb.c
>>> @@ -1334,11 +1334,11 @@ static netdev_tx_t bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
>>> return NETDEV_TX_OK;
>>> }
>>>
>>> -netdev_tx_t bond_tlb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
>>> +struct slave *bond_xmit_tlb_slave_get(struct bonding *bond,
>>> + struct sk_buff *skb)
>>> {
>>> - struct bonding *bond = netdev_priv(bond_dev);
>>> - struct ethhdr *eth_data;
>>> struct slave *tx_slave = NULL;
>>> + struct ethhdr *eth_data;
>>> u32 hash_index;
>>>
>>> skb_reset_mac_header(skb);
>>> @@ -1369,20 +1369,29 @@ netdev_tx_t bond_tlb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
>>> break;
>>> }
>>> }
>>> - return bond_do_alb_xmit(skb, bond, tx_slave);
>>> + return tx_slave;
>>> }
>>>
>>> -netdev_tx_t bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
>>> +netdev_tx_t bond_tlb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
>>> {
>>> struct bonding *bond = netdev_priv(bond_dev);
>>> - struct ethhdr *eth_data;
>>> + struct slave *tx_slave;
>>> +
>>> + tx_slave = bond_xmit_tlb_slave_get(bond, skb);
>>> + return bond_do_alb_xmit(skb, bond, tx_slave);
>>> +}
>>> +
>>> +struct slave *bond_xmit_alb_slave_get(struct bonding *bond,
>>> + struct sk_buff *skb)
>>> +{
>>> struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
>>> - struct slave *tx_slave = NULL;
>>> static const __be32 ip_bcast = htonl(0xffffffff);
>>> - int hash_size = 0;
>>> + struct slave *tx_slave = NULL;
>>> + const u8 *hash_start = NULL;
>>> bool do_tx_balance = true;
>>> + struct ethhdr *eth_data;
>>> u32 hash_index = 0;
>>> - const u8 *hash_start = NULL;
>>> + int hash_size = 0;
>>>
>>> skb_reset_mac_header(skb);
>>> eth_data = eth_hdr(skb);
>>> @@ -1501,7 +1510,15 @@ netdev_tx_t bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
>>> count];
>>> }
>>> }
>>> + return tx_slave;
>>> +}
>>> +
>>> +netdev_tx_t bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
>>> +{
>>> + struct bonding *bond = netdev_priv(bond_dev);
>>> + struct slave *tx_slave = NULL;
>>>
>>> + tx_slave = bond_xmit_alb_slave_get(bond, skb);
>>> return bond_do_alb_xmit(skb, bond, tx_slave);
>>> }
>>>
>>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>> index 2cb41d480ae2..7e04be86fda8 100644
>>> --- a/drivers/net/bonding/bond_main.c
>>> +++ b/drivers/net/bonding/bond_main.c
>>> @@ -82,6 +82,7 @@
>>> #include <net/bonding.h>
>>> #include <net/bond_3ad.h>
>>> #include <net/bond_alb.h>
>>> +#include <net/lag.h>
>>>
>>> #include "bonding_priv.h"
>>>
>>> @@ -3406,10 +3407,26 @@ u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb)
>>> (__force u32)flow_get_u32_src(&flow);
>>> hash ^= (hash >> 16);
>>> hash ^= (hash >> 8);
>>> -
>> Please avoid changes like this one.
>>
>>
>>> return hash >> 1;
>>> }
>>>
>>> +static struct slave *bond_xmit_3ad_xor_slave_get(struct bonding *bond,
>>> + struct sk_buff *skb,
>>> + struct bond_up_slave *slaves)
>>> +{
>>> + struct slave *slave;
>>> + unsigned int count;
>>> + u32 hash;
>>> +
>>> + hash = bond_xmit_hash(bond, skb);
>>> + count = slaves ? READ_ONCE(slaves->count) : 0;
>>> + if (unlikely(!count))
>>> + return NULL;
>>> +
>>> + slave = slaves->arr[hash % count];
>>> + return slave;
>>> +}
>> Why don't you have this helper near bond_3ad_xor_xmit() as you have for
>> round robin for example?
No good reason, will move it near.
>>
>> I think it would make this patch much easier to review if you split to
>> multiple patches, per-mode.
>>
>>
>>> +
>>> /*-------------------------- Device entry points ----------------------------*/
>>>
>>> void bond_work_init_all(struct bonding *bond)
>>> @@ -3923,16 +3940,15 @@ static int bond_set_mac_address(struct net_device *bond_dev, void *addr)
>>> }
>>>
>>> /**
>>> - * bond_xmit_slave_id - transmit skb through slave with slave_id
>>> + * bond_get_slave_by_id - get xmit slave with slave_id
>>> * @bond: bonding device that is transmitting
>>> - * @skb: buffer to transmit
>>> * @slave_id: slave id up to slave_cnt-1 through which to transmit
>>> *
>>> - * This function tries to transmit through slave with slave_id but in case
>>> + * This function tries to get slave with slave_id but in case
>>> * it fails, it tries to find the first available slave for transmission.
>>> - * The skb is consumed in all cases, thus the function is void.
>>> */
>>> -static void bond_xmit_slave_id(struct bonding *bond, struct sk_buff *skb, int slave_id)
>>> +static struct slave *bond_get_slave_by_id(struct bonding *bond,
>>> + int slave_id)
>>> {
>>> struct list_head *iter;
>>> struct slave *slave;
>>> @@ -3941,10 +3957,8 @@ static void bond_xmit_slave_id(struct bonding *bond, struct sk_buff *skb, int sl
>>> /* Here we start from the slave with slave_id */
>>> bond_for_each_slave_rcu(bond, slave, iter) {
>>> if (--i < 0) {
>>> - if (bond_slave_can_tx(slave)) {
>>> - bond_dev_queue_xmit(bond, skb, slave->dev);
>>> - return;
>>> - }
>>> + if (bond_slave_can_tx(slave))
>>> + return slave;
>>> }
>>> }
>>>
>>> @@ -3953,13 +3967,11 @@ static void bond_xmit_slave_id(struct bonding *bond, struct sk_buff *skb, int sl
>>> bond_for_each_slave_rcu(bond, slave, iter) {
>>> if (--i < 0)
>>> break;
>>> - if (bond_slave_can_tx(slave)) {
>>> - bond_dev_queue_xmit(bond, skb, slave->dev);
>>> - return;
>>> - }
>>> + if (bond_slave_can_tx(slave))
>>> + return slave;
>>> }
>>> - /* no slave that can tx has been found */
>>> - bond_tx_drop(bond->dev, skb);
>>> +
>>> + return NULL;
>>> }
>>>
>>> /**
>>> @@ -3995,10 +4007,9 @@ static u32 bond_rr_gen_slave_id(struct bonding *bond)
>>> return slave_id;
>>> }
>>>
>>> -static netdev_tx_t bond_xmit_roundrobin(struct sk_buff *skb,
>>> - struct net_device *bond_dev)
>>> +static struct slave *bond_xmit_roundrobin_slave_get(struct bonding *bond,
>>> + struct sk_buff *skb)
>>> {
>>> - struct bonding *bond = netdev_priv(bond_dev);
>>> struct slave *slave;
>>> int slave_cnt;
>>> u32 slave_id;
>>> @@ -4020,24 +4031,40 @@ static netdev_tx_t bond_xmit_roundrobin(struct sk_buff *skb,
>>> if (iph->protocol == IPPROTO_IGMP) {
>>> slave = rcu_dereference(bond->curr_active_slave);
>>> if (slave)
>>> - bond_dev_queue_xmit(bond, skb, slave->dev);
>>> - else
>>> - bond_xmit_slave_id(bond, skb, 0);
>>> - return NETDEV_TX_OK;
>>> + return slave;
>>> + return bond_get_slave_by_id(bond, 0);
>>> }
>>> }
>>>
>>> non_igmp:
>>> slave_cnt = READ_ONCE(bond->slave_cnt);
>>> if (likely(slave_cnt)) {
>>> - slave_id = bond_rr_gen_slave_id(bond);
>>> - bond_xmit_slave_id(bond, skb, slave_id % slave_cnt);
>>> - } else {
>>> - bond_tx_drop(bond_dev, skb);
>>> + slave_id = bond_rr_gen_slave_id(bond) % slave_cnt;
>>> + return bond_get_slave_by_id(bond, slave_id);
>>> }
>>> + return NULL;
>>> +}
>>> +
>>> +static netdev_tx_t bond_xmit_roundrobin(struct sk_buff *skb,
>>> + struct net_device *bond_dev)
>>> +{
>>> + struct bonding *bond = netdev_priv(bond_dev);
>>> + struct slave *slave;
>>> +
>>> + slave = bond_xmit_roundrobin_slave_get(bond, skb);
>>> + if (slave)
>>> + bond_dev_queue_xmit(bond, skb, slave->dev);
>>> + else
>>> + bond_tx_drop(bond_dev, skb);
>>> return NETDEV_TX_OK;
>>> }
>>>
>>> +static struct slave *bond_xmit_activebackup_slave_get(struct bonding *bond,
>>> + struct sk_buff *skb)
>>> +{
>>> + return rcu_dereference(bond->curr_active_slave);
>>> +}
>>> +
>>> /* In active-backup mode, we know that bond->curr_active_slave is always valid if
>>> * the bond has a usable interface.
>>> */
>>> @@ -4047,7 +4074,7 @@ static netdev_tx_t bond_xmit_activebackup(struct sk_buff *skb,
>>> struct bonding *bond = netdev_priv(bond_dev);
>>> struct slave *slave;
>>>
>>> - slave = rcu_dereference(bond->curr_active_slave);
>>> + slave = bond_xmit_activebackup_slave_get(bond, skb);
>>> if (slave)
>>> bond_dev_queue_xmit(bond, skb, slave->dev);
>>> else
>>> @@ -4193,18 +4220,15 @@ static netdev_tx_t bond_3ad_xor_xmit(struct sk_buff *skb,
>>> struct net_device *dev)
>>> {
>>> struct bonding *bond = netdev_priv(dev);
>>> - struct slave *slave;
>>> struct bond_up_slave *slaves;
>>> - unsigned int count;
>>> + struct slave *slave;
>>>
>>> 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];
>>> + slave = bond_xmit_3ad_xor_slave_get(bond, skb, slaves);
>>> + if (likely(slave))
>>> bond_dev_queue_xmit(bond, skb, slave->dev);
>>> - } else {
>>> + else
>>> bond_tx_drop(dev, skb);
>>> - }
>>>
>>> return NETDEV_TX_OK;
>>> }
>>> diff --git a/include/net/bond_alb.h b/include/net/bond_alb.h
>>> index b3504fcd773d..f6af76c87a6c 100644
>>> --- a/include/net/bond_alb.h
>>> +++ b/include/net/bond_alb.h
>>> @@ -158,6 +158,10 @@ void bond_alb_handle_link_change(struct bonding *bond, struct slave *slave, char
>>> void bond_alb_handle_active_change(struct bonding *bond, struct slave *new_slave);
>>> int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev);
>>> int bond_tlb_xmit(struct sk_buff *skb, struct net_device *bond_dev);
>>> +struct slave *bond_xmit_alb_slave_get(struct bonding *bond,
>>> + struct sk_buff *skb);
>>> +struct slave *bond_xmit_tlb_slave_get(struct bonding *bond,
>>> + struct sk_buff *skb);
>>> void bond_alb_monitor(struct work_struct *);
>>> int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr);
>>> void bond_alb_clear_vlan(struct bonding *bond, unsigned short vlan_id);
>>> --
>>> 2.17.2
>>>
> ---
> -Jay Vosburgh, jay.vosburgh@...onical.com
Powered by blists - more mailing lists