| Message ID | 20240620061053.1116077-1-liuhangbin@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] bonding: 3ad: send rtnl ifinfo notify when mux state changed | expand |
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
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
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
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;
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(-)