From: Ding Tianhong <dingtianhong@huawei•com>
To: Veaceslav Falico <vfalico@redhat•com>
Cc: <netdev@vger•kernel.org>, <jiri@resnulli•us>,
Jay Vosburgh <fubar@us•ibm.com>,
Andy Gospodarek <andy@greyhouse•net>
Subject: Re: [PATCH v5 net-next 11/27] bonding: rework bond_3ad_xmit_xor() to use bond_for_each_slave() only
Date: Thu, 26 Sep 2013 14:39:11 +0800 [thread overview]
Message-ID: <5243D68F.4090806@huawei.com> (raw)
In-Reply-To: <20130926054317.GA2547@redhat.com>
On 2013/9/26 13:43, Veaceslav Falico wrote:
> On Thu, Sep 26, 2013 at 10:31:35AM +0800, Ding Tianhong wrote:
>
> First of all - thanks a lot for the review! Answered below.
>
>> On 2013/9/25 15:20, Veaceslav Falico wrote:
>>> Currently, there are two loops - first we find the first slave in an
>>> aggregator after the xmit_hash_policy() returned number, and after that we
>>> loop from that slave, over bonding head, and till that slave to find any
>>> suitable slave to send the packet through.
>>>
>>> Replace it by just one bond_for_each_slave() loop, which first loops
>>> through the requested number of slaves, saving the first suitable one, and
>>> after that we've hit the requested number of slaves to skip - search for
>>> any up slave to send the packet through. If we don't find such kind of
>>> slave - then just send the packet through the first suitable slave found.
>>>
>>> Logic remains unchainged, and we skip two loops. Also, refactor it a bit
>>> for readability.
>>>
>>> CC: Jay Vosburgh <fubar@us•ibm.com>
>>> CC: Andy Gospodarek <andy@greyhouse•net>
>>> Signed-off-by: Veaceslav Falico <vfalico@redhat•com>
>>> ---
>>>
>>> Notes:
>>> v4 -> v5
>>> No change.
>>>
>>> v3 -> v4:
>>> No change.
>>>
>>> v2 -> v3:
>>> No change.
>>>
>>> v1 -> v2:
>>> No changes.
>>>
>>> RFC -> v1:
>>> New patch.
>>>
>>> drivers/net/bonding/bond_3ad.c | 46 ++++++++++++++++++++----------------------
>>> 1 file changed, 22 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>>> index 3847aee..c861ee7 100644
>>> --- a/drivers/net/bonding/bond_3ad.c
>>> +++ b/drivers/net/bonding/bond_3ad.c
>>> @@ -2417,15 +2417,15 @@ 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, *first_ok_slave;
>>> + struct aggregator *agg;
>>> + struct ad_info ad_info;
>>> struct list_head *iter;
>>> - int slave_agg_no;
>>> int slaves_in_agg;
>>> - int agg_id;
>>> - int i;
>>> - struct ad_info ad_info;
>>> + int slave_agg_no;
>>> int res = 1;
>>> + int agg_id;
>>>
>>> read_lock(&bond->lock);
>>> if (__bond_3ad_get_active_agg_info(bond, &ad_info)) {
>>> @@ -2438,20 +2438,28 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
>>> agg_id = ad_info.aggregator_id;
>>>
>>> if (slaves_in_agg == 0) {
>>> - /*the aggregator is empty*/
>>> pr_debug("%s: Error: active aggregator is empty\n", dev->name);
>>> goto out;
>>> }
>>>
>>> slave_agg_no = bond->xmit_hash_policy(skb, slaves_in_agg);
>>> + first_ok_slave = NULL;
>>>
>>> bond_for_each_slave(bond, slave, iter) {
>>> - struct aggregator *agg = SLAVE_AD_INFO(slave).port.aggregator;
>>> + agg = SLAVE_AD_INFO(slave).port.aggregator;
>>> + if (!agg || agg->aggregator_identifier != agg_id)
>>> + continue;
>>>
>>> - if (agg && (agg->aggregator_identifier == agg_id)) {
>>> + if (slave_agg_no >= 0) {
>>> + if (!first_ok_slave && SLAVE_IS_OK(slave))
>>> + first_ok_slave = slave;
>>> slave_agg_no--;
>>> - if (slave_agg_no < 0)
>>> - break;
>>> + continue;
>>> + }
>
> [1] ^^^^^^^^^^^^^^^^^^^^^^^^^
>
>>> +
>>> + if (SLAVE_IS_OK(slave)) {
>>> + res = bond_dev_queue_xmit(bond, skb, slave->dev);
>>> + goto out;
>>> }
>
> [2] ^^^^^^^^^^^^^^^^^^^^^^^^^
>
>>
>> I think you miss something, you will send skb always by the first suitable port,
>> it could not support load balance.
>
> Well, yes, it will send always by the first suitable port AFTER
> slave_agg_no, returned by the xmit_hash_policy(), which is the whole point
> in the hash function and in the load balance.
>
> It will first loop through the slaves, decrementing slave_agg_no to a
> negative value, while saving the first good to send slave, as shown in [1].
> Notice the "continue;":
>
> if (slave_agg_no >= 0) {
> if (!first_ok_slave && SLAVE_IS_OK(slave))
> first_ok_slave = slave;
> slave_agg_no--;
> continue;
> }
>
yes, I got your opinion, tnanks.
> Once we hit the negative value - which means we've skipped enough slaves,
> as requested by the hash function - we can start looking for the first
> slave that is good to send AFTER those all skipped slaves, as shown in [2].
>
> Down the patch we also use that 'first_ok_slave' - in case we didn't find
> any suitable one after we've skipped first slave_agg_no and till the last
> slave, so it implements the same 'circular' logic as was in
> bond_for_each_slave_from().
>
>> pls consult my function.
>> 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;
>> + }
>> + }
>
> I'll review your function in your patch.
>
>> }
>> }
>>
>>> }
>>>
>>> @@ -2461,20 +2469,10 @@ 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;
>>> - 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)) {
>>> - res = bond_dev_queue_xmit(bond, skb, slave->dev);
>>> - break;
>>> - }
>>> - }
>>> + /* we couldn't find any suitable slave after the agg_no, so use the
>>> + * first suitable found, if found. */
>>> + if (first_ok_slave)
>>> + res = bond_dev_queue_xmit(bond, skb, first_ok_slave->dev);
>>>
>>> out:
>>> read_unlock(&bond->lock);
>>>
>>
>>
>
> .
>
next prev parent reply other threads:[~2013-09-26 6:39 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-09-25 7:20 [PATCH v5 net-next 00/27] bonding: use neighbours instead of own lists Veaceslav Falico
2013-09-25 7:20 ` [PATCH v5 net-next 01/27] net: use lists as arguments instead of bool upper Veaceslav Falico
2013-09-25 7:20 ` [PATCH v5 net-next 02/27] net: add adj_list to save only neighbours Veaceslav Falico
2013-09-25 7:20 ` [PATCH v5 net-next 03/27] net: add RCU variant to search for netdev_adjacent link Veaceslav Falico
2013-09-25 7:20 ` [PATCH v5 net-next 04/27] net: add netdev_adjacent->private and allow to use it Veaceslav Falico
2013-09-25 7:20 ` [PATCH v5 net-next 05/27] bonding: populate neighbour's private on enslave Veaceslav Falico
2013-09-25 7:20 ` [PATCH v5 net-next 06/27] bonding: modify bond_get_slave_by_dev() to use neighbours Veaceslav Falico
2013-09-25 7:20 ` [PATCH v5 net-next 07/27] net: add for_each iterators through neighbour lower link's private Veaceslav Falico
2013-09-25 7:20 ` [PATCH v5 net-next 08/27] bonding: remove bond_for_each_slave_continue_reverse() Veaceslav Falico
2013-09-25 7:20 ` [PATCH v5 net-next 09/27] bonding: make bond_for_each_slave() use lower neighbour's private Veaceslav Falico
2013-09-25 7:20 ` [PATCH v5 net-next 10/27] bonding: use bond_for_each_slave() in bond_uninit() Veaceslav Falico
2013-09-25 7:20 ` [PATCH v5 net-next 11/27] bonding: rework bond_3ad_xmit_xor() to use bond_for_each_slave() only Veaceslav Falico
2013-09-26 2:31 ` Ding Tianhong
2013-09-26 5:43 ` Veaceslav Falico
2013-09-26 6:39 ` Ding Tianhong [this message]
2013-09-25 7:20 ` [PATCH v5 net-next 12/27] bonding: rework rlb_next_rx_slave() to use bond_for_each_slave() Veaceslav Falico
2013-09-25 7:20 ` [PATCH v5 net-next 13/27] bonding: rework bond_find_best_slave() " Veaceslav Falico
2013-09-25 7:20 ` [PATCH v5 net-next 14/27] bonding: rework bond_ab_arp_probe() " Veaceslav Falico
2013-09-25 7:20 ` [PATCH v5 net-next 15/27] bonding: remove unused bond_for_each_slave_from() Veaceslav Falico
2013-09-25 7:20 ` [PATCH v5 net-next 16/27] bonding: add bond_has_slaves() and use it Veaceslav Falico
2013-09-25 7:20 ` [PATCH v5 net-next 17/27] bonding: convert bond_has_slaves() to use the neighbour list Veaceslav Falico
2013-09-25 7:20 ` [PATCH v5 net-next 18/27] net: add a possibility to get private from netdev_adjacent->list Veaceslav Falico
2013-09-25 7:20 ` [PATCH v5 net-next 19/27] bonding: convert first/last slave logic to use neighbours Veaceslav Falico
2013-09-25 7:20 ` [PATCH v5 net-next 20/27] bonding: remove bond_prev_slave() Veaceslav Falico
2013-09-25 7:20 ` [PATCH v5 net-next 21/27] bonding: add __bond_next_slave() which uses neighbours Veaceslav Falico
2013-09-25 7:20 ` [PATCH v5 net-next 22/27] bonding: use neighbours for bond_next_slave() Veaceslav Falico
2013-09-25 7:20 ` [PATCH v5 net-next 23/27] bonding: remove slave lists Veaceslav Falico
2013-09-25 7:20 ` [PATCH v5 net-next 24/27] vlan: link the upper neighbour only after registering Veaceslav Falico
2013-09-25 7:20 ` [PATCH v5 net-next 25/27] vlan: unlink the upper neighbour before unregistering Veaceslav Falico
2013-09-25 7:20 ` [PATCH v5 net-next 26/27] net: expose the master link to sysfs, and remove it from bond Veaceslav Falico
2013-09-25 7:20 ` [PATCH v5 net-next 27/27] net: create sysfs symlinks for neighbour devices Veaceslav Falico
2013-09-26 20:02 ` [PATCH v5 net-next 00/27] bonding: use neighbours instead of own lists David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5243D68F.4090806@huawei.com \
--to=dingtianhong@huawei$(echo .)com \
--cc=andy@greyhouse$(echo .)net \
--cc=fubar@us$(echo .)ibm.com \
--cc=jiri@resnulli$(echo .)us \
--cc=netdev@vger$(echo .)kernel.org \
--cc=vfalico@redhat$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox