| Message ID | 994d2dc994a801fc3aae7aa912266b09ea45d95c.1378489255.git.mleitner@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 09/06/2013 07:41 PM, Marcelo Ricardo Leitner wrote: > Hi Dave, > > Please queue this one for -stable trees too. > > Thanks. > > ---8<--- > > As bond_store_arp_validation and bond_store_arp_interval doesn't > deal with each other, we can't chain both at bond_open, otherwise > we are open to this de-sync state, steps in sequence: > - bond is down > - arp_interval = 0 > - arp_validate = 1 or 2 > - bond is up > - arp_interval = 1 > > This would lead to bond issuing ARP requests but never listening > to the replies, because the tap wasn't attached. After reaching > this state, while bond is up, setting arp_validate won't help > as it only acts if needed. > --- > drivers/net/bonding/bond_main.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > index 39e5b1c..805d098 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -3180,11 +3180,10 @@ static int bond_open(struct net_device *bond_dev) > if (bond->params.miimon) /* link check interval, in milliseconds. */ > queue_delayed_work(bond->wq, &bond->mii_work, 0); > > - if (bond->params.arp_interval) { /* arp interval, in milliseconds. */ > + if (bond->params.arp_interval) /* arp interval, in milliseconds. */ > queue_delayed_work(bond->wq, &bond->arp_work, 0); > - if (bond->params.arp_validate) > - bond->recv_probe = bond_arp_rcv; > - } > + if (bond->params.arp_validate) > + bond->recv_probe = bond_arp_rcv; > > if (bond->params.mode == BOND_MODE_8023AD) { > queue_delayed_work(bond->wq, &bond->ad_work, 0); > Thanks for fixing this. Acked-by: Nikolay Aleksandrov <nikolay@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Marcelo Ricardo Leitner <mleitner@redhat.com> wrote: >Hi Dave, > >Please queue this one for -stable trees too. > >Thanks. > >---8<--- > >As bond_store_arp_validation and bond_store_arp_interval doesn't >deal with each other, we can't chain both at bond_open, otherwise >we are open to this de-sync state, steps in sequence: >- bond is down >- arp_interval = 0 >- arp_validate = 1 or 2 >- bond is up >- arp_interval = 1 > >This would lead to bond issuing ARP requests but never listening >to the replies, because the tap wasn't attached. After reaching >this state, while bond is up, setting arp_validate won't help >as it only acts if needed. You need to sign off you patch. > drivers/net/bonding/bond_main.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >index 39e5b1c..805d098 100644 >--- a/drivers/net/bonding/bond_main.c >+++ b/drivers/net/bonding/bond_main.c >@@ -3180,11 +3180,10 @@ static int bond_open(struct net_device *bond_dev) > if (bond->params.miimon) /* link check interval, in milliseconds. */ > queue_delayed_work(bond->wq, &bond->mii_work, 0); > >- if (bond->params.arp_interval) { /* arp interval, in milliseconds. */ >+ if (bond->params.arp_interval) /* arp interval, in milliseconds. */ > queue_delayed_work(bond->wq, &bond->arp_work, 0); >- if (bond->params.arp_validate) >- bond->recv_probe = bond_arp_rcv; >- } >+ if (bond->params.arp_validate) >+ bond->recv_probe = bond_arp_rcv; Won't this set the recv_probe when arp_interval is 0 (disabled)? That's going to make bonding look at a bunch of traffic it's not going to do anything with. Why is this better than having bonding_store_arp_interval set recv_probe if arp_validate is set when it queues the arp_work? And, presumably, clear the recv_probe if arp_interval is being cleared. -J > if (bond->params.mode == BOND_MODE_8023AD) { > queue_delayed_work(bond->wq, &bond->ad_work, 0); >-- >1.8.3.1 > --- -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Marcelo Ricardo Leitner <mleitner@redhat.com> Date: Fri, 6 Sep 2013 14:41:53 -0300 > Please queue this one for -stable trees too. This is very much not the correct way to submit a patch. First of all, you haven't signed off on the patch. Second of all, any supplemental information that doesn't belong in the commit message needs to go after a "---" dileanator line. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em 06-09-2013 14:58, David Miller escreveu: > From: Marcelo Ricardo Leitner <mleitner@redhat.com> > Date: Fri, 6 Sep 2013 14:41:53 -0300 > >> Please queue this one for -stable trees too. > > This is very much not the correct way to submit a patch. > > First of all, you haven't signed off on the patch. > > Second of all, any supplemental information that doesn't belong in the > commit message needs to go after a "---" dileanator line. Okay, sorry for that. I'll resubmit as soon as I work on Jay's comments. Thanks Marcelo -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em 06-09-2013 14:56, Jay Vosburgh escreveu: > Marcelo Ricardo Leitner <mleitner@redhat.com> wrote: > >> Hi Dave, >> >> Please queue this one for -stable trees too. >> >> Thanks. >> >> ---8<--- >> >> As bond_store_arp_validation and bond_store_arp_interval doesn't >> deal with each other, we can't chain both at bond_open, otherwise >> we are open to this de-sync state, steps in sequence: >> - bond is down >> - arp_interval = 0 >> - arp_validate = 1 or 2 >> - bond is up >> - arp_interval = 1 >> >> This would lead to bond issuing ARP requests but never listening >> to the replies, because the tap wasn't attached. After reaching >> this state, while bond is up, setting arp_validate won't help >> as it only acts if needed. > > You need to sign off you patch. My bad, sorry >> drivers/net/bonding/bond_main.c | 7 +++---- >> 1 file changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >> index 39e5b1c..805d098 100644 >> --- a/drivers/net/bonding/bond_main.c >> +++ b/drivers/net/bonding/bond_main.c >> @@ -3180,11 +3180,10 @@ static int bond_open(struct net_device *bond_dev) >> if (bond->params.miimon) /* link check interval, in milliseconds. */ >> queue_delayed_work(bond->wq, &bond->mii_work, 0); >> >> - if (bond->params.arp_interval) { /* arp interval, in milliseconds. */ >> + if (bond->params.arp_interval) /* arp interval, in milliseconds. */ >> queue_delayed_work(bond->wq, &bond->arp_work, 0); >> - if (bond->params.arp_validate) >> - bond->recv_probe = bond_arp_rcv; >> - } >> + if (bond->params.arp_validate) >> + bond->recv_probe = bond_arp_rcv; > > Won't this set the recv_probe when arp_interval is 0 (disabled)? > That's going to make bonding look at a bunch of traffic it's not going > to do anything with. Yes. > Why is this better than having bonding_store_arp_interval set > recv_probe if arp_validate is set when it queues the arp_work? And, > presumably, clear the recv_probe if arp_interval is being cleared. > > -J Yeah, no good. You're right. Self-NACK here. Nikolay will submit a better patch later. Thanks, Marcelo >> if (bond->params.mode == BOND_MODE_8023AD) { >> queue_delayed_work(bond->wq, &bond->ad_work, 0); >> -- >> 1.8.3.1 >> > > --- > -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com > > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 39e5b1c..805d098 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -3180,11 +3180,10 @@ static int bond_open(struct net_device *bond_dev) if (bond->params.miimon) /* link check interval, in milliseconds. */ queue_delayed_work(bond->wq, &bond->mii_work, 0); - if (bond->params.arp_interval) { /* arp interval, in milliseconds. */ + if (bond->params.arp_interval) /* arp interval, in milliseconds. */ queue_delayed_work(bond->wq, &bond->arp_work, 0); - if (bond->params.arp_validate) - bond->recv_probe = bond_arp_rcv; - } + if (bond->params.arp_validate) + bond->recv_probe = bond_arp_rcv; if (bond->params.mode == BOND_MODE_8023AD) { queue_delayed_work(bond->wq, &bond->ad_work, 0);