Date: Fri, 28 Jun 2024 16:36:17 -0700
From: Jay Vosburgh <jay.vosburgh@...onical.com>
To: Hangbin Liu <liuhangbin@...il.com>
cc: Nikolay Aleksandrov <razor@...ckwall.org>,
    Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
    Andy Gospodarek <andy@...yhouse.net>,
    "David S. Miller" <davem@...emloft.net>,
    Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
    Ido Schimmel <idosch@...dia.com>, Jiri Pirko <jiri@...nulli.us>,
    Amit Cohen <amcohen@...dia.com>
Subject: Re: [PATCHv3 net-next] bonding: 3ad: send ifinfo notify when mux
 state changed

Hangbin Liu <liuhangbin@...il.com> wrote:

>Hi Nikolay,
>On Fri, Jun 28, 2024 at 10:22:25AM +0300, Nikolay Aleksandrov wrote:
>> > Actually I was talking about:
>> >  /sys/class/net/<bond port>/bonding_slave/ad_actor_oper_port_state
>> >  /sys/class/net/<bond port>/bonding_slave/ad_partner_oper_port_state
>> > etc
>> > 
>> > Wouldn't these work for you?
>> > 
>> 
>> But it gets much more complicated, I guess it will be easier to read the
>> proc bond file with all the LACP information. That is under RCU only as
>> well.
>
>Good question. The monitor application want a more elegant/general way
>to deal with the LACP state and do other network reconfigurations.
>Here is the requirement I got from customer.
>
>1) As a server administrator, I want ip monitor to show state change events
>   related to LACP bonds so that I can react quickly to network reconfigurations.
>2) As a network monitoring application developer, I want my application to be
>   notified about LACP bond operational state changes without having to
>   poll /proc/net/bonding/<bond> and parse its output so that it can trigger
>   predefined failover remediation policies.
>3) As a server administrator, I want my LACP bond monitoring application to
>   receive a Netlink-based notification whenever the number of member
>   interfaces is reduced so that the operations support system can provision
>   a member interface replacement.

	What notifications are they not getting that they want?  Or is
it that events happen but lack some information they want?

	Currently, the end of bond_3ad_state_machine_handler() will send
notifications via bond_slave_state_notify() if the state of any member
of the bond has changed (via the struct slave.should_notify field).

	The notifications here come from bond_lower_state_changed(),
which propagates link up/down and tx_enabled (active-ness), but not any
LACP specifics.  These are sent as NETDEV_CHANGELOWERSTATE events, which
rtnetlink_event() handles, so they should be visible to ip monitor.

>What I understand is the user/admin need to know the latest stable state so
>they can do some other network configuration based on the status. Losing
>a middle state notification during fast changes is acceptable.

	This is a much simpler problem to solve.

>> Well, you mentioned administrators want to see the state changes, please
>> better clarify the exact end goal. Note that technically may even not be
>> the last state as the state change itself happens in parallel (different
>> locks) and any update could be delayed depending on rtnl availability
>> and workqueue re-scheduling. But sure, they will get some update at some point. :)
>
>Would you please help explain why we may not get the latest state? From what
>I understand:
>
>1) State A -> B, queue notify
>       rtnl_trylock, fail, queue again
>2) State B -> C, queue notify
>      rtnl_trylock, success, post current state C
>3) State C -> D, queue notify
>      rtnl_trylock, fail, queue again
>4) State D -> A, queue notify
>      rtnl_trylock, fail, queue again
>      rtnl_trylock, fail, queue again
>      rtnl_trylock, success, post current state A
>
>So how could the step 3) state send but step 4) state not send?

	I'm going to speculate here that the scenario envisioned would
be something like CPU A is in the midst of generating an event at the
end of the state machine, but CPU B could be processing a LACPDU
simultaneously-ish.  The CPU A event is sent, but the CPU B event is
delayed due to RTNL contention.  In this scenario, the last event seen
in user space is CPU A, but the actual state has moved on to that set by
CPU B, whose notification will be received eventually.

>BTW, in my code, I should set the should_notify_lacp = 0 first before sending
>ifinfo message. So that even the should_notify_lacp = 1 in ad_mux_machine()
>is over written here, it still send the latest status.
>
>> +
>> +             if (slave->should_notify_lacp) {
>> +                     slave->should_notify_lacp = 0;
>> +                     rtmsg_ifinfo(RTM_NEWLINK, slave->dev, 0, GFP_KERNEL, 0, NULL);
>> +             }
>
>The side effect is that we may send 2 same latest lacp status(the
>should_notify_lacp is over written to 1 and queue again), which should
>be OK.

	Looking at the current notifications in bonding, I wonder if it
would be sufficient to add the desired information to what
bond_lower_state_changed() sends, rather than trying to shoehorn in
another rtnl_trylock() gizmo.

	-J

---
	-Jay Vosburgh, jay.vosburgh@...onical.com

Powered by blists - more mailing lists