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

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

[Hangbin Liu]

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>
---

After this patch, we can see the following info with `ip -d monitor link`

7: veth1@if6: <BROADCAST,MULTICAST,SLAVE,UP,LOWER_UP> mtu 1500 qdisc noqueue master bond0 state UP group default
    link/ether 02:0a:04:c2:d6:21 brd ff:ff:ff:ff:ff:ff link-netns b promiscuity 0 allmulti 0 minmtu 68 maxmtu 65535
    veth
    bond_slave state BACKUP mii_status UP ... ad_aggregator_id 1 ad_actor_oper_port_state 143 ad_actor_oper_port_state_str <active,short_timeout,aggregating,in_sync,expired> ad_partner_oper_port_state 55 ad_partner_oper_port_state_str <active,short_timeout,aggregating,collecting,distributing> ...
7: veth1@if6: <BROADCAST,MULTICAST,SLAVE,UP,LOWER_UP> mtu 1500 qdisc noqueue master bond0 state UP group default
    link/ether 02:0a:04:c2:d6:21 brd ff:ff:ff:ff:ff:ff link-netns b promiscuity 0 allmulti 0 minmtu 68 maxmtu 65535
    veth
    bond_slave state ACTIVE mii_status UP ... ad_aggregator_id 1 ad_actor_oper_port_state 79 ad_actor_oper_port_state_str <active,short_timeout,aggregating,in_sync,defaulted> ad_partner_oper_port_state 1 ad_partner_oper_port_state_str <active> ...
7: veth1@if6: <BROADCAST,MULTICAST,SLAVE,UP,LOWER_UP> mtu 1500 qdisc noqueue master bond0 state UP group default
    link/ether 02:0a:04:c2:d6:21 brd ff:ff:ff:ff:ff:ff link-netns b promiscuity 0 allmulti 0 minmtu 68 maxmtu 65535
    veth
    bond_slave state ACTIVE mii_status UP ... ad_aggregator_id 1 ad_actor_oper_port_state 63 ad_actor_oper_port_state_str <active,short_timeout,aggregating,in_sync,collecting,distributing> ad_partner_oper_port_state 63 ad_partner_oper_port_state_str <active,short_timeout,aggregating,in_sync,collecting,distributing> ...

---
 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);
 	}
 }
 
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index c83b390191d4..76723091bbb2 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2891,6 +2891,7 @@ enum netdev_cmd {
 	NETDEV_OFFLOAD_XSTATS_REPORT_USED,
 	NETDEV_OFFLOAD_XSTATS_REPORT_DELTA,
 	NETDEV_XDP_FEAT_CHANGE,
+	NETDEV_LACP_STATE_CHANGE,
 };
 const char *netdev_cmd_to_name(enum netdev_cmd cmd);
 
diff --git a/net/core/dev.c b/net/core/dev.c
index c361a7b69da8..be6dfe185771 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1669,7 +1669,7 @@ const char *netdev_cmd_to_name(enum netdev_cmd cmd)
 	N(SVLAN_FILTER_PUSH_INFO) N(SVLAN_FILTER_DROP_INFO)
 	N(PRE_CHANGEADDR) N(OFFLOAD_XSTATS_ENABLE) N(OFFLOAD_XSTATS_DISABLE)
 	N(OFFLOAD_XSTATS_REPORT_USED) N(OFFLOAD_XSTATS_REPORT_DELTA)
-	N(XDP_FEAT_CHANGE)
+	N(XDP_FEAT_CHANGE) N(LACP_STATE_CHANGE)
 	}
 #undef N
 	return "UNKNOWN_NETDEV_EVENT";
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index eabfc8290f5e..5025b4f81b21 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -6689,6 +6689,7 @@ static int rtnetlink_event(struct notifier_block *this, unsigned long event, voi
 	case NETDEV_CHANGEINFODATA:
 	case NETDEV_CHANGELOWERSTATE:
 	case NETDEV_CHANGE_TX_QUEUE_LEN:
+	case NETDEV_LACP_STATE_CHANGE:
 		rtmsg_ifinfo_event(RTM_NEWLINK, dev, 0, rtnl_get_event(event),
 				   GFP_KERNEL, NULL, 0, 0, NULL);
 		break;
-- 
2.45.0

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

[Nikolay Aleksandrov]

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>
> ---
> 
> After this patch, we can see the following info with `ip -d monitor link`
> 
> 7: veth1@if6: <BROADCAST,MULTICAST,SLAVE,UP,LOWER_UP> mtu 1500 qdisc noqueue master bond0 state UP group default
>     link/ether 02:0a:04:c2:d6:21 brd ff:ff:ff:ff:ff:ff link-netns b promiscuity 0 allmulti 0 minmtu 68 maxmtu 65535
>     veth
>     bond_slave state BACKUP mii_status UP ... ad_aggregator_id 1 ad_actor_oper_port_state 143 ad_actor_oper_port_state_str <active,short_timeout,aggregating,in_sync,expired> ad_partner_oper_port_state 55 ad_partner_oper_port_state_str <active,short_timeout,aggregating,collecting,distributing> ...
> 7: veth1@if6: <BROADCAST,MULTICAST,SLAVE,UP,LOWER_UP> mtu 1500 qdisc noqueue master bond0 state UP group default
>     link/ether 02:0a:04:c2:d6:21 brd ff:ff:ff:ff:ff:ff link-netns b promiscuity 0 allmulti 0 minmtu 68 maxmtu 65535
>     veth
>     bond_slave state ACTIVE mii_status UP ... ad_aggregator_id 1 ad_actor_oper_port_state 79 ad_actor_oper_port_state_str <active,short_timeout,aggregating,in_sync,defaulted> ad_partner_oper_port_state 1 ad_partner_oper_port_state_str <active> ...
> 7: veth1@if6: <BROADCAST,MULTICAST,SLAVE,UP,LOWER_UP> mtu 1500 qdisc noqueue master bond0 state UP group default
>     link/ether 02:0a:04:c2:d6:21 brd ff:ff:ff:ff:ff:ff link-netns b promiscuity 0 allmulti 0 minmtu 68 maxmtu 65535
>     veth
>     bond_slave state ACTIVE mii_status UP ... ad_aggregator_id 1 ad_actor_oper_port_state 63 ad_actor_oper_port_state_str <active,short_timeout,aggregating,in_sync,collecting,distributing> ad_partner_oper_port_state 63 ad_partner_oper_port_state_str <active,short_timeout,aggregating,in_sync,collecting,distributing> ...
> 
> ---
>  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().

Minor (and rather more personal pref) I'd split the addition of the new
event and adding its first user (bond) for separate review.

Cheers,
 Nik

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

[Hangbin Liu]

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

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

[Nikolay Aleksandrov]

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