Message-ID: <CAJYOGF_jq8b=FS6+8gCkbELw-A-2WtjkdjHU3-Rt+L1YBFtGgQ@mail.gmail.com>
Date:   Fri, 27 Sep 2019 14:14:22 +0300
From:   Aleksei Zakharov <zaharov@...ectel.ru>
To:     Jay Vosburgh <jay.vosburgh@...onical.com>
Cc:     netdev@...r.kernel.org, "zhangsha (A)" <zhangsha.zhang@...wei.com>
Subject: Re: Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race

чт, 26 сент. 2019 г. в 23:01, Jay Vosburgh <jay.vosburgh@...onical.com>:
>
> Aleksei Zakharov <zaharov@...ectel.ru> wrote:
>
> >чт, 26 сент. 2019 г. в 07:38, Jay Vosburgh <jay.vosburgh@...onical.com>:
> >>
> >> Aleksei Zakharov <zaharov@...ectel.ru> wrote:
> >>
> >> >ср, 25 сент. 2019 г. в 03:31, Jay Vosburgh <jay.vosburgh@...onical.com>:
> >> >>
> >> >> Алексей Захаров wrote:
> >> >> [...]
> >> >> >Right after reboot one of the slaves hangs with actor port state 71
> >> >> >and partner port state 1.
> >> >> >It doesn't send lacpdu and seems to be broken.
> >> >> >Setting link down and up again fixes slave state.
> >> >> [...]
> >> >>
> >> >>         I think I see what failed in the first patch, could you test the
> >> >> following patch?  This one is for net-next, so you'd need to again swap
> >> >> slave_err / netdev_err for the Ubuntu 4.15 kernel.
> >> >>
> >> >I've tested new patch. It seems to work. I can't reproduce the bug
> >> >with this patch.
> >> >There are two types of messages when link becomes up:
> >> >First:
> >> >bond-san: EVENT 1 llu 4294895911 slave eth2
> >> >8021q: adding VLAN 0 to HW filter on device eth2
> >> >bond-san: link status definitely down for interface eth2, disabling it
> >> >mlx4_en: eth2: Link Up
> >> >bond-san: EVENT 4 llu 4294895911 slave eth2
> >> >bond-san: link status up for interface eth2, enabling it in 500 ms
> >> >bond-san: invalid new link 3 on slave eth2
> >> >bond-san: link status definitely up for interface eth2, 10000 Mbps full duplex
> >> >Second:
> >> >bond-san: EVENT 1 llu 4295147594 slave eth2
> >> >8021q: adding VLAN 0 to HW filter on device eth2
> >> >mlx4_en: eth2: Link Up
> >> >bond-san: EVENT 4 llu 4295147594 slave eth2
> >> >bond-san: link status up again after 0 ms for interface eth2
> >> >bond-san: link status definitely up for interface eth2, 10000 Mbps full duplex
> >> > [...]
> >>
> >>         The "invalid new link" is appearing because bond_miimon_commit
> >> is being asked to commit a new state that isn't UP or DOWN (3 is
> >> BOND_LINK_BACK).  I looked through the patched code today, and I don't
> >> see a way to get to that message with the new link set to 3, so I'll add
> >> some instrumentation and send out another patch to figure out what's
> >> going on, as that shouldn't happen.
> >>
> >>         I don't see the "invalid" message testing locally, I think
> >> because my network device doesn't transition to carrier up as quickly as
> >> yours.  I thought you were getting BOND_LINK_BACK passed through from
> >> bond_enslave (which calls bond_set_slave_link_state, which will set
> >> link_new_link to BOND_LINK_BACK and leave it there), but the
> >> link_new_link is reset first thing in bond_miimon_inspect, so I'm not
> >> sure how it gets into bond_miimon_commit (I'm thinking perhaps a
> >> concurrent commit triggered by another slave, which then picks up this
> >> proposed link state change by happenstance).
> >I assume that "invalid new link" happens in this way:
> >Interface goes up
> >NETDEV_CHANGE event occurs
> >bond_update_speed_duplex fails
> >and slave->last_link_up returns true
> >slave->link becomes BOND_LINK_FAIL
> >bond_check_dev_link returns 0
> >miimon proposes slave->link_new_state BOND_LINK_DOWN
> >NETDEV_UP event occurs
> >miimon sets commit++
> >miimon proposes slave->link_new_state BOND_LINK_BACK
> >miimon sets slave->link to BOND_LINK_BACK
>
>         I removed the "proposes link_new_state BOND_LINK_BACK" from the
> second test patch and replaced it with the slave->link = BOND_LINK_BACK.
> This particular place in the code also does not do commit++.  If you
> have both of those in the code you're running, then perhaps you have a
> merge error or some such.
You are right, it was a merge issue.
I re-applied the patch and now it works without any error messages.
As usual, there are two types of messages.
This one is less often:
bond-san: EVENT 1 llu 4295238188 slave eth2
8021q: adding VLAN 0 to HW filter on device eth2
mlx4_en: eth2: Link Up
bond-san: EVENT 4 llu 4295238188 slave eth2
bond-san: link status up again after 0 ms for interface eth2
bond-san: link status definitely up for interface eth2, 10000 Mbps full duplex

And this one is more often:
bond-san: EVENT 1 llu 4295242465 slave eth2
8021q: adding VLAN 0 to HW filter on device eth2
bond-san: link status definitely down for interface eth2, disabling it
mlx4_en: eth2: Link Up
bond-san: EVENT 4 llu 4295242465 slave eth2
bond-san: link status up for interface eth2, enabling it in 500 ms
bond-san: link status definitely up for interface eth2, 10000 Mbps full duplex

Bonding works as expected in both cases.

>
> [...]
> >
> >I don't understand a few things here:
> >How could a link be in a different state from time to time during the
> >first NETDEV_* event?
>
>         I'm not sure; possibly a race between the events in the kernel
> and how long it takes for the hardware to establish Ethernet link up.
>
> >And why slave->last_link_up is set when the first NETDEV event occurs?
>
>         slave->last_link_up can be set at enslave time if the carrier
> state of the slave (and thus the initial slave->link) is in a not-down
> state.  There are some paths as well for modes that have an "active"
> slave, but 802.3ad is not one of those.
Thanks for the explanation!

-- 
Best Regards,
Aleksei Zakharov
System administrator

Powered by blists - more mailing lists