On 9/28/2018 11:00 AM, Ferruh Yigit wrote: > On 8/24/2018 3:05 PM, Chas Williams wrote: >> >> >> On Fri, Aug 24, 2018 at 6:39 AM Ferruh Yigit <[email protected] >> <mailto:[email protected]>> wrote: >> >> On 8/23/2018 4:21 PM, Chas Williams wrote: >> > >> > >> > On Thu, Aug 23, 2018 at 9:15 AM Ferruh Yigit <[email protected] >> <mailto:[email protected]> >> > <mailto:[email protected] <mailto:[email protected]>>> wrote: >> > >> > On 8/6/2018 4:50 PM, Chas Williams wrote: >> > > On Sun, Aug 5, 2018 at 5:55 PM Thomas Monjalon >> <[email protected] >> <mailto:[email protected]> >> > <mailto:[email protected] <mailto:[email protected]>>> wrote: >> > > >> > >> 02/08/2018 15:38, Doherty, Declan: >> > >>> On 01/08/2018 2:18 PM, Radu Nicolau wrote: >> > >>>> When a bonding port is stopped also stop and deactivate all >> slaves. >> > >>>> Otherwise slaves will be still listed as active. >> > >>>> >> > >>>> Fixes: 69bce062132b ("net/bonding: do not clear active slave >> count") >> > >>>> Cc: [email protected] <mailto:[email protected]> >> <mailto:[email protected] <mailto:[email protected]>> >> > >>>> >> > >>>> Signed-off-by: Radu Nicolau <[email protected] >> <mailto:[email protected]> >> > <mailto:[email protected] <mailto:[email protected]>>> >> > >>> >> > >>> Acked-by: Declan Doherty <[email protected] >> <mailto:[email protected]> >> > <mailto:[email protected] >> <mailto:[email protected]>>> >> > >> >> > >> Waiting for opinion from the other bonding maintainer (Chas) >> > >> who started to review and has some doubts. >> > >> >> > > >> > > The slaves being listed as active is not a bug. If the slaves >> are not >> > > deactivated, then they should be considered activated. >> Previously, >> > > stopping the bonding PMD just reset the active slave count. >> That's >> > > not the right way to deactivate slaves. This was fixed by >> 69bce062132b. >> > > >> > > This patch is new behavior of explicitly deactivating the slaves >> when >> > > the bonding PMD is stopped. >> > > >> > > As I mentioned, I think this makes life difficult for those of >> us using >> > > an external state machine. However, that should probably be >> fixed >> > > differently then. >> > > >> > > >> > >> >> > >> Chas, please do you agree with Declan's ack? >> > >> >> > >> >> > >> >> > > Change the Fixes line. >> > >> > Hi Chas, >> > >> > Are you OK with the rest of the patch if Fixes line fixed? >> > If already have a proposed fixes line I can fix it while merging. >> > >> > >> > Yes, the rest of the patch is fine as long as the Fixes is correct. >> > Try this: >> > >> > Fixes: 2efb58cbab6e ("bond: new link bonding library") >> > >> > And it's really new behavior. Perhaps Fixes: isn't quite right. >> > The current code works fine with activated slaves existing outside >> > of the stop/star. >> >> From your description dropping Fixes line seems OK, but it will effect >> if the >> patch backported or not. >> >> >> Then dont' drop the Fixes line. See below. >> >> >> Isn't it clear from bonding requirement what should slave ports' status >> be when >> bonding port it stopped? >> >> >> I am guessing the author's original intent was to deactivate all the >> slaves because he reset the activate slave count to 0. However, that >> didn't actually deactivate a slave and later activating an already >> active slave after another start would result in some odd failures. > > Yes, intention seems as you said, there is an unit test that expects active > slave count to be 0 after bond port stopped. > Your change to remove setting active slave count to zero on bond stop causes > that test fail. > To fix the unit test, all slaves stopped and deactivated. > > I think it is still clear if that expectation from unit test coming from > bonding > requirement and development assumption. But will get the patch, with updated > fixes line, to fix the unit test.
it is still _not_ clear ... bonding requirement _or_ development assumption. ... > >> >> In a more existential sense, what does active mean? In the case of >> 802.3ad, a slave is potentially active until the protocol says otherwise >> which means a timeout. So the stopped/started aspect of the port >> isn't necessarily a concern. If you are just briefly stopping a >> port to reconfigure, perhaps you don't want to renegotiate the >> 802.3ad state. Of course, if you reconfigure a port, there is a >> good chance you want to renegotiate with the other end anyway. >> >> TL;DR -- fine with this patch. Add new Fixes line so backported. >> >> >> >> >
