Re: [PATCH net-next] bonding: 3ad: send rtnl ifinfo notify when mux state changed

[Nikolay Aleksandrov <razor@blackwall.org>]

On 6/20/24 12:08, Hangbin Liu wrote:
> Hi Nikolay,
> On Thu, Jun 20, 2024 at 11:47:46AM +0300, Nikolay Aleksandrov wrote:
>> On 6/20/24 09:10, Hangbin Liu wrote:
>>> Currently, administrators need to retrieve LACP mux state changes from
>>> the kernel DEBUG log using netdev_dbg and slave_dbg macros. To simplify
>>> this process, let's send the ifinfo notification whenever the mux state
>>> changes. This will enable users to directly access and monitor this
>>> information using the ip monitor command.
>>>
>>> To achieve this, add a new enum NETDEV_LACP_STATE_CHANGE in netdev_cmd.
>>>
>>> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
>>> ---
>>>  drivers/net/bonding/bond_3ad.c | 2 ++
>>>  include/linux/netdevice.h      | 1 +
>>>  net/core/dev.c                 | 2 +-
>>>  net/core/rtnetlink.c           | 1 +
>>>  4 files changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>>> index c6807e473ab7..bcd8b16173f2 100644
>>> --- a/drivers/net/bonding/bond_3ad.c
>>> +++ b/drivers/net/bonding/bond_3ad.c
>>> @@ -1185,6 +1185,8 @@ static void ad_mux_machine(struct port *port, bool *update_slave_arr)
>>>  		default:
>>>  			break;
>>>  		}
>>> +
>>> +		call_netdevice_notifiers(NETDEV_LACP_STATE_CHANGE, port->slave->dev);
>>>  	}
>>
>> This will cause sleeping while atomic because
>> ad_mux_machine() is called in atomic context (both rcu and bond mode
>> spinlock held with bh disabled) in bond_3ad_state_machine_handler().
> 
> Ah, that's why we call the bond_slave_state_notify() after spin_unlock_bh()?
> Where can I check if call_netdevice_notifiers() would sleep? So I can avoid
> this error next time.
> 
>> Minor (and rather more personal pref) I'd split the addition of the new
>> event and adding its first user (bond) for separate review.
> 
> Hmm, with out using call_netdevice_notifiers(). How about just call
> rtmsg_ifinfo() or rtmsg_ifinfo_event() directly?
> 
> Thanks
> Hangbin

Yep, I think that would be fine (w/ GFP_ATOMIC of course, if under the
locks).
Excuse me for the late response, have been having connectivity
issues the past few days.

Cheers,
 Nik