* [PATCH net-next 1/3] bonding: Set the correct value to fail_over_mac at enslavement
@ 2014-01-22 9:22 Ding Tianhong
2014-01-22 14:20 ` Nikolay Aleksandrov
2014-01-22 20:51 ` Jay Vosburgh
0 siblings, 2 replies; 5+ messages in thread
From: Ding Tianhong @ 2014-01-22 9:22 UTC (permalink / raw)
To: Jay Vosburgh, Veaceslav Falico, David S. Miller, Netdev,
Andy Gospodarek
If the new slave don't support setting the MAC address, there are
two ways to handle this situation:
1). If the new slave is the first slave, set bond to the new slave's
MAC address, if the mode is active-backup, set fail_over_mac to
active, otherwise set fail_over_mac to none.
2). If the new slave is not the first slave and the fail_over_mac is
active, it means that the slave could work normally in active-backup
mode, otherwise if the fail_over_mac is none, the slave could not
work normally for no active-backup mode, so bond could not ensalve
the new dev.
Cc: Jay Vosburgh <fubar@us.ibm.com>
Cc: Veaceslav Falico <vfalico@redhat.com>
Cc: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
---
drivers/net/bonding/bond_main.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 3220b48..598f100 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1334,9 +1334,17 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
if (slave_ops->ndo_set_mac_address == NULL) {
if (!bond_has_slaves(bond)) {
- pr_warning("%s: Warning: The first slave device specified does not support setting the MAC address. Setting fail_over_mac to active.",
+ pr_warning("%s: Warning: The first slave device specified does not support setting the MAC address.\n",
bond_dev->name);
- bond->params.fail_over_mac = BOND_FOM_ACTIVE;
+ if (bond->params.mode == BOND_MODE_ACTIVEBACKUP) {
+ bond->params.fail_over_mac = BOND_FOM_ACTIVE;
+ pr_warning("%s: Setting fail_over_mac to active for active-backup mode.\n",
+ bond_dev->name);
+ } else {
+ bond->params.fail_over_mac = BOND_FOM_NONE;
+ pr_warning("%s: Setting fail_over_mac to none for no active-backup modes",
+ bond_dev->name);
+ }
} else if (bond->params.fail_over_mac != BOND_FOM_ACTIVE) {
pr_err("%s: Error: The slave device specified does not support setting the MAC address, but fail_over_mac is not set to active.\n",
bond_dev->name);
--
1.8.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net-next 1/3] bonding: Set the correct value to fail_over_mac at enslavement
2014-01-22 9:22 [PATCH net-next 1/3] bonding: Set the correct value to fail_over_mac at enslavement Ding Tianhong
@ 2014-01-22 14:20 ` Nikolay Aleksandrov
2014-01-23 2:31 ` Ding Tianhong
2014-01-22 20:51 ` Jay Vosburgh
1 sibling, 1 reply; 5+ messages in thread
From: Nikolay Aleksandrov @ 2014-01-22 14:20 UTC (permalink / raw)
To: Ding Tianhong, Jay Vosburgh, Veaceslav Falico, David S. Miller,
Netdev, Andy Gospodarek
On 01/22/2014 10:22 AM, Ding Tianhong wrote:
> If the new slave don't support setting the MAC address, there are
> two ways to handle this situation:
>
> 1). If the new slave is the first slave, set bond to the new slave's
> MAC address, if the mode is active-backup, set fail_over_mac to
> active, otherwise set fail_over_mac to none.
> 2). If the new slave is not the first slave and the fail_over_mac is
> active, it means that the slave could work normally in active-backup
> mode, otherwise if the fail_over_mac is none, the slave could not
> work normally for no active-backup mode, so bond could not ensalve
> the new dev.
>
> Cc: Jay Vosburgh <fubar@us.ibm.com>
> Cc: Veaceslav Falico <vfalico@redhat.com>
> Cc: Andy Gospodarek <andy@greyhouse.net>
> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
> ---
> drivers/net/bonding/bond_main.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 3220b48..598f100 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1334,9 +1334,17 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>
> if (slave_ops->ndo_set_mac_address == NULL) {
> if (!bond_has_slaves(bond)) {
> - pr_warning("%s: Warning: The first slave device specified does not support setting the MAC address. Setting fail_over_mac to active.",
> + pr_warning("%s: Warning: The first slave device specified does not support setting the MAC address.\n",
> bond_dev->name);
> - bond->params.fail_over_mac = BOND_FOM_ACTIVE;
> + if (bond->params.mode == BOND_MODE_ACTIVEBACKUP) {
> + bond->params.fail_over_mac = BOND_FOM_ACTIVE;
> + pr_warning("%s: Setting fail_over_mac to active for active-backup mode.\n",
> + bond_dev->name);
> + } else {
> + bond->params.fail_over_mac = BOND_FOM_NONE;
> + pr_warning("%s: Setting fail_over_mac to none for no active-backup modes",
At least make the warnings consistent: "... for no active-backup mode.\n".
Also you might consider switching to pr_warn.
> + bond_dev->name);
> + }
> } else if (bond->params.fail_over_mac != BOND_FOM_ACTIVE) {
> pr_err("%s: Error: The slave device specified does not support setting the MAC address, but fail_over_mac is not set to active.\n",
> bond_dev->name);
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next 1/3] bonding: Set the correct value to fail_over_mac at enslavement
2014-01-22 9:22 [PATCH net-next 1/3] bonding: Set the correct value to fail_over_mac at enslavement Ding Tianhong
2014-01-22 14:20 ` Nikolay Aleksandrov
@ 2014-01-22 20:51 ` Jay Vosburgh
2014-01-23 2:50 ` Ding Tianhong
1 sibling, 1 reply; 5+ messages in thread
From: Jay Vosburgh @ 2014-01-22 20:51 UTC (permalink / raw)
To: Ding Tianhong; +Cc: Veaceslav Falico, David S. Miller, Netdev, Andy Gospodarek
Ding Tianhong <dingtianhong@huawei.com> wrote:
>If the new slave don't support setting the MAC address, there are
>two ways to handle this situation:
>
>1). If the new slave is the first slave, set bond to the new slave's
> MAC address, if the mode is active-backup, set fail_over_mac to
> active, otherwise set fail_over_mac to none.
This should be "if the mode is active-backup, set fail_over_mac
to active, otherwise do not change fail_over_mac." Setting to none here
would undo any setting of fail_over_mac that the user had set prior to
adding the first slave.
>2). If the new slave is not the first slave and the fail_over_mac is
> active, it means that the slave could work normally in active-backup
> mode, otherwise if the fail_over_mac is none, the slave could not
> work normally for no active-backup mode, so bond could not ensalve
> the new dev.
This (#2) is not a code change, correct? You're just restating
the existing behavior of the code, right?
Also, I don't see where this patch set updates the slave removal
processing where the slave's original MAC is restored. At present, this
is done by a test against fail_over_mac, but should be tested against
the mode and fail_over_mac.
My comment to the prior version of this patchset, again:
The correct way to fix this in general is to permit setting an
option at any time, but only have it take effect in active-backup mode.
This minimizes ordering requirements when setting options.
I would instead modify the bond enslave and removal processing
to check the mode in addition to fail_over_mac when setting a slave's
MAC during enslavement. The change active slave processing already only
calls the fail_over_mac function when in active-backup mode. This
should also be a simpler change set.
These comments still apply to this version of the patchset.
-J
>Cc: Jay Vosburgh <fubar@us.ibm.com>
>Cc: Veaceslav Falico <vfalico@redhat.com>
>Cc: Andy Gospodarek <andy@greyhouse.net>
>Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>---
> drivers/net/bonding/bond_main.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 3220b48..598f100 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -1334,9 +1334,17 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>
> if (slave_ops->ndo_set_mac_address == NULL) {
> if (!bond_has_slaves(bond)) {
>- pr_warning("%s: Warning: The first slave device specified does not support setting the MAC address. Setting fail_over_mac to active.",
>+ pr_warning("%s: Warning: The first slave device specified does not support setting the MAC address.\n",
> bond_dev->name);
>- bond->params.fail_over_mac = BOND_FOM_ACTIVE;
>+ if (bond->params.mode == BOND_MODE_ACTIVEBACKUP) {
>+ bond->params.fail_over_mac = BOND_FOM_ACTIVE;
>+ pr_warning("%s: Setting fail_over_mac to active for active-backup mode.\n",
>+ bond_dev->name);
>+ } else {
>+ bond->params.fail_over_mac = BOND_FOM_NONE;
>+ pr_warning("%s: Setting fail_over_mac to none for no active-backup modes",
>+ bond_dev->name);
>+ }
> } else if (bond->params.fail_over_mac != BOND_FOM_ACTIVE) {
> pr_err("%s: Error: The slave device specified does not support setting the MAC address, but fail_over_mac is not set to active.\n",
> bond_dev->name);
>--
>1.8.0
---
-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next 1/3] bonding: Set the correct value to fail_over_mac at enslavement
2014-01-22 14:20 ` Nikolay Aleksandrov
@ 2014-01-23 2:31 ` Ding Tianhong
0 siblings, 0 replies; 5+ messages in thread
From: Ding Tianhong @ 2014-01-23 2:31 UTC (permalink / raw)
To: Nikolay Aleksandrov, Jay Vosburgh, Veaceslav Falico,
David S. Miller, Netdev, Andy Gospodarek
On 2014/1/22 22:20, Nikolay Aleksandrov wrote:
> On 01/22/2014 10:22 AM, Ding Tianhong wrote:
>> If the new slave don't support setting the MAC address, there are
>> two ways to handle this situation:
>>
>> 1). If the new slave is the first slave, set bond to the new slave's
>> MAC address, if the mode is active-backup, set fail_over_mac to
>> active, otherwise set fail_over_mac to none.
>> 2). If the new slave is not the first slave and the fail_over_mac is
>> active, it means that the slave could work normally in active-backup
>> mode, otherwise if the fail_over_mac is none, the slave could not
>> work normally for no active-backup mode, so bond could not ensalve
>> the new dev.
>>
>> Cc: Jay Vosburgh <fubar@us.ibm.com>
>> Cc: Veaceslav Falico <vfalico@redhat.com>
>> Cc: Andy Gospodarek <andy@greyhouse.net>
>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>> ---
>> drivers/net/bonding/bond_main.c | 12 ++++++++++--
>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 3220b48..598f100 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -1334,9 +1334,17 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>>
>> if (slave_ops->ndo_set_mac_address == NULL) {
>> if (!bond_has_slaves(bond)) {
>> - pr_warning("%s: Warning: The first slave device specified does not support setting the MAC address. Setting fail_over_mac to active.",
>> + pr_warning("%s: Warning: The first slave device specified does not support setting the MAC address.\n",
>> bond_dev->name);
>> - bond->params.fail_over_mac = BOND_FOM_ACTIVE;
>> + if (bond->params.mode == BOND_MODE_ACTIVEBACKUP) {
>> + bond->params.fail_over_mac = BOND_FOM_ACTIVE;
>> + pr_warning("%s: Setting fail_over_mac to active for active-backup mode.\n",
>> + bond_dev->name);
>> + } else {
>> + bond->params.fail_over_mac = BOND_FOM_NONE;
>> + pr_warning("%s: Setting fail_over_mac to none for no active-backup modes",
> At least make the warnings consistent: "... for no active-backup mode.\n".
> Also you might consider switching to pr_warn.
>
ok, thanks.
>> + bond_dev->name);
>> + }
>> } else if (bond->params.fail_over_mac != BOND_FOM_ACTIVE) {
>> pr_err("%s: Error: The slave device specified does not support setting the MAC address, but fail_over_mac is not set to active.\n",
>> bond_dev->name);
>>
>
>
> .
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next 1/3] bonding: Set the correct value to fail_over_mac at enslavement
2014-01-22 20:51 ` Jay Vosburgh
@ 2014-01-23 2:50 ` Ding Tianhong
0 siblings, 0 replies; 5+ messages in thread
From: Ding Tianhong @ 2014-01-23 2:50 UTC (permalink / raw)
To: Jay Vosburgh; +Cc: Veaceslav Falico, David S. Miller, Netdev, Andy Gospodarek
On 2014/1/23 4:51, Jay Vosburgh wrote:
> Ding Tianhong <dingtianhong@huawei.com> wrote:
>
>> If the new slave don't support setting the MAC address, there are
>> two ways to handle this situation:
>>
>> 1). If the new slave is the first slave, set bond to the new slave's
>> MAC address, if the mode is active-backup, set fail_over_mac to
>> active, otherwise set fail_over_mac to none.
>
> This should be "if the mode is active-backup, set fail_over_mac
> to active, otherwise do not change fail_over_mac." Setting to none here
> would undo any setting of fail_over_mac that the user had set prior to
> adding the first slave.
>
I thought about this question for a long time, I still can not think clearly.
ex: the first slave did not support setting MAC, and mode is RR, fail_over_mac=1(user set it
when load the driver), so the after the enslavement, the fail_over_mac is still 1,
then the second slave added, if the second slave did not support setting MAC, it will pass
the check and go ahead, until the dev_set_mac_address(), and return error, so
I think set it to none may cause the second slave return early, but no substance logic change.
I will modify this place as your opinion.
>> 2). If the new slave is not the first slave and the fail_over_mac is
>> active, it means that the slave could work normally in active-backup
>> mode, otherwise if the fail_over_mac is none, the slave could not
>> work normally for no active-backup mode, so bond could not ensalve
>> the new dev.
>
> This (#2) is not a code change, correct? You're just restating
> the existing behavior of the code, right?
>
> Also, I don't see where this patch set updates the slave removal
> processing where the slave's original MAC is restored. At present, this
> is done by a test against fail_over_mac, but should be tested against
> the mode and fail_over_mac.
>
> My comment to the prior version of this patchset, again:
>
> The correct way to fix this in general is to permit setting an
> option at any time, but only have it take effect in active-backup mode.
> This minimizes ordering requirements when setting options.
>
> I would instead modify the bond enslave and removal processing
> to check the mode in addition to fail_over_mac when setting a slave's
> MAC during enslavement. The change active slave processing already only
> calls the fail_over_mac function when in active-backup mode. This
> should also be a simpler change set.
>
> These comments still apply to this version of the patchset.
>
> -J
>
OK.
Regards
Ding
>> Cc: Jay Vosburgh <fubar@us.ibm.com>
>> Cc: Veaceslav Falico <vfalico@redhat.com>
>> Cc: Andy Gospodarek <andy@greyhouse.net>
>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>> ---
>> drivers/net/bonding/bond_main.c | 12 ++++++++++--
>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 3220b48..598f100 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -1334,9 +1334,17 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>>
>> if (slave_ops->ndo_set_mac_address == NULL) {
>> if (!bond_has_slaves(bond)) {
>> - pr_warning("%s: Warning: The first slave device specified does not support setting the MAC address. Setting fail_over_mac to active.",
>> + pr_warning("%s: Warning: The first slave device specified does not support setting the MAC address.\n",
>> bond_dev->name);
>> - bond->params.fail_over_mac = BOND_FOM_ACTIVE;
>> + if (bond->params.mode == BOND_MODE_ACTIVEBACKUP) {
>> + bond->params.fail_over_mac = BOND_FOM_ACTIVE;
>> + pr_warning("%s: Setting fail_over_mac to active for active-backup mode.\n",
>> + bond_dev->name);
>> + } else {
>> + bond->params.fail_over_mac = BOND_FOM_NONE;
>> + pr_warning("%s: Setting fail_over_mac to none for no active-backup modes",
>> + bond_dev->name);
>> + }
>> } else if (bond->params.fail_over_mac != BOND_FOM_ACTIVE) {
>> pr_err("%s: Error: The slave device specified does not support setting the MAC address, but fail_over_mac is not set to active.\n",
>> bond_dev->name);
>> --
>> 1.8.0
>
> ---
> -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
>
>
> .
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-01-23 2:50 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-01-22 9:22 [PATCH net-next 1/3] bonding: Set the correct value to fail_over_mac at enslavement Ding Tianhong
2014-01-22 14:20 ` Nikolay Aleksandrov
2014-01-23 2:31 ` Ding Tianhong
2014-01-22 20:51 ` Jay Vosburgh
2014-01-23 2:50 ` Ding Tianhong
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).