Re: [Bridge] [PATCH net-next v2] net: bridge: add STP xstats
On 11/12/2019 04:02, Vivien Didelot wrote:
> Hi Nikolay,
>
> On Tue, 10 Dec 2019 23:45:13 +0200, Nikolay Aleksandrov
> wrote:
>>> + if (p) {
>>> + nla = nla_reserve_64bit(skb, BRIDGE_XSTATS_STP,
>>> + sizeof(p->stp_xstats),
>>> + BRIDGE_XSTATS_PAD);
>>> + if (!nla)
>>> + goto nla_put_failure;
>>> +
>>> + memcpy(nla_data(nla), &p->stp_xstats, sizeof(p->stp_xstats));
>>
>> You need to take the STP lock here to get a proper snapshot of the values.
>
> Good catch! I see a br->multicast_lock but no br->stp_lock. Is this what
> you expect?
>
> spin_lock_bh(&br->lock);
> memcpy(nla_data(nla), &p->stp_xstats, sizeof(p->stp_xstats));
> spin_unlock_bh(&br->lock);
>
Yeah, this is a very old lock (pre-git) which needs some attention. :)
That is the lock and the above looks good to me.
>
> Thanks,
>
> Vivien
>
Cheers,
Nik
Re: [Bridge] [PATCH net-next v2] net: bridge: add STP xstats
On 12/10/19 2:20 PM, Vivien Didelot wrote:
> diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
> index 1b3c2b643a02..e7f2bb782006 100644
> --- a/include/uapi/linux/if_bridge.h
> +++ b/include/uapi/linux/if_bridge.h
> @@ -156,6 +156,15 @@ struct bridge_vlan_xstats {
> __u32 pad2;
> };
>
> +struct bridge_stp_xstats {
> + __u64 transition_blk;
> + __u64 transition_fwd;
> + __u64 rx_bpdu;
> + __u64 tx_bpdu;
> + __u64 rx_tcn;
> + __u64 tx_tcn;
> +};
> +
> /* Bridge multicast database attributes
> * [MDBA_MDB] = {
> * [MDBA_MDB_ENTRY] = {
> @@ -261,6 +270,7 @@ enum {
> BRIDGE_XSTATS_UNSPEC,
> BRIDGE_XSTATS_VLAN,
> BRIDGE_XSTATS_MCAST,
> + BRIDGE_XSTATS_STP,
> BRIDGE_XSTATS_PAD,
> __BRIDGE_XSTATS_MAX
> };
Shouldn't the new entry be appended to the end - after BRIDGE_XSTATS_PAD
Re: [Bridge] [PATCH net-next v2] net: bridge: add STP xstats
On 11/12/2019 17:38, David Ahern wrote:
> On 12/10/19 2:20 PM, Vivien Didelot wrote:
>> diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
>> index 1b3c2b643a02..e7f2bb782006 100644
>> --- a/include/uapi/linux/if_bridge.h
>> +++ b/include/uapi/linux/if_bridge.h
>> @@ -156,6 +156,15 @@ struct bridge_vlan_xstats {
>> __u32 pad2;
>> };
>>
>> +struct bridge_stp_xstats {
>> +__u64 transition_blk;
>> +__u64 transition_fwd;
>> +__u64 rx_bpdu;
>> +__u64 tx_bpdu;
>> +__u64 rx_tcn;
>> +__u64 tx_tcn;
>> +};
>> +
>> /* Bridge multicast database attributes
>> * [MDBA_MDB] = {
>> * [MDBA_MDB_ENTRY] = {
>> @@ -261,6 +270,7 @@ enum {
>> BRIDGE_XSTATS_UNSPEC,
>> BRIDGE_XSTATS_VLAN,
>> BRIDGE_XSTATS_MCAST,
>> +BRIDGE_XSTATS_STP,
>> BRIDGE_XSTATS_PAD,
>> __BRIDGE_XSTATS_MAX
>> };
>
> Shouldn't the new entry be appended to the end - after BRIDGE_XSTATS_PAD
>
Oh yes, good catch. That has to be fixed, too.
Re: [Bridge] [PATCH net-next v2] net: bridge: add STP xstats
Hi David, Nikolay,
On Wed, 11 Dec 2019 17:42:33 +0200, Nikolay Aleksandrov
wrote:
> >> /* Bridge multicast database attributes
> >> * [MDBA_MDB] = {
> >> * [MDBA_MDB_ENTRY] = {
> >> @@ -261,6 +270,7 @@ enum {
> >>BRIDGE_XSTATS_UNSPEC,
> >>BRIDGE_XSTATS_VLAN,
> >>BRIDGE_XSTATS_MCAST,
> >> + BRIDGE_XSTATS_STP,
> >>BRIDGE_XSTATS_PAD,
> >>__BRIDGE_XSTATS_MAX
> >> };
> >
> > Shouldn't the new entry be appended to the end - after BRIDGE_XSTATS_PAD
> >
>
> Oh yes, good catch. That has to be fixed, too.
>
This I don't get. Why new attributes must come between BRIDGE_XSTATS_PAD
and __BRIDGE_XSTATS_MAX?
Thanks,
Vivien
Re: [Bridge] [PATCH net-next v2] net: bridge: add STP xstats
From: Vivien Didelot
Date: Wed, 11 Dec 2019 13:41:33 -0500
> Hi David, Nikolay,
>
> On Wed, 11 Dec 2019 17:42:33 +0200, Nikolay Aleksandrov
> wrote:
>> >> /* Bridge multicast database attributes
>> >> * [MDBA_MDB] = {
>> >> * [MDBA_MDB_ENTRY] = {
>> >> @@ -261,6 +270,7 @@ enum {
>> >> BRIDGE_XSTATS_UNSPEC,
>> >> BRIDGE_XSTATS_VLAN,
>> >> BRIDGE_XSTATS_MCAST,
>> >> + BRIDGE_XSTATS_STP,
>> >> BRIDGE_XSTATS_PAD,
>> >> __BRIDGE_XSTATS_MAX
>> >> };
>> >
>> > Shouldn't the new entry be appended to the end - after BRIDGE_XSTATS_PAD
>> >
>>
>> Oh yes, good catch. That has to be fixed, too.
>>
>
> This I don't get. Why new attributes must come between BRIDGE_XSTATS_PAD
> and __BRIDGE_XSTATS_MAX?
Because, just like any other attribute value, BRIDGE_XSTATS_PAD is an
API and fixed in stone. You can't add things before it which change
it's value.
Re: [Bridge] [PATCH net-next v2] net: bridge: add STP xstats
Hi David,
On Wed, 11 Dec 2019 12:01:20 -0800 (PST), David Miller
wrote:
> >> >> /* Bridge multicast database attributes
> >> >> * [MDBA_MDB] = {
> >> >> * [MDBA_MDB_ENTRY] = {
> >> >> @@ -261,6 +270,7 @@ enum {
> >> >> BRIDGE_XSTATS_UNSPEC,
> >> >> BRIDGE_XSTATS_VLAN,
> >> >> BRIDGE_XSTATS_MCAST,
> >> >> + BRIDGE_XSTATS_STP,
> >> >> BRIDGE_XSTATS_PAD,
> >> >> __BRIDGE_XSTATS_MAX
> >> >> };
> >> >
> >> > Shouldn't the new entry be appended to the end - after BRIDGE_XSTATS_PAD
> >> >
> >>
> >> Oh yes, good catch. That has to be fixed, too.
> >>
> >
> > This I don't get. Why new attributes must come between BRIDGE_XSTATS_PAD
> > and __BRIDGE_XSTATS_MAX?
>
> Because, just like any other attribute value, BRIDGE_XSTATS_PAD is an
> API and fixed in stone. You can't add things before it which change
> it's value.
I thought the whole point of using enums was to avoid caring about fixed
numeric values, but well. To be more precise, what I don't get is that when
I move the BRIDGE_XSTATS_STP definition *after* BRIDGE_XSTATS_PAD, the STP
xstats don't show up anymore in iproute2.
Thanks,
Vivien
Re: [Bridge] [PATCH net-next v2] net: bridge: add STP xstats
From: Vivien Didelot Date: Wed, 11 Dec 2019 16:47:54 -0500 > I thought the whole point of using enums was to avoid caring about fixed > numeric values, but well. I don't get where you got that idea from. Each and every netlink attribute value is like IPPROTO_TCP in an ipv4 header, the exact values matter, and therefore you cannot make changes that modify existing values. Therefore, you must add new attributes to the end of the enumberation, right before the __MAX value. > To be more precise, what I don't get is that when > I move the BRIDGE_XSTATS_STP definition *after* BRIDGE_XSTATS_PAD, the STP > xstats don't show up anymore in iproute2. Because you ahve to recompile iproute2 so that it uses the corrected value in the kernel header, did you do that?
Re: [Bridge] [PATCH iproute2 v2] iplink: add support for STP xstats
On Tue, 10 Dec 2019 13:16:33 -0500 Vivien Didelot wrote: > Hi Stephen, > > On Mon, 9 Dec 2019 16:13:45 -0800, Stephen Hemminger > wrote: > > On Mon, 9 Dec 2019 18:05:22 -0500 > > Vivien Didelot wrote: > > > > > Add support for the BRIDGE_XSTATS_STP xstats, as follow: > > > > > > # ip link xstats type bridge_slave dev lan5 > > > STP BPDU: > > > RX: 0 > > > TX: 39 > > > STP TCN: > > > RX: 0 > > > TX: 0 > > > STP Transitions: > > > Blocked: 0 > > > Forwarding: 1 > > > IGMP queries: > > > RX: v1 0 v2 0 v3 0 > > > TX: v1 0 v2 0 v3 0 > > > ... > > > > Might I suggest a more concise format: > > STP BPDU: RX: 0 TX: 39 > > STP TCN: RX: 0 TX:0 > > STP Transitions: Blocked: 0 Forwarding: 1 > > ... > > I don't mind if you prefer this format ;-) Just trying to reduce the long output. Which is already too long for the xstat as it is.
Re: [Bridge] [PATCH net-next v2] net: bridge: add STP xstats
Hi David, On Wed, 11 Dec 2019 14:16:58 -0800 (PST), David Miller wrote: > > To be more precise, what I don't get is that when > > I move the BRIDGE_XSTATS_STP definition *after* BRIDGE_XSTATS_PAD, the STP > > xstats don't show up anymore in iproute2. > > Because you ahve to recompile iproute2 so that it uses the corrected value > in the kernel header, did you do that? Meh you were correct, my rebuild didn't pick up the header change :-/ I also moved the STP xstats copy below the mcast xstats copy to be consistent with the order. I'll respin right away. Thanks, Vivien
[Bridge] [PATCH iproute2 v4] iplink: add support for STP xstats
Add support for the BRIDGE_XSTATS_STP xstats, as follow:
# ip link xstats type bridge_slave dev lan4 stp
lan4
STP BPDU: RX: 0 TX: 61
STP TCN: RX: 0 TX: 0
STP Transitions: Blocked: 2 Forwarding: 1
Or below as JSON:
# ip -j -p link xstats type bridge_slave dev lan0 stp
[ {
"ifname": "lan0",
"stp": {
"rx_bpdu": 0,
"tx_bpdu": 500,
"rx_tcn": 0,
"tx_tcn": 0,
"transition_blk": 0,
"transition_fwd": 0
}
} ]
Signed-off-by: Vivien Didelot
---
include/uapi/linux/if_bridge.h | 10 ++
ip/iplink_bridge.c | 26 ++
2 files changed, 36 insertions(+)
diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
index 31fc51bd..9fefc7f3 100644
--- a/include/uapi/linux/if_bridge.h
+++ b/include/uapi/linux/if_bridge.h
@@ -156,6 +156,15 @@ struct bridge_vlan_xstats {
__u32 pad2;
};
+struct bridge_stp_xstats {
+ __u64 transition_blk;
+ __u64 transition_fwd;
+ __u64 rx_bpdu;
+ __u64 tx_bpdu;
+ __u64 rx_tcn;
+ __u64 tx_tcn;
+};
+
/* Bridge multicast database attributes
* [MDBA_MDB] = {
* [MDBA_MDB_ENTRY] = {
@@ -262,6 +271,7 @@ enum {
BRIDGE_XSTATS_VLAN,
BRIDGE_XSTATS_MCAST,
BRIDGE_XSTATS_PAD,
+ BRIDGE_XSTATS_STP,
__BRIDGE_XSTATS_MAX
};
#define BRIDGE_XSTATS_MAX (__BRIDGE_XSTATS_MAX - 1)
diff --git a/ip/iplink_bridge.c b/ip/iplink_bridge.c
index 06f736d4..bbd6f3a8 100644
--- a/ip/iplink_bridge.c
+++ b/ip/iplink_bridge.c
@@ -688,6 +688,7 @@ static void bridge_print_xstats_help(struct link_util *lu,
FILE *f)
static void bridge_print_stats_attr(struct rtattr *attr, int ifindex)
{
struct rtattr *brtb[LINK_XSTATS_TYPE_MAX+1];
+ struct bridge_stp_xstats *sstats;
struct br_mcast_stats *mstats;
struct rtattr *i, *list;
const char *ifname = "";
@@ -807,6 +808,29 @@ static void bridge_print_stats_attr(struct rtattr *attr,
int ifindex)
mstats->mld_parse_errors);
close_json_object();
break;
+ case BRIDGE_XSTATS_STP:
+ sstats = RTA_DATA(i);
+ open_json_object("stp");
+ print_string(PRINT_FP, NULL,
+"%-16sSTP BPDU: ", "");
+ print_u64(PRINT_ANY, "rx_bpdu", "RX: %llu ",
+ sstats->rx_bpdu);
+ print_u64(PRINT_ANY, "tx_bpdu", "TX: %llu\n",
+ sstats->tx_bpdu);
+ print_string(PRINT_FP, NULL,
+"%-16sSTP TCN: ", "");
+ print_u64(PRINT_ANY, "rx_tcn", "RX: %llu ",
+ sstats->rx_tcn);
+ print_u64(PRINT_ANY, "tx_tcn", "TX: %llu\n",
+ sstats->tx_tcn);
+ print_string(PRINT_FP, NULL,
+"%-16sSTP Transitions: ", "");
+ print_u64(PRINT_ANY, "transition_blk", "Blocked: %llu ",
+ sstats->transition_blk);
+ print_u64(PRINT_ANY, "transition_fwd", "Forwarding:
%llu\n",
+ sstats->transition_fwd);
+ close_json_object();
+ break;
}
}
close_json_object();
@@ -843,6 +867,8 @@ int bridge_parse_xstats(struct link_util *lu, int argc,
char **argv)
while (argc > 0) {
if (strcmp(*argv, "igmp") == 0 || strcmp(*argv, "mcast") == 0) {
xstats_print_attr = BRIDGE_XSTATS_MCAST;
+ } else if (strcmp(*argv, "stp") == 0) {
+ xstats_print_attr = BRIDGE_XSTATS_STP;
} else if (strcmp(*argv, "dev") == 0) {
NEXT_ARG();
filter_index = ll_name_to_index(*argv);
--
2.24.0
[Bridge] [PATCH net-next v3] net: bridge: add STP xstats
This adds rx_bpdu, tx_bpdu, rx_tcn, tx_tcn, transition_blk,
transition_fwd xstats counters to the bridge ports copied over via
netlink, providing useful information for STP.
Signed-off-by: Vivien Didelot
---
include/uapi/linux/if_bridge.h | 10 ++
net/bridge/br_netlink.c| 13 +
net/bridge/br_private.h| 2 ++
net/bridge/br_stp.c| 15 +++
net/bridge/br_stp_bpdu.c | 4
5 files changed, 44 insertions(+)
diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
index 1b3c2b643a02..4a58e3d7de46 100644
--- a/include/uapi/linux/if_bridge.h
+++ b/include/uapi/linux/if_bridge.h
@@ -156,6 +156,15 @@ struct bridge_vlan_xstats {
__u32 pad2;
};
+struct bridge_stp_xstats {
+ __u64 transition_blk;
+ __u64 transition_fwd;
+ __u64 rx_bpdu;
+ __u64 tx_bpdu;
+ __u64 rx_tcn;
+ __u64 tx_tcn;
+};
+
/* Bridge multicast database attributes
* [MDBA_MDB] = {
* [MDBA_MDB_ENTRY] = {
@@ -262,6 +271,7 @@ enum {
BRIDGE_XSTATS_VLAN,
BRIDGE_XSTATS_MCAST,
BRIDGE_XSTATS_PAD,
+ BRIDGE_XSTATS_STP,
__BRIDGE_XSTATS_MAX
};
#define BRIDGE_XSTATS_MAX (__BRIDGE_XSTATS_MAX - 1)
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index a0a54482aabc..60136575aea4 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -1607,6 +1607,19 @@ static int br_fill_linkxstats(struct sk_buff *skb,
br_multicast_get_stats(br, p, nla_data(nla));
}
#endif
+
+ if (p) {
+ nla = nla_reserve_64bit(skb, BRIDGE_XSTATS_STP,
+ sizeof(p->stp_xstats),
+ BRIDGE_XSTATS_PAD);
+ if (!nla)
+ goto nla_put_failure;
+
+ spin_lock_bh(&br->lock);
+ memcpy(nla_data(nla), &p->stp_xstats, sizeof(p->stp_xstats));
+ spin_unlock_bh(&br->lock);
+ }
+
nla_nest_end(skb, nest);
*prividx = 0;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 36b0367ca1e0..f540f3bdf294 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -283,6 +283,8 @@ struct net_bridge_port {
#endif
u16 group_fwd_mask;
u16 backup_redirected_cnt;
+
+ struct bridge_stp_xstatsstp_xstats;
};
#define kobj_to_brport(obj)container_of(obj, struct net_bridge_port, kobj)
diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index 1f1410f8d312..6856a6d9282b 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -45,6 +45,17 @@ void br_set_state(struct net_bridge_port *p, unsigned int
state)
br_info(p->br, "port %u(%s) entered %s state\n",
(unsigned int) p->port_no, p->dev->name,
br_port_state_names[p->state]);
+
+ if (p->br->stp_enabled == BR_KERNEL_STP) {
+ switch (p->state) {
+ case BR_STATE_BLOCKING:
+ p->stp_xstats.transition_blk++;
+ break;
+ case BR_STATE_FORWARDING:
+ p->stp_xstats.transition_fwd++;
+ break;
+ }
+ }
}
/* called under bridge lock */
@@ -484,6 +495,8 @@ void br_received_config_bpdu(struct net_bridge_port *p,
struct net_bridge *br;
int was_root;
+ p->stp_xstats.rx_bpdu++;
+
br = p->br;
was_root = br_is_root_bridge(br);
@@ -517,6 +530,8 @@ void br_received_config_bpdu(struct net_bridge_port *p,
/* called under bridge lock */
void br_received_tcn_bpdu(struct net_bridge_port *p)
{
+ p->stp_xstats.rx_tcn++;
+
if (br_is_designated_port(p)) {
br_info(p->br, "port %u(%s) received tcn bpdu\n",
(unsigned int) p->port_no, p->dev->name);
diff --git a/net/bridge/br_stp_bpdu.c b/net/bridge/br_stp_bpdu.c
index 7796dd9d42d7..0e4572f31330 100644
--- a/net/bridge/br_stp_bpdu.c
+++ b/net/bridge/br_stp_bpdu.c
@@ -118,6 +118,8 @@ void br_send_config_bpdu(struct net_bridge_port *p, struct
br_config_bpdu *bpdu)
br_set_ticks(buf+33, bpdu->forward_delay);
br_send_bpdu(p, buf, 35);
+
+ p->stp_xstats.tx_bpdu++;
}
/* called under bridge lock */
@@ -133,6 +135,8 @@ void br_send_tcn_bpdu(struct net_bridge_port *p)
buf[2] = 0;
buf[3] = BPDU_TYPE_TCN;
br_send_bpdu(p, buf, 4);
+
+ p->stp_xstats.tx_tcn++;
}
/*
--
2.24.0
Re: [Bridge] [PATCH net-next v3] net: bridge: add STP xstats
On 12/12/2019 03:07, Vivien Didelot wrote: > This adds rx_bpdu, tx_bpdu, rx_tcn, tx_tcn, transition_blk, > transition_fwd xstats counters to the bridge ports copied over via > netlink, providing useful information for STP. > > Signed-off-by: Vivien Didelot > --- > include/uapi/linux/if_bridge.h | 10 ++ > net/bridge/br_netlink.c| 13 + > net/bridge/br_private.h| 2 ++ > net/bridge/br_stp.c| 15 +++ > net/bridge/br_stp_bpdu.c | 4 > 5 files changed, 44 insertions(+) > Looks good to me, thanks! Acked-by: Nikolay Aleksandrov
