Subject: Re: [PATCH net-next v3 1/6] bonding: simplify and use RCU protection for 3ad xmit path

From: Ding Tianhong <[email-masked]>
Date: Fri, 6 Sep 2013 09:41:38 +0800

On 2013/9/5 21:42, Veaceslav Falico wrote:
> On Thu, Sep 05, 2013 at 03:48:44PM +0800, Ding Tianhong wrote:
>> The commit 278b20837511776dc9d5f6ee1c7fabd5479838bb
>> (bonding: initial RCU conversion) has convert the roundrobin, active-backup,
>> broadcast and xor xmit path to rcu protection, the performance will be better
>> for these mode, so this time, convert xmit path for 3ad mode.
>>
>> Suggested-by: Nikolay Aleksandrov <[email-masked]>
>> Suggested-by: Veaceslav Falico <[email-masked]>
>> Signed-off-by: Ding Tianhong <[email-masked]>
>> Signed-off-by: Wang Yufen <[email-masked]>
>> Cc: Nikolay Aleksandrov <[email-masked]>
>> Cc: Veaceslav Falico <[email-masked]>
>> ---
>> drivers/net/bonding/bond_3ad.c | 32 ++++++++++++++------------------
>> drivers/net/bonding/bonding.h  | 32 +++++++++++++++++++++++++++++++-
>> 2 files changed, 45 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>> index 0d8f427..13f1deb 100644
>> --- a/drivers/net/bonding/bond_3ad.c
>> +++ b/drivers/net/bonding/bond_3ad.c
>> @@ -143,7 +143,7 @@ static inline struct bonding *__get_bond_by_port(struct port *port)
>>  */
>> static inline struct port *__get_first_port(struct bonding *bond)
>> {
>> -    struct slave *first_slave = bond_first_slave(bond);
>> +    struct slave *first_slave = bond_first_slave_rcu(bond);
>>
>>     return first_slave ? &(SLAVE_AD_INFO(first_slave).port) : NULL;
>> }
>> @@ -163,7 +163,7 @@ static inline struct port *__get_next_port(struct port *port)
>>     // If there's no bond for this port, or this is the last slave
>>     if (bond == NULL)
>>         return NULL;
>> -    slave_next = bond_next_slave(bond, slave);
>> +    slave_next = bond_next_slave_rcu(bond, slave);
>>     if (!slave_next || bond_is_first_slave(bond, slave_next))
>>         return NULL;
>>
>> @@ -2417,16 +2417,14 @@ int bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info)
>>
>> int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
>> {
>> -    struct slave *slave, *start_at;
>>     struct bonding *bond = netdev_priv(dev);
>> +    struct slave *slave;
>>     int slave_agg_no;
>>     int slaves_in_agg;
>>     int agg_id;
>> -    int i;
>>     struct ad_info ad_info;
>>     int res = 1;
>>
>> -    read_lock(&bond->lock);
>>     if (__bond_3ad_get_active_agg_info(bond, &ad_info)) {
>>         pr_debug("%s: Error: __bond_3ad_get_active_agg_info failed\n",
>>              dev->name);
>> @@ -2444,13 +2442,17 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
>>
>>     slave_agg_no = bond->xmit_hash_policy(skb, slaves_in_agg);
>>
>> -    bond_for_each_slave(bond, slave) {
>> +    bond_for_each_slave_rcu(bond, slave) {
>>         struct aggregator *agg = SLAVE_AD_INFO(slave).port.aggregator;
>>
>>         if (agg && (agg->aggregator_identifier == agg_id)) {
>> -            slave_agg_no--;
>> -            if (slave_agg_no < 0)
>> -                break;
>> +            if (--slave_agg_no < 0) {
>> +                if (SLAVE_IS_OK(slave)) {
>> +                    res = bond_dev_queue_xmit(bond,
>> +                        skb, slave->dev);
>> +                    goto out;
>> +                }
>> +            }
>>         }
>>     }
>>
>> @@ -2460,23 +2462,17 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
>>         goto out;
>>     }
>>
>> -    start_at = slave;
>> -
>> -    bond_for_each_slave_from(bond, slave, i, start_at) {
>> -        int slave_agg_id = 0;
>> +    bond_for_each_slave_rcu(bond, slave) {
>>         struct aggregator *agg = SLAVE_AD_INFO(slave).port.aggregator;
>>
>> -        if (agg)
>> -            slave_agg_id = agg->aggregator_identifier;
>> -
>> -        if (SLAVE_IS_OK(slave) && agg && (slave_agg_id == agg_id)) {
>> +        if (SLAVE_IS_OK(slave) && agg &&
>> +            agg->aggregator_identifier == agg_id) {
>>             res = bond_dev_queue_xmit(bond, skb, slave->dev);
>>             break;
>>         }
>>     }
>>
>> out:
>> -    read_unlock(&bond->lock);
>>     if (res) {
>>         /* no suitable interface, frame not sent */
>>         kfree_skb(skb);
>> diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
>> index f7ab161..f013b12 100644
>> --- a/drivers/net/bonding/bonding.h
>> +++ b/drivers/net/bonding/bonding.h
>> @@ -74,13 +74,34 @@
>> /* slave list primitives */
>> #define bond_to_slave(ptr) list_entry(ptr, struct slave, list)
>>
>> +/* slave list primitives, Caller must hold rcu_read_lock */
>> +#define bond_to_slave_rcu(ptr) list_entry_rcu(ptr, struct slave, list)
>> +
>> +/* bond_is_empty return NULL if slave list is empty*/
>> +#define bond_is_empty(bond) \
>> +    (list_empty(&(bond)->slave_list))
>> +
>> +/* bond_is_empty_rcu return NULL if slave list is empty*/
>> +#define bond_is_empty_rcu(bond) \
>> +    (!list_first_or_null_rcu(&(bond)->slave_list, struct slave, list))
>> +
>> /* IMPORTANT: bond_first/last_slave can return NULL in case of an empty list */
>> #define bond_first_slave(bond) \
>>     list_first_entry_or_null(&(bond)->slave_list, struct slave, list)
>> #define bond_last_slave(bond) \
>> -    (list_empty(&(bond)->slave_list) ? NULL : \
>> +    (bond_is_empty(bond) ? NULL : \
>>                        bond_to_slave((bond)->slave_list.prev))
>>
>> +/**
>> + * IMPORTANT: bond_first/last_slave_rcu can return NULL in case of an empty list
>> + * Caller must hold rcu_read_lock
>> + */
>> +#define bond_first_slave_rcu(bond) \
>> +    list_first_or_null_rcu(&(bond)->slave_list, struct slave, list)
>> +#define bond_last_slave_rcu(bond) \
>> +    (bond_is_empty_rcu(bond) ? NULL : \
>> +                    bond_to_slave_rcu((bond)->slave_list.prev))
> 
> Really?
> 
> No. Again - take a look at list_first_or_null_rcu. And its comments.
> 
> I'm sorry that I'm acting that negatively... But if that gets accepted -
> I'll have days of nightmares.
> 
> Try to understand how RCU works, please, before sending patches using it.
> 

After careful consideration and review the code, I find my problem and agree you,
thanks for your patient guidance, I will fix it as soon as possible.

>> +
>> #define bond_is_first_slave(bond, pos) ((pos)->list.prev == &(bond)->slave_list)
>> #define bond_is_last_slave(bond, pos) ((pos)->list.next == &(bond)->slave_list)
>>
>> @@ -93,6 +114,15 @@
>>     (bond_is_first_slave(bond, pos) ? bond_last_slave(bond) : \
>>                       bond_to_slave((pos)->list.prev))
>>
>> +/* Since bond_first/last_slave_rcu can return NULL, these can return NULL too */
>> +#define bond_next_slave_rcu(bond, pos) \
>> +    (bond_is_last_slave(bond, pos) ? bond_first_slave_rcu(bond) : \
>> +                     bond_to_slave_rcu((pos)->list.next))
>> +
>> +#define bond_prev_slave_rcu(bond, pos) \
>> +    (bond_is_first_slave(bond, pos) ? bond_last_slave_rcu(bond) : \
>> +                      bond_to_slave_rcu((pos)->list.prev))
>> +
>> /**
>>  * bond_for_each_slave_from - iterate the slaves list from a starting point
>>  * @bond:    the bond holding this list.
>> -- 
>> 1.8.2.1
>>
>>
>>
> 
> .
>